997440 - Use a sane entry script for XPCWrappedJS invocation

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

In bug 988383 comment 13, Bob ran into failures when he tried to remove the cx push in nsGlobalWindow::SetNewDocument. As it turns out, we associated XPCWrappedNativeScope instances with the stack-top cx at creation time, and fall back to that for a JSContext in XPCWrappedJS invocation if there's nothing on the stack.

So when we use the SafeJSContext in SetNewDocument, that spreads to the XPCWrappedNativeScope, causing us to use the SafeJSContext to invoke XPCWrappedJS instances, which gives them no useful base URI. This causes us to fail things like docshell/test/test_bug660404.html.

I'll attach some patches to fix this all up.

Comment 1

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #0)

> So when we use the SafeJSContext in SetNewDocument, that spreads to the
> XPCWrappedNativeScope, causing us to use the SafeJSContext to invoke
> XPCWrappedJS instances, which gives them no useful base URI.

Ah, I'd looked at that code a couple of times, where it does a bare |new XPCWrappedNativeScope|.
Couldn't quite see how it was causing the uncaught exceptions though.

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=7339362561f3

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=19c964a954a1

Comment 4

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #3)
> https://tbpl.mozilla.org/?tree=Try&rev=19c964a954a1

I know these may not be the final patches, but should these usages of AutoEntryScript have comments about why we need them, linking to the spec or stating that they are Gecko specific?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #4)
> (In reply to Bobby Holley (:bholley) from comment #3)
> > https://tbpl.mozilla.org/?tree=Try&rev=19c964a954a1
> 
> I know these may not be the final patches, but should these usages of
> AutoEntryScript have comments about why we need them, linking to the spec or
> stating that they are Gecko specific?

Good point! I've added them.

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1 - Push some more cxes in jsd. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Fix an XPConnect test bug. v1

This failure is currently being silently squelched on trunk.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix some error reporting nastiness in the CPS tests. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce a special null principal to return when we're not in any compartment at all. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 5 - Use the WrappedJS's global as an entry global. v1

This basically means that baseURI will be relative to global in which the code
is defined, which seems reasonable.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 6 - Stop associating XPCWrappedNativeScopes with an XPCContext. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove cx pushing in SetNewDocument. v1

Comment 14

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=5c5aae0260cf

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8420254 [details] [diff] [review]
Part 7 - Remove cx pushing in SetNewDocument. v1

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

::: dom/base/nsGlobalWindow.cpp
@@ +2279,5 @@
>    }
>  #endif
>  
> +  AutoJSAPI jsapi;
> +  JSContext *cx = jsapi.cx();

* to the left

@@ +2341,3 @@
>    // Check if we're near the stack limit before we get anywhere near the
>    // transplanting code.
>    JS_CHECK_RECURSION(cx, return NS_ERROR_FAILURE);

This is in a bit of a weird place now. Should it be right after the declaration of cx?

Comment 16

[Bob Owen (:bobowen)]

Comment on attachment 8420248 [details] [diff] [review]
Part 1 - Push some more cxes in jsd. v1

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

::: js/jsd/jsd_stak.cpp
@@ +269,5 @@
>      JSD_LOCK_THREADSTATES(jsdc);
>  
>      if( jsd_IsValidFrameInThreadState(jsdc, jsdthreadstate, jsdframe) )
>      {
> +        AutoPushJSContext cx(jsdthreadstate->context);

Er ... aren't we trying to get rid of these?

Comment 17

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #16)
> >      if( jsd_IsValidFrameInThreadState(jsdc, jsdthreadstate, jsdframe) )
> >      {
> > +        AutoPushJSContext cx(jsdthreadstate->context);
> 
> Er ... aren't we trying to get rid of these?

Yes, but the entire jsd directory is going to be rm -f ed sometime this cycle or next, so we just want to whack the mole a little bit longer. ;-)

Comment 18

[Bobby Holley (:bholley)]

Bob, there were a few errors in the try push in comment 14. Do you have the cycles to take a quick look at them and see what's going on? I'm pretty swamped at the moment.

Comment 19

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #18)
> Bob, there were a few errors in the try push in comment 14. Do you have the
> cycles to take a quick look at them and see what's going on? I'm pretty
> swamped at the moment.

I'll take a look and see what I can do.

Comment 20

[Bob Owen (:bobowen)]

(In reply to Bob Owen (:bobowen) from comment #19)
> (In reply to Bobby Holley (:bholley) from comment #18)
> > Bob, there were a few errors in the try push in comment 14. Do you have the
> > cycles to take a quick look at them and see what's going on? I'm pretty
> > swamped at the moment.
> 
> I'll take a look and see what I can do.

I've been ruling out all the extraneous failures throughout the day while doing other things finally got chance to have a look at the real ones.

So the xpcshell test that is failing is definitely because of Part 5.
It looks like it's observing extra exceptions that it is not expecting.
Possibly something to do with directory services.

Sorry, haven't had much experience with xpcshell tests and I'm having trouble using gdb.
I get the following:

.../xpcshell: error while loading shared libraries: libxul.so: cannot open shared object file: No such file or directory

Comment 21

[Bob Owen (:bobowen)]

I've got gdb working now, it appears to be the change in nsXPCWrappedJSClass::CallMethod that is causing the problem.
I'll carry on digging while I have time.

Comment 22

[Bob Owen (:bobowen)]

I've been working on Windows today, so I've just had a look at the browser chrome failure and it looks like it is also caused by the change in nsXPCWrappedJSClass::CallMethod.

We appear to be ending up with a different JSContext* from the one we get with the old code.
Not sure if that is intentional.

Comment 23

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #22)
> We appear to be ending up with a different JSContext* from the one we get
> with the old code.
> Not sure if that is intentional.

Yeah, that's intentional. The current JSContext is very arbitrary, and that machinery needs to go away (see part 6). So the replacement is to use the JSContext we from the AutoEntryScript, which makes much more sense.

So we basically just need to understand why this is failing in the new world (presumably, because there was an error that was squelched in the old world but reported in the new world), and hammer it down appropriately (a la Part 3).

Comment 24

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #23)

> So we basically just need to understand why this is failing in the new world
> (presumably, because there was an error that was squelched in the old world
> but reported in the new world), and hammer it down appropriately (a la Part
> 3).

Right, I see.
I'll look at doing that tomorrow.

Comment 25

[Bob Owen (:bobowen)]

Attachment: Part 3a: Fix some more error reporting in crashreporter tests. v1

All of my time got consumed by others things yesterday.

This test uses a fake AddData dir to hold the test crash reports, it also registers its own nsIDirectoryServiceProvider to provide the fake path.

This fake provider was also getting requests for plugin.scan.Acrobat and plugin.scan.Quicktime, and as a result threw exceptions.
This used to get lost, but after patch Part 5 doesn't.

I think I could have just added a SimpleTest.ignoreAllUncaughtExceptions to the test, but this seems more targeted.
Also, I decided to return null rather than delegate to the real provider as it seemed closest to the existing behaviour.

Interestingly, as I discovered while trying to debug this, if you delayed the start of the test you didn't get the errors, presumably because the getFile calls had already been made.
If you started the tests soon enough the errors got reported, but always in the same place, as if they were queued up waiting to be released.

Try push with this patch:
https://tbpl.mozilla.org/?tree=Try&rev=9a1665905367

I'll switch to Linux now as I'm hoping I have a fix for that one as well.

Comment 26

[Bob Owen (:bobowen)]

Attachment: Part 3b: Yet another error reporting fix, this time in devtools server tests. v1

This is a very similar change to Part 3, although I'm not entirely sure what is causing the new errors that have surfaced.
Do you think they should be followed up in another bug?

Here's a try push for the xpcshell tests with this patch:
https://tbpl.mozilla.org/?tree=Try&rev=67ade2710bd4

Oh, and patch Part 4 doesn't apply cleanly at the moment because of the rename of GetSubjectPrincipal and GetObjectPrincipal to SubjectPrincipal and ObjectPrincipal.

Comment 27

[Bob Owen (:bobowen)]

Whoops, bzexport stole the bug for me.

Comment 28

[Bob Owen (:bobowen)]

Attachment: Part 5 - Use the WrappedJS's global as an entry global. v2

The build from that last try push was failing on Linux debug.
Looks like Part 5 lost the move of a using statement from the try push in comment 14.
As the patch holds the original author, I assume I can just add this.

Here's a try push just for the failing build:
https://tbpl.mozilla.org/?tree=Try&rev=9415d6c65e04

I'll upload Part 4 with the surrounding function name changes I mentioned, as well.

Comment 29

[Bob Owen (:bobowen)]

Attachment: Part 4 - Introduce a special null principal to return when we're not in any compartment at all. v2

Same as version 1 of this patch, but just with a couple of renames of functions on the surrounding context code, so it applies cleanly.

Comment 30

[Bobby Holley (:bholley)]

Awesome, thanks Bob! Looks like this is all green now, so we're just waiting on Boris' review.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8420248 [details] [diff] [review]
Part 1 - Push some more cxes in jsd. v1

>@@ -287,19 +288,18 @@ jsd_GetScopeChainForStackFrame(JSDContext* jsdc,

Is it OK to still be in the request when we do JSD_NewValue?

>@@ -311,19 +311,18 @@ jsd_GetThisForStackFrame(JSDContext* jsdc,

Likewise.

r=me modulo that

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8420249 [details] [diff] [review]
Part 2 - Fix an XPConnect test bug. v1

r==me

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8420250 [details] [diff] [review]
Part 3 - Fix some error reporting nastiness in the CPS tests. v1

r=me

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8424469 [details] [diff] [review]
Part 4 - Introduce a special null principal to return when we're not in any compartment at all. v2

r=me

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8424467 [details] [diff] [review]
Part 5 - Use the WrappedJS's global as an entry global. v2

r=me.  This is the guts of the change, right?  ;)

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8420253 [details] [diff] [review]
Part 6 - Stop associating XPCWrappedNativeScopes with an XPCContext. v1

r=me

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8420254 [details] [diff] [review]
Part 7 - Remove cx pushing in SetNewDocument. v1

r=me, I think

Comment 38

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #31)
> Is it OK to still be in the request when we do JSD_NewValue?

AFAIK, it's really never a problem to be in a request. The main reasons we have requests is so that we have some way of running GCs with the conservative stack scanner disabled (in configurations where we still use that).

Comment 39

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=3af80b2666c8

Comment 40

[Bobby Holley (:bholley)]

Green. Thanks to both Bob and Boris for helping get this over the line!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8210c3079afc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cde76de1bea6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/267663b1b22a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/93188d7e82e8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b204d8e53153
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2278430a3f23
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e55153ae2539
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f22f1c4043f9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f135d7122262

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8210c3079afc
https://hg.mozilla.org/mozilla-central/rev/cde76de1bea6
https://hg.mozilla.org/mozilla-central/rev/267663b1b22a
https://hg.mozilla.org/mozilla-central/rev/93188d7e82e8
https://hg.mozilla.org/mozilla-central/rev/b204d8e53153
https://hg.mozilla.org/mozilla-central/rev/2278430a3f23
https://hg.mozilla.org/mozilla-central/rev/e55153ae2539
https://hg.mozilla.org/mozilla-central/rev/f22f1c4043f9
https://hg.mozilla.org/mozilla-central/rev/f135d7122262