961327 - Firefox should tell SpiderMonkey why it called JavaScript code

Status: RESOLVED DUPLICATE of bug 961325

(Core :: General, defect)

Description

[Jim Blandy :jimb]

Whenever Firefox asks SpiderMonkey to run some JavaScript, it should use the API proposed in bug 961325 to tell SpiderMonkey why. Debuggers and heap profilers can use this data.

Comment 1

[Boris Zbarsky [:bzbarsky]]

What sort of data structure will this API need?  Just a const char*, or something else?

Comment 2

[Jim Blandy :jimb]

These reasons will almost never be asked for, so they should be cheap when not used. I was thinking that we should have an abstract base class roughly like:

struct JSExecutionReason {
    virtual JSObject *explain(JSContext *cx) = 0;
};

and then an RAII class like:

struct AutoJSExecutionReason {
  AutoJSExecutionReason(JSContext *cx, JSExecutionReason &reason);
  ~AutoJSExecutionReason();
};

Then call sites could say:

class EventHandlerCallReason: JSExecutionReason {
  JSObject *explain(JSContext *cx) MOZ_OVERRIDE;
};

{
  ...
  EventHandlerCallReason reason(event); // for example
  AutoJSExecutionReason(cx, reason);
  ... perform JS call(s) ...
}

JSContext would acquire a JSExecutionReason * member, which AutoCallReason would save, supplant, and restore. This would be saved in the oldest js::Activation instances, so it could be retrieved at the appropriate point when traversing the stack. If the code walking the stack was interested, it could invoke the 'explain' member, which would construct a nice JSObject like { type: "eventHandler", ... } where the further properties provided details appropriate to the type.

Overengineered? A 'const jschar *' is better than nothing!

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.

This seems plausibly doable, yes.

Comment 4

[Jim Blandy :jimb]

Hopefully it's clear, but: in the common case, the JSExecutionReason concrete class instance would never construct anything --- it would just hold onto enough information from the ambient C++ code to produce an object if requested. The intent is to be lightweight when unused.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Right.  It'll take a bit of work, because in the common case the C++ code that calls into JS is actually autogenerated.  So either we need to make all callers of the generated code supply reasons, or we generate a default reason which is good enough in most cases (e.g. "calling EventListener.handleEvent") and then allow the caller to override (e.g. for when the caller is calling a dom::Function, so the callee has no way to provide anything interesting).

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v1

My work-in-progress extracted from the patch in bug 961325.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v2

Rebased.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v3

I got Cu.evalInSandbox working with a couple of tests an I added an execution reason for Cu.evalInWindow (with a test) that asserts currently. I probably need to use some wrapper magic when storing the window parameter, or maybe not bother storing the window at all and instead keep its most useful properties, like maybe location and windowID.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add JS execution reasons for miscellaneous Firefox locations

I decided to focus my efforts on the most important JS execution code paths first: script loading, events, evalInSandbox, etc. Therefore I've extracted the bits that aren't super important right now and I'm attaching them in a separate patch. I expect to get back to this once the important stuff is finished first.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v4

This is the main patch and it currently implements execution reasons for loading script tags in documents (HTML and XUL) and Cu.evalInSandbox. I have tests that verify the API works and I plan to add tests for each new location I add going forward.

The only puzzling issue in this patch is that adding a script in a XUL document creates an execution reason with about:blank as the URL, not the actual data: URL that is injected. It might be that I'm setting the execution reason too soon, or that it's a peculiarity of the particular test setup, but I haven't figured that out yet.

Comment 11

[Boris Zbarsky [:bzbarsky]]

JS_DefineProperty, please, not JS_SetProperty.

For the other, xul:iframe might be doing weird stuff in terms of the initial about:blank....

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v5

Switched to JS_DefineProperty and added support for event handlers. My only problem is that I don't know how to get a reference to the element from the Call method, so I could use something like this:

def getExecutionReason(self):
    return ("uint32_t lineNo = 0;\n"
            "nsAutoCString url (NS_LITERAL_CSTRING(\"-moz-evil:lying-event-listener\"));\n"
            "nsIURI *uri = mElement->OwnerDoc()->GetDocumentURI();\n"
            "if (uri) {\n"
            "  uri->GetSpec(url);\n"
            "  lineNo = 1;\n"
            "}"
            "AutoEventHandlerReason reason(cx, url, lineNo);\n")

How do I get something like mElement above?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I don't know how to get a reference to the element from the Call method

Which "the element"?  Callbacks are used for many many things other than event handlers.  Typically your execution reason will need to go in whatever code calls the callback, unless you want a very generic execution reason of some sort (e.g. the callback's name in the IDL or some such)...

Certainly the callback has no idea what the url of its caller is.

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

What would those other things be? Do they include timeout and XHR callbacks for example? I would definitely want to treat those differently, but I don't care (at this point at least) about differentiating between a load and a click event handler.

Would EventListenerManager::CompileEventHandlerInternal be a logical place for setting the execution reason? Can I assume that no other Activation will appear between callback compilation and execution?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> What would those other things be?

A probably incomplete list: webaudio decode error/success callbacks, various camera API callbacks, event handlers, event listeners, geolocation callbacks, canvas print/toblob/etc callbacks, media query change callbacks, mutation observers, user media error/success callbacks, idle observer callbacks, treewalker/nodeiterator nodefilter callbacks, the argument to the Promise constructor, arguments to Promise.prototype.then, some more webrtc stuff, settings manager observers, web components lifecycle callbacks, setTimeout/setInterval, requestAnimationFrame.

Plus whatever I missed or gets added for new specs.

Note that XHR callbacks are in fact event handlers and event listeners.

> Would EventListenerManager::CompileEventHandlerInternal be a logical place for setting
> the execution reason?

You want this to be set when executing, not compiling, right?

If so, I'd think that doing this in EventListenerManager::HandleEventSubType would make the most sense.

This can probably lead to some weirdness if the event listener is C++ (or even JS that calls into other C++) that then calls some JS that breaks in the debugger, because the reason will look like "Event listener", but that's a matter of adding these execution reasons at other callsites...

> Can I assume that no other Activation will appear between callback compilation and
> execution?

Absolutely not.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Looking at the first patch, I had a few other comments:

1) The plugin thing doesn't make much sense to me: that's the page calling into the plugin, not us calling into JS.

2) I'm not a huge fan of the amount of JSAPI involved in setting up these things.  It would be much simpler for all the Gecko consumers to not actually use createWithType but instead to use a WebIDL dictionary, set it up, then create a JS object from it.  Given that createWithType is just creating a vanilla object, this should be black-box identical and _way_ shorter and less error prone.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v6

Moved the executionReason setting to EventListenerManager::HandleEventSubType and it seems to work well. I had a hard time figuring out how to get a JSContext and the element to grab the URI from, and it turns out that those are not always available (particularly during startup). I don't know if there is a better way to do this and I'll be glad to hear about any.

Haven't looked at the WebIDL dictionary idea yet, I need to read up on it and find some examples. That's for next week.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add JS execution reasons for miscellaneous Firefox locations v2

Removed the plugin execution reason. It wasn't well thought out anyway.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I had a hard time figuring out how to get a JSContext

Getting _a_ JSContext in general is easy as long as you don't mind it being slow.  The hard part usually is finding the right compartment.  Assuming you care about the compartment, of course...

However, in practice explain() is passed a JSContext (presumably in the right compartment), and the only reason you need the JSContext the construction site is to stick yourself on its linked list.  But it's not clear to me why that linked list lives on the JSContext instead of the JSRuntime in the first place...  Does whatever consumes these things walk all contexts in the runtime?  Because if you need "the JSContext the execution will happen on", then you're slightly in trouble with the code as it is now; it would need to be pushed down to the point where we actually know what that JSContext is or something.  In particular, the JSContext an event listener will run on today is not necessarily the thing you're grabbing in the patch.

> and the element to grab the URI from

You probably want to grab the URI from the target's GetOwnerGlobal() or something?  Unless you want an event listener on an XHR result document to have as the URI the URI the XHR document came from instead of the URI of the page that did the XHR.

Also, getting the URI string eagerly is probably not great performance-wise.  Ideally we'd only do it if explain() is called.  The AutoEventHandlerReason could hold on to the EventTarget and interrogate it in explain().  That would also fix the bug where you're holding on to a pointer (url.get()) that's gone out of scope and points to garbage....

Let me know if you want help with the WebIDL dictionary thing; I'm happy to throw something together as an example.

Comment 20

[Bobby Holley (:bholley)]

Yeah, please don't add anything new to JSContext. Stuff should either go on JSRuntime or JSCompartment.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Boris Zbarsky [:bz] from comment #19)
> Let me know if you want help with the WebIDL dictionary thing; I'm happy to
> throw something together as an example.

If you have the time that would be immensely helpful!

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: WebIDL dictionary bits for

Something like this, for the dictionary approach

Comment 23

[Boris Zbarsky [:bzbarsky]]

Note that bug 987112 will nix that useless second argument to ToObject, simplifying the code a bit.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v7

In this iteration I've made the following changes:
- The reference to the execution reason is now stored in the JSRuntime instead of the JSContext.
- Used the WebIDL dictionary for creating Gecko types instead of createWithType().
- Added an eventType property to the IDL that I think will be useful for event handlers.

I don't have a good answer to the question about mURI vs. mURL since I don't know the cases in which they differ, so I left it as it was. I do have a couple of questions though:

- I didn't use the WebIDL dictionary for Sandbox, should I have?
- Is there a simpler way to get the JSRuntime in EventListenerManager::HandleEventSubType?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> mURI vs. mURL since I don't know the cases in which they differ

mURI is the actual URI the script was loaded from.

mURL is what the JSScripts "filename" will be set to.  It typically looks like "document URI -> script URI" and all this only matters in the case of a chrome document linking to a non-chrome URI or something.

>- I didn't use the WebIDL dictionary for Sandbox, should I have?

You might as well, I think.  Pretty much anything in Gecko can use it except SpiderMonkey proper.

> - Is there a simpler way to get the JSRuntime in
> EventListenerManager::HandleEventSubType?

Yes.  If you want a way that also works in web workers, btw, you do need something different than what you have right now.

In any case, for something that works for workers as well as mainthread, you can do:

  JS_GetRuntime(nsContentUtils::GetDefaultJSContextForThread())

Comment 26

[Boris Zbarsky [:bzbarsky]]

Though it sure would be nice if we had a fast way to just get the current-thread JSRuntime.  Bobby?

Comment 27

[Boris Zbarsky [:bzbarsky]]

One other thing.  If you wanted to, you could not give eventType a default value, which would mean it would not get defined on the object unless it was explicitly set.  Whether that's better than having it default to defining "" depends on what consumers will want.

Comment 28

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #26)
> Though it sure would be nice if we had a fast way to just get the
> current-thread JSRuntime.  Bobby?

CycleCollectedJSRuntime::Get().

Comment 29

[Boris Zbarsky [:bzbarsky]]

Ah, CycleCollectedJSRuntime::Get()->Runtime() returns a JSRuntime*.  Perfect!

Comment 30

[Andrew McCreight [:mccr8]]

CycleCollectedJSRuntime really should be renamed to something more generic.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v8

Thanks for the mURI/mURL clarification, we don't use mURL in the debugger server or client code, in fact we always split URLs before showing them to the user. So it makes sense to prefer mURI in this case. Changes in this version:

* Converted the code in Sandbox.cpp to use the WebIDL dictionary.
* Used the simpler method to grab the JSRuntime reference.
* Changed the optional eventType attribute to not have a default value. It doesn't matter for the debugger server as it always uses truthy tests, but who knows what other code might end up doing.

I'm holding off on adding execution reasons for workers until we make workers debuggable, so I can actually write tests. I'll take care of eval() and friends in SpiderMonkey next and I think that should be OK for a first cut.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

I've combined this patch into the one in bug 961325, since the approach has changed considerably.