941876 - Firefox should tell SpiderMonkey which DOM element each bit of JS belongs to.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jim Blandy :jimb]

Firefox should use JS::CompileOptions::setElement and ...::setElementProperty to let the debugger see which element owns the JS code being compiled, and how.

Changeset cd5a9f4094bc (2013-11-21) adds two member functions to JS::CompileOptions, setElement and setElementProperty, which an embedding can call to indicate which DOM element owns the JS being compiled, and which property of that DOM element (if any) holds the JS text. The Debugger API can retrieve this information.

This data is among the last few things that Firebug currently uses the old JSD API to discover, so fixing this is a prerequisite for deleting JSD.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: When compiling script attached to DOM elements, tell the JS engine what element is involved and which attribute is involved, if any.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8336406 [details] [diff] [review]
When compiling script attached to DOM elements, tell the JS engine what element is involved and which attribute is involved, if any.

Bobby, this is ugly, but we can simplify the event handler bits a bit (by pushing them down into the compilation method) once we stop doing the two-step compile/clone dance, right?

Jim, does this do what you want?

Comment 3

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=6237c1a7f18e

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8336406 [details] [diff] [review]
When compiling script attached to DOM elements, tell the JS engine what element is involved and which attribute is involved, if any.

Review of attachment 8336406 [details] [diff] [review]:
-----------------------------------------------------------------

This all looks reasonable, but I've never really looked at this code before, so I wouldn't know if we're missing a path or something. Maybe smaug should review it.

(In reply to Boris Zbarsky [:bz] from comment #2)
> Bobby, this is ugly, but we can simplify the event handler bits a bit (by
> pushing them down into the compilation method) once we stop doing the
> two-step compile/clone dance, right?

I don't understand this.

::: content/base/src/nsScriptLoader.h
@@ +279,5 @@
>                            const nsAFlatString& aScript,
>                            void **aOffThreadToken);
>  
>    nsIScriptContext *GetScriptContext(JSObject **aGlobal);
> +  void FillCompileOptionsForRequest(JSContext* aCx,

Rather than adding new cx arg in places like this, I'd rather just use AutoJSContext in the callee. It's guaranteed to do the right thing.

::: content/events/src/nsEventListenerManager.cpp
@@ +900,5 @@
> +    if (NS_SUCCEEDED(nsContentUtils::WrapNative(cx, scope, mTarget,
> +                                                &targetVal))) {
> +      MOZ_ASSERT(targetVal.isObject());
> +
> +      // XXXbz do we want the function name or the attr name here?

The attr name, I'd think.

@@ +912,5 @@
> +                                                          str.Length()));
> +      NS_ENSURE_TRUE(jsStr, NS_ERROR_OUT_OF_MEMORY);
> +
> +      options.setElement(&targetVal.toObject())
> +             .setElementProperty(jsStr);

I think the SM API should rename this to 'setAttributeName'.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I don't understand this.

The codepath that binds the listener has to wrap the mTarget too, since it needs to stick it on the scopechain.

> Rather than adding new cx arg in places like this, I'd rather just use AutoJSContext in
> the callee

Hmm.  Is that fast enough that we don't care?  I mean, in this case both callers have a cx already...

> The attr name, I'd think.

OK, that will need more surgery.  :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

Actually, it shouldn't be too bad.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: When compiling script attached to DOM elements, tell the JS engine what element is involved and which attribute is involved, if any.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8336430 [details] [diff] [review]
When compiling script attached to DOM elements, tell the JS engine what element is involved and which attribute is involved, if any.

r+

This doesn't handle xul:scripts, but I don't know what should be reported there.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I will claim people generally don't use Firebug to debug xul:script.

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> I will claim people generally don't use Firebug to debug xul:script.
Yes, agree.

Honza

Comment 11

[Boris Zbarsky [:bzbarsky]]

Jan, since you're here, what exact string do you want attached to inline event handler compilation?  The attribute name or the property name or something else?

Comment 12

[Jim Blandy :jimb]

(In reply to Boris Zbarsky [:bz] from comment #9)
> I will claim people generally don't use Firebug to debug xul:script.

(Well, eventually, we want our devtools to present this kind of "code provenance" information as well.)

Comment 13

[Jim Blandy :jimb]

(In reply to Bobby Holley (:bholley) from comment #4)
> @@ +912,5 @@
> > +                                                          str.Length()));
> > +      NS_ENSURE_TRUE(jsStr, NS_ERROR_OUT_OF_MEMORY);
> > +
> > +      options.setElement(&targetVal.toObject())
> > +             .setElementProperty(jsStr);
> 
> I think the SM API should rename this to 'setAttributeName'.

Strongly agree. Filed as bug 942251.

Comment 14

[Boris Zbarsky [:bzbarsky]]

And of course the base changeset for that try run was busted.  :(  https://tbpl.mozilla.org/?tree=Try&rev=79ce0172cb91

Comment 15

[Jim Blandy :jimb]

Comment on attachment 8336430 [details] [diff] [review]
When compiling script attached to DOM elements, tell the JS engine what element is involved and which attribute is involved, if any.

Review of attachment 8336430 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! Thanks very much.

This should have some tests before landing, shouldn't it?

Comment 16

[Boris Zbarsky [:bzbarsky]]

How would I write tests for this?

Also, this is orange on try with compartment mismatches, in the usual way.  I'll fix that up.

Comment 17

[Boris Zbarsky [:bzbarsky]]

The issue seems to be as follows.

In nsEventListenerManager::CompileEventHandlerInternal we do this:

    JSAutoCompartment ac(cx, context->GetWindowProxy());


and then I added code that calls nsContentUtils::WrapNative, which by default will return the canonical wrapper, not something in the compartment of cx.

Bobby, is just telling it to go ahead and wrap into the compartment of cx the right thing to do here?  I _think_ that's what aAllowWrapping=true does, right?

Boy to I look forward to all EventTargets being on WebIDL bindings, where WrapNewBindingObject has no such gotchas.  :(

Comment 18

[Boris Zbarsky [:bzbarsky]]

That doesn't seem to help, fwiw.  See https://tbpl.mozilla.org/?tree=Try&rev=513e5db84f2b

God, I hate JSAPI.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK.  So what I see is that at the point when we make the WrapNative call cx is in the compartment of context->GetWindowProxy(), which is _not_ the same as the compartment of "scope".  The return value of WrapNative ends up in the compartment of "scope".

And of course the issue is that the static NativeInterface2JSObject in nsXPConnect.cpp helpfully enters the compartment of scope, which is totally not what we want here...  <sigh>

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Make the fragile APIs happy

Comment 21

[Boris Zbarsky [:bzbarsky]]

And it's still failing in browser-chrome tests.  In particular we get a compartment mismatch with this stack:

#0  js::ObjectImpl::initSlot (this=0x12511f0c0, slot=1, value=@0x112b8b530) at ObjectImpl.h:1351
#1  0x0000000101c47a54 in js::ScriptSourceObject::create (cx=0x116a98f80, source=0x1120cd780, options=@0x10f69d0c8) at jsscript.cpp:1013
#2  0x00000001015ee312 in js::frontend::CompileScript (cx=0x116a98f80, alloc=0x10f69d168, scopeChain={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x1020380c0}, evalCaller={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x1020380c0}, options=@0x10f69d0c8, chars=0x111a5f008, length=2828, source_=0x0, staticLevel=0, extraSct=0x0) at BytecodeCompiler.cpp:201
#3  0x0000000101c645f9 in js::WorkerThread::handleParseWorkload (this=0x112737668, state=@0x100656620) at jsworkers.cpp:744
#4  0x0000000101c63a89 in js::WorkerThread::threadLoop (this=0x112737668) at jsworkers.cpp:917
#5  0x0000000101c62867 in js::WorkerThread::ThreadMain (arg=0x112737668) at jsworkers.cpp:624

where frame 1 is:

1013        sourceObject->initSlot(ELEMENT_SLOT, ObjectOrNullValue(options.element()));

In this case, options.element is the <script> element involved and is in the compartment of the window that the script will in fact run in.  But at this point sourceObject (and cx) are both in some random JS-internal compartment:

(gdb) p sourceObject.ptr->compartment()->global_.value->getClass()->name
$23 = 0x10297fcf6 "internal-worker-global"

Jim, how is this stuff supposed to work, exactly?  That is, what exact input does this options API expect in this case?  Or should it be internally doing a JS_WrapValue as needed here?

Comment 22

[Jim Blandy :jimb]

This is weird. I have never heard of "internal-worker-global". Let me look a bit...

Comment 23

[Jim Blandy :jimb]

(... but I suspect off-main-thread compilation...)

Comment 24

[Jim Blandy :jimb]

Indeed, it's an off-main-thread compilation issue.

When we compile JS off the main thread, we create a separate, temporary compartment in which to do the compilation, with its own global, whose class is named "internal-worker-global". Once the compilation has produced a JSScript, we *merge the temporary compartment* with the compartment that actually requested the compilation, and *repoint all the references to the temporary global that we anticipate* to the proper global.

So, the ScriptSourceObject gets allocated in the temporary compartment, and then objects to being asked to point to the owning element, which is in the true target compartment.

This is something I hadn't anticipated when implementing the 'element' CompileOptions; I apologize for that. The SpiderMonkey tests really don't exercise off-main-thread compilation very well. In order to test it, we'll need to create a version of js/src/shell/js.cpp's OffThreadCompileScript that has all the options of js/src/shell/js.cpp's Evaluate.

Comment 25

[Jim Blandy :jimb]

One fix would be for ParseTask::init, or possibly OwningCompileOptions::copy, to not actually copy over those elements of the CompileOptions that have compartments --- and then for WorkerThreadState::finishParseTask to store them in the new script's ScriptSourceObject, after merging the compartments.

Other possibilities might be:
1) creating the ScriptSourceObject early, in the right compartment, and then attaching it when compilation is finished.

2) creating the ScriptSourceObject late, etc.

But it seems to me those involve a lot more plumbing, because they deviate from the current approach: creating the ScriptSourceObject in the same compartment as its JSScript, and letting the compartment merge put everything in the right place. Better to treat element and elementProperty (should be elementAttribute) as yet more things to be fixed up in finishParseTask, which is the place where we do all sorts of similar fixups.

Comment 26

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> Jan, since you're here, what exact string do you want attached to inline
> event handler compilation?  The attribute name or the property name or
> something else?
I think it should be the attribute name. 

I believe that:

1) The owner-element is <body> and attribute "onload":
<body onload="console.log('case1')">

2) The owner-element is <script> (no attribute, empty string):
<script type="text/javascript">
document.body.onload = function() {
    console.log('case2');
}
</script>

Honza

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 1) The owner-element is <body> and attribute "onload":
> <body onload="console.log('case1')">

That's actually not what the attached patch implements, because in this case the event listener isn't on the body, so mTarget is the window and hence there is no element to set when we compile.

If we need this case to work, then we need some more surgery on how event handlers are generally handled....

Comment 28

[Bobby Holley (:bholley)]

re: comment 24: yikes.

Boris - yeah, it'll be nice when we can kill that aAllowWrapping stuff.

Let me know if there's any other input you need.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Let me know if there's any other input you need.

Just review on that patch.

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 8337299 [details] [diff] [review]
Make the fragile APIs happy

Review of attachment 8337299 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley with comments addressed.

::: content/base/src/nsScriptLoader.cpp
@@ +982,3 @@
>    if (NS_SUCCEEDED(nsContentUtils::WrapNative(cx, aScopeChain,
>                                                aRequest->mElement, &elementVal,
>                                                nullptr, true))) {

Please do |/* aAllowWrapping = */ true|.

::: content/events/src/nsEventListenerManager.cpp
@@ +903,2 @@
>      if (NS_SUCCEEDED(nsContentUtils::WrapNative(cx, scope, mTarget,
> +                                                &targetVal, nullptr))) {

Instead, how about we just pass JS::CurrentGlobalOrNull(cx) as scope, and nix the manual wrapping below?

Also, please add /* aAllowWrapping = */ to make the bool param clearer.

Comment 31

[Jim Blandy :jimb]

(In reply to Jan Honza Odvarko from comment #26)
> I believe that:
> 
> 1) The owner-element is <body> and attribute "onload":
> <body onload="console.log('case1')">
> 
> 2) The owner-element is <script> (no attribute, empty string):
> <script type="text/javascript">
> document.body.onload = function() {
>     console.log('case2');
> }
> </script>

This is my understanding of what would be valuable as well.

Comment 32

[Jim Blandy :jimb]

(In reply to Boris Zbarsky [:bz] from comment #27)
> > 1) The owner-element is <body> and attribute "onload":
> > <body onload="console.log('case1')">
> 
> That's actually not what the attached patch implements, because in this case
> the event listener isn't on the body, so mTarget is the window and hence
> there is no element to set when we compile.
> 
> If we need this case to work, then we need some more surgery on how event
> handlers are generally handled....

So, you're saying that even thought the 'onload' attribute appears on a <body> element, the listener is moved to the window, not the element?

The intended use of the 'element' CompileOptions field is to help debugger users find out where a given piece of code came from. So here, the <body> element would be the helpful value for CompileOptions::element() to return.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> So, you're saying that even thought the 'onload' attribute appears on a <body> element,
> the listener is moved to the window, not the element?

Yep, for the special cases of <body> and <frameset>.  Gotta love backwards compat on the web.  ;)

I can return the element here, but it'll need more extensive changes to punch the element down to the relevant code.  Will do that in a bit.

Comment 34

[Jim Blandy :jimb]

I have a patch to rename elementProperty to elementAttributeName up in bug 942251; you might want to slip that into your patch queue.

Comment 35

[Jim Blandy :jimb]

(In reply to Boris Zbarsky [:bz] from comment #33)
> > So, you're saying that even thought the 'onload' attribute appears on a <body> element,
> > the listener is moved to the window, not the element?
> 
> Yep, for the special cases of <body> and <frameset>.  Gotta love backwards
> compat on the web.  ;)
> 
> I can return the element here, but it'll need more extensive changes to
> punch the element down to the relevant code.  Will do that in a bit.

GO BZ GO

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Instead, how about we just pass JS::CurrentGlobalOrNull(cx) as scope, and nix the manual
> wrapping below?

Worksforme.

Updated patches coming up once I verify that try is happy with them.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Try push with the known-busted <script> stuff commented out so I can focus on just the event handler bits: https://tbpl.mozilla.org/?tree=Try&rev=9e6cf069f523

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Pass in the element we're compiling the event handler for to the event listener manager, so it knows which element to associate with the compiled script.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 2 from previously reviewed patch

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. When compiling script attached to DOM elements, tell the JS engine what element is involved and which attribute is involved, if any.

Comment 41

[Boris Zbarsky [:bzbarsky]]

> This is something I hadn't anticipated when implementing the 'element' CompileOptions

Jim, is there a bug tracking that bit?  Should probably block this one...

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8338580 [details] [diff] [review]
part 1.  Pass in the element we're compiling the event handler for to the event listener manager, so it knows which element to associate with the compiled script.

ok. <body> and <frameset> make this API a bit odd, but
if devtools want this behavior....

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8338583 [details] [diff] [review]
part 2.  When compiling script attached to DOM elements, tell the JS engine what element is involved and which attribute is involved, if any.

>+    uint32_t lineNo = 1;
>+    nsAutoCString url;
>+    aElement->OwnerDoc()->GetDocumentURI()->GetSpec(url);
Technically GetDocumentURI may return null, so add a null check.

Comment 44

[Jim Blandy :jimb]

(In reply to Boris Zbarsky [:bz] from comment #41)
> Jim, is there a bug tracking that bit?  Should probably block this one...

Filed as bug 944121. I may not be able to get to this before Monday.

Comment 45

[Jim Blandy :jimb]

Okay, bug 944121 and its blocker have all the necessary patches to let this work move forward.

Comment 46

[Jim Blandy :jimb]

Okay, I think all the blockers' patches are landed now.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merged to tip

Comment 48

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4066993a6a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb3ea68c86e

Jim, you should be good to go here.

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bb4066993a6a
https://hg.mozilla.org/mozilla-central/rev/fbb3ea68c86e

Comment 50

[Jim Blandy :jimb]

(Marked as 'in-testsuite' because bug 964507 is now landed.)