Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get console messages to go code #77

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Conversation

stroiman
Copy link

@stroiman stroiman commented Dec 5, 2024

This adds the ability to get console messages to Go, but to do that you need to tap into the "inspector", which is also AFAIK the entry point to controlling the debugger. I just need the console messages. Although it seems that I can also use it to control time; which would be useful for my purpose.

This is still WIP, functionally close to done (for my needs), but I'd appreciate feedback, and opinion on code structure.

I also intend to add code-doc before merging.

Inheriting from InspectorClient

In C++, you must create a class inheriting from InspectorClient. This must then be installed using Inspector::Create, and you need to manually attach to each v8 context.

The InspectorClient has all virtual functions, and the client code overrides the ones they care about.

My idea was to create a Go interface for each capability. The user can choose to implement the functions they care about.

So far, I only care about console messages, so that's what I have now. It it still lacking stack traces, but should be simple enough.

When registering an inspector, you need a name, and a contextGroupId. I have no idea what they are used for, except contextGroupId must be <> 0, otherwise nothing happens. Right now they are hardcoded in C++ code.

That also means that I don't know if it makes sense to control from Go code or not.

The client itself does not seem to be related to an isolate. I don't know if it's valid to use the same client for multiple isolates. The code seems to indicate it's possible.

Utility types

The code uses a StringView type for strings. In reality; they're always utf16le (i.e. no byte order mark). Nothing is documented, so all is from experimenting, and only on one platform, my M2 mac. Btw, there must be a better way of converting []byte to []uint16, but I didn't find anything. Btw, I will add a test case that uses some weird unicode characters that take up more than one element in utf16. I did test that manually, and it did work, but better have it in the tests.

I created a more generic type for registering "callbacks". Just like the Isolate registers function callbacks, which I think should just be replaced with that type; which should be moved somewhere else.

Package name

If we follow the idea to create a separate interface for each capability that the inspector supports, it will be quite a lot of types, not necessarily with a sensible name of their own without some context. Prefixing them with InspectorClient makes them even longer, they're long enough IMHO.

I was wondering if it would make sense to put it in a sub-package, v8go/inspector. The C++ code does have it's own namespace, v8_inspector.

p.s. it's only now I see that I have reformatted function_template.go. I have just opened and saved it

@stroiman stroiman marked this pull request as draft December 5, 2024 09:34
@stroiman
Copy link
Author

stroiman commented Dec 5, 2024

Btw, the initial code here predates the previous PR. I noticed I have created InspectorPtr and InspectorClientPtr types, which really isn't necessary, and I'm in favour of just replacing with *v8Inspector / *v8InspectorClient.

If you expressed your opinion on that topic, I have missed that.

@stroiman
Copy link
Author

stroiman commented Dec 5, 2024

I was just browsing the Go documentation, and I notice that CGo has a mechanism for passing Go values to C, and get it back again:

https://pkg.go.dev/runtime/cgo#Handle

So the registry thing seems unnecessary.

@tommie
Copy link
Owner

tommie commented Dec 7, 2024

Btw, the initial code here predates the previous PR. I noticed I have created InspectorPtr and InspectorClientPtr types, which really isn't necessary, and I'm in favour of just replacing with *v8Inspector / *v8InspectorClient.

If you expressed your opinion on that topic, I have missed that.

No, I didn't. I don't see the point of the Ptr types, but I also don't have a strong opinion.

@stroiman
Copy link
Author

stroiman commented Dec 7, 2024

Allright, I'll replace it with normal pointers, IMHO it's a code convention that adds more code in type declarations with no apparent benefit. I can easily imagine it's a result of finally finding a solution that compiles after struggling with the Go-compatible C-types vs. C++ types problem.

@stroiman
Copy link
Author

stroiman commented Jan 3, 2025

Finally got around to update this.

I added documentation to types, replaced the registry with cgo.Handle types, and removed the ...Ptr types.

While I'm in this branch, I'll just check to see if I can control time. https://v8.github.io/api/head/classv8__inspector_1_1V8InspectorClient.html#ac27be4891cda5413f6156df6c98dc831

I assume that this controls the time reported by new Date() in JS land, which would be very useful to control from my purpose.

While that function use a double to represent time, I would use a Go time.Time type, as that would be the idiomatic way to represent a point in time.

Controlling time isn't achieved through the inspector, but Platform::CurrentClockTimeMilliseconds, so I'll leave this for now.

@stroiman stroiman marked this pull request as ready for review January 3, 2025 12:32
Copy link
Owner

@tommie tommie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a CHANGELOG entry for this added functionality.

inspector.cc Show resolved Hide resolved
inspector.cc Outdated
void InspectorContextCreated(InspectorPtr inspector, ContextPtr context) {
LOCAL_CONTEXT(context);
int groupId = 1;
StringView name = StringView((const uint8_t*)"Test", 4);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this used for? Why "Test"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. It's called "humanReadableName", though I haven't found out where it's used. But I didn't realise you can create a StringView without constructor args.

The groupId appears to be sent back to the caller in the callback, consoleAPIMessage. It must be non-zero (no messages are received if it's zero).

Maybe I should expose both of these as arguments to the caller, but I was preferring a more minimal interface, rather than forcing the caller to pass arguments it has no use for.

"unicode/utf16"
)

type MessageErrorLevel uint8
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to take the values from the underlying enum, but at least document this is a copy of v8::Isolate::MessageErrorLevel.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. For the values, I adapted how e.g., PropertyAttribute is also implemented.

I think even linking to v8 documentation would make sense, but this doesn't look like a stable URL>

https://v8.github.io/api/head/classv8_1_1Isolate.html#acfc7c4d5c93913201249045e2655d3dd

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, though https://v8.github.io/api/head/classv8_1_1Isolate.html is probably useful on its own.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some documentation for the exported functions and types? Ideally for someone who hasn't (yet) read the C++ docs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume, you mean the "exported to C" code function? goHandleConsoleAPIMessageCallback? Done!

inspector.go Outdated
Url string
LineNumber uint
ColumnNumber uint
// stackTrace StackTrace
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this and the TODO below and instead document that stack traces aren't implemented.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment about how the fields map to C++ function arguments, and a link to the InspectorClient C++ class documentation.

inspector.go Show resolved Hide resolved
inspector.go Outdated Show resolved Hide resolved
inspector.h Show resolved Hide resolved
inspector_test.go Outdated Show resolved Hide resolved
inspector_test.go Outdated Show resolved Hide resolved
Copy link
Owner

@tommie tommie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay.

inspector.cc Show resolved Hide resolved
"unicode/utf16"
)

type MessageErrorLevel uint8
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, though https://v8.github.io/api/head/classv8_1_1Isolate.html is probably useful on its own.

inspector.go Show resolved Hide resolved
inspector.h Show resolved Hide resolved
@stroiman
Copy link
Author

Sorry for the delay.

He, me too, I think there are still a few things I wanted to follow up on, in particular having a test case with special characters that have larger utf16-sequences.

@stroiman
Copy link
Author

For the utf-16->utf-8 decoding, I had two ideas

  • Would it make sense to do the decoding in C++ instead of Go? But I have no idea how to do that.
  • The C++ code could prefix the string 0xFEFF (check it's not already there). That way, Go code just needs to treat it as UTF-16, and can forget about endianness.

However, I'm not sure that is the right way. Looking at v8-inspector.h, I see this

  // TODO(dgozman): add DCHECK(m_is8Bit) to accessors once platform can be used
  // here.
  const uint8_t* characters8() const { return m_characters8; }
  const uint16_t* characters16() const { return m_characters16; }

This indicates that the Platform is supposed to control this (eventually). So it would appear that we should be able to control the encoding, and therefore be able to make assumptions about it too.

@stroiman
Copy link
Author

Regarding the context group id specified here (where I found that it doesn't work with zero-value)

void InspectorContextCreated(v8Inspector* inspector, ContextPtr context) {
  LOCAL_CONTEXT(context);
  int groupId = 1;
  V8ContextInfo info = V8ContextInfo(local_ctx, groupId, StringView());
  inspector->contextCreated(info);
}

There are functions on the Inspector that use the group id, e.g. connecting an inspector session (which seems to just be a different name for a debugger). And resetContextGroup, whatever that does.

My conclusion is that the intention is that e.g., for a web page with <iframe>s, each frame has a separate context, but participate in the same context group, so one session can monitor the entire page.

This is just FYI, just sharing what I learned about the apparent purpose of these undocumented values are used. Didn't yet find where the "human readable name" is used, but didn't specifically look for it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants