871306 - Remove JS_GetGlobalObject from JSD

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This looks kinda nasty. I'll see if I can make any simplifying assumptions here.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Add an API to explicitly access |glob| and use it in jsd. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add an API for directly accessing the default JSD global and use it in ActivateDebugger. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove globalObject from jsdIFilter. v1

It looks like firebug only ever passes null, which is equivalent to not using
it at all.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use the current global rather than the default global in jsdContext::GetGlobalObject. v1

As far as I can tell from the IDL docs and digging through the Firebug source,
this is what we want here.

Comment 5

[Bobby Holley (:bholley)]

CCing Honza as a heads-up.

Honza, hopefully none if this breaks firebug. The two API changes are:
1 - Dropping support for globalObject on jsdIFilter. Firebug only ever appears to pass null here.
2 - Returning the current global for the scope chain (that is to say, the global of the current compartment), rather than the default global in jsdIContext::globalObject.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 752826 [details] [diff] [review]
Part 1 - Add an API for directly accessing the default JSD global and use it in ActivateDebugger. v1

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

-    JS::RootedObject glob(cx, JS_GetGlobalObject (cx));
+    JS::RootedObject glob(cx, JSD_GetDefaultGlobal(mCx));

an extra space is missing after GetDefaultGlobal to match this crazy coding style in this file

Comment 7

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 752827 [details] [diff] [review]
Part 2 - Remove globalObject from jsdIFilter. v1

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

Should we update some docs about this?

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 752829 [details] [diff] [review]
Part 3 - Use the current global rather than the default global in jsdContext::GetGlobalObject. v1

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

Ok, this looks all good now, let's hope we have not caused any extra work on firebug side with this.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> Should we update some docs about this?

Are there any?

Honza is basically the only consumer of jsd at this point, and hopefully will be the last one there ever is. So as long as he's in the loop it's probably fine. Either way, he probably knows what to update. :-)

Comment 10

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/23eecc43bbfd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a4df8651a2a4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ca3f8a75b3b8

Comment 11

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

(In reply to Bobby Holley (:bholley) from comment #5)
> CCing Honza as a heads-up.
> 
> Honza, hopefully none if this breaks firebug. The two API changes are:
> 1 - Dropping support for globalObject on jsdIFilter. Firebug only ever
> appears to pass null here.
> 2 - Returning the current global for the scope chain (that is to say, the
> global of the current compartment), rather than the default global in
> jsdIContext::globalObject.

Thanks for the heads up!
Honza

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/23eecc43bbfd
https://hg.mozilla.org/mozilla-central/rev/a4df8651a2a4
https://hg.mozilla.org/mozilla-central/rev/ca3f8a75b3b8

Comment 13

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

So, one problem appeared. Firebug is using 'frame.executionContext.globalObject' (within onDebug JSD hook) to find out what window the current frame is coming from

https://github.com/firebug/firebug/blob/firebug1.11/extension/content/firebug/js/debugger.js#L1465

Is there any other way how to get the current window for the frame?

Honza

Comment 14

[Gabor Krizsanits (INACTIVE)]

(In reply to Jan Honza Odvarko from comment #13)
> Is there any other way how to get the current window for the frame?

Well... that should exactly happen now... I'm not sure I understand your definition for the current window, currently frame.executionContext.globalObject should return the global where the function in which we've stopped was defined.

What does it return now? Does it return null, or some other window than you would expect? What is context.stoppedGlobal used exactly in fB?

Unfortunately I don't think there is a way to get the same thing from executionContext as before right now, and I don't see an easy way to fix that without backing this patch out, which would be quite unfortunate. But I'm very curious what is this piece of information used in Fb on the first place, to understand the whole problem space. In particular this property should return the same object in most cases as before... So could you explain me some more about the issue you're facing?

Comment 15

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> (In reply to Jan Honza Odvarko from comment #13)
> > Is there any other way how to get the current window for the frame?
> 
> Well... that should exactly happen now... I'm not sure I understand your
> definition for the current window, currently
> frame.executionContext.globalObject should return the global where the
> function in which we've stopped was defined.
> 
> What does it return now? Does it return null, or some other window than you
> would expect? What is context.stoppedGlobal used exactly in fB?
frame.executionContext.globalObject used to return the content window (could be top level window or an inner iframe). Now it returns chrome window -> firebugFrame.xul, which represents the Firebug UI. This is the chrome window/space where all Firebug JS code and UI lives.

Firebug is using the globalObject to get the content window and see if reported error (passed into onError JSD hook) happened in the associated tab (either in the top level content doc or in an inner iframe)

Here is the related code:
https://github.com/firebug/firebug/blob/firebug1.11/extension/content/firebug/js/debugger.js#L1469

see the comment:
// Check if the eventOrigin (window) comes from this context.

Here is a test case for the issue:
https://getfirebug.com/tests/head/console/breakOnError/breakOnError.html

context.stoppedGlobal is used for dynamic JS evaluation -> eval()
Evaluation usually happens in the current top level window or in an iframe if the user changed the context using cd() command (on the command line)

But in case, the debugger is halted, evaluation prefers the current global i.e. context.stoppedGlobal.

globalObject on jsdIFilter is fine, Firebug doesn't use it.

> Unfortunately I don't think there is a way to get the same thing from
> executionContext as before right now, and I don't see an easy way to fix
> that without backing this patch out, which would be quite unfortunate. But
> I'm very curious what is this piece of information used in Fb on the first
> place, to understand the whole problem space. In particular this property
> should return the same object in most cases as before... So could you
> explain me some more about the issue you're facing?
So, depends on why the change happened.

We are hardworking on JSD2 adoption and it looks like Firefox 24 could have all API ready (depends on Bug 637572), but for time being Firebug needs JSD1 support.

Honza

Comment 16

[Gabor Krizsanits (INACTIVE)]

(In reply to Jan Honza Odvarko from comment #15)
> We are hardworking on JSD2 adoption and it looks like Firefox 24 could have
> all API ready (depends on Bug 637572), but for time being Firebug needs JSD1
> support.

Thanks a lot for the very detailed description, I understand you need JSD for the time being.

> frame.executionContext.globalObject used to return the content window (could
> be top level window or an inner iframe). Now it returns chrome window ->
> firebugFrame.xul, which represents the Firebug UI. This is the chrome
> window/space where all Firebug JS code and UI lives.

This explains a lot... :( Bobby: do you have any good idea, or are we going to back this out? I assume there is no way to get the 'previous' compartment of a JSContext. If we could cache JS_GetGlobalForScopeChain() at the right moment and store in on jsdContext then use that cached value in jsdContext::GetGlobalObject that could work maybe...

Comment 17

[Bobby Holley (:bholley)]

I filed bug 877235. Let's continue the discussion there.