868130 - Fold JSAutoRequest into nsCxPusher

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Now that we force everyone to push a cx before using it, and all pushes go through nsCxPusher, we should be able to replace all the random JSAutoRequests littered throughout gecko with a single JSAutoRequest inside nsCxPusher.

Can anyone think of a reason why this would be a problem?

Comment 1

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

Maybe performance? nsCxPusher isn't a cheap thing to use, but perhaps JSAutoRequest isn't either.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #1)
> Maybe performance? nsCxPusher isn't a cheap thing to use, but perhaps
> JSAutoRequest isn't either.

JSAutoRequest is quite cheap compared to nsCxPusher.

Comment 3

[Gabor Krizsanits (INACTIVE)]

When do we want to do a request on a cx that is not on the stack? Do we have a use case for that? If so, shouldn't we use there a different API/autoclass that features the fact that we do something tricky?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> When do we want to do a request on a cx that is not on the stack? Do we have
> a use case for that? If so, shouldn't we use there a different API/autoclass
> that features the fact that we do something tricky?

I think we more or less should never have a request for a cx that isn't on the stack.

I was talking with Luke about this on IRC. He said that we should actually just use a JSAutoCompartment to explicitly enter the compartment of the default object (rather than just using the default compartment implicitly), at which point we should no longer need requests at all.

Comment 5

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #4)
From IRC: I tried to remove requests altogether last year (see old WIP patch in bug 722345) but got bogged down in the ton of JSAutoRequests scattered about and the problem of how to find the right compartment to enter for each one.  It sounds like the situation is a lot better now so ride with the wind bholley!

Comment 6

[Bobby Holley (:bholley)]

So, a good path on this bug might be:

1 - Embed both a JSAutoRequest and a JSAutoCompartment in nsCxPusher.
2 - Remove all the other instances of JSAutoRequest in gecko
3 - Make sure things run without asserting.
4 - Remove the request API and the JSAutoRequest from nsCxPusher.

Luke, does that sound like a plan?

Comment 7

[Luke Wagner [:luke]]

A very good plan.

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Bobby Holley (:bholley)]

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

Comment 13

[Bobby Holley (:bholley)]

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

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Bobby Holley (:bholley)]

wohoo, green! Uploading patches and flagging for review.

Gabor, feel free to cancel/bounce review on any of these that you don't feel comfortable with.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 1 - Enter a request in OnJSContextNew. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make sure mContext is stack-top in nsJSContext::CompileScript. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make sure the new cx is stack-top in nsFrameMessageManager. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 4 - Use an AutoSafeJSContext in hueyfix. v1

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 5 - Make sure to push a JSContext in TabParent::ReceiveMessage. v1

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 6 - Use an AutoSafeJSContext in nsObjectLoadingContent::TeardownProtoChain. v1

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 7 - Make sure mContext is stack-top in the various places where nsJSEnvironment uses it. v1

I also replaced some uses of mJSContext with AutoJSContext upon auditing that
they're only ever called by other nsJSContext functions that just pushed. This
makes the code much easier to audit because we know that we're just inheriting
the caller's cx (which is stack-top) rather than injecting a potentially-
unrelated cx.

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 8 - Make sure to push the context in the ObjectWrapperChild constructor. v2

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 9 - Separate nsCxPusher an friends into their own file. v1

We want to put a JSAutoRequest into nsCxPusher, but that would involve
including jsapi.h in nsContentUtils.h, which we'd probably rather avoid doing.
Let's just bite the bullet and do this.

Comment 25

[Bobby Holley (:bholley)]

Attachment: Part 10 - Include nsCxPusher.h everywhere we need it, and stop including it from nsContentUtils.h. v1

Comment 26

[Bobby Holley (:bholley)]

Attachment: Part 11 - Move nsCxPusher into XPConnect. v1

The JSContext stack is an XPConnect construction. In particular, there are
situations where we want to manipulate it outside the lifetime of nsContentUtils
but within the lifetime of the stack itself. In order to do this cleanly, it's
helpful to use private XPConnect APIs. So the first step here is to move this into
js/src/xpconnect, so that we can take advantage of the stuff in xpcprivate.h.

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 12 - Remove the dependencies of the nsCxPusher machinery on nsContentUtils, use nsCxPusher in xpcshell, and privatize APIs. v1

No more xpc::danger, for now. ;-)

Comment 28

[Bobby Holley (:bholley)]

Attachment: Part 13 - Add an AutoRequest inside nsCxPusher. v1

Comment 29

[Bobby Holley (:bholley)]

Attachment: Part 14 - Remove the lion's share of JSAutoRequests in gecko. v2

There are still a handful that either are used with other runtimes, or that
happen very early/late in cx the lifetime of various things where it doesn't
necessarily make sense to have a cx on the stack. This should definitely ensure
that we're not doing double-duty with the nsCxPusher change, though.

Comment 30

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #16)
> Created attachment 750889 [details] [diff] [review]
> Part 1 - Enter a request in OnJSContextNew. v1

There is already a JSAutoRequest a few lines below yours in the if branch. What does it do now? And why is it important that it goes out of scope before DefineStaticJSVals anyway (see comment there)? I guess it should just go with the comment right?

Comment 31

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 750892 [details] [diff] [review]
Part 2 - Make sure mContext is stack-top in nsJSContext::CompileScript. v1

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

Nit: Any reason you didn't replace mContect with cx here?
XPCAutoRequest ar(mContext);

Not that it should matter much...

Comment 32

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 750897 [details] [diff] [review]
Part 7 - Make sure mContext is stack-top in the various places where nsJSEnvironment uses it. v1

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

Thanks for the helpful description, it took me a while to figure it out what's going on until I noticed it :)

Comment 33

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 750903 [details] [diff] [review]
Part 12 - Remove the dependencies of the nsCxPusher machinery on nsContentUtils, use nsCxPusher in xpcshell, and privatize APIs. v1

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

I'm so glad to see that namespace go ;)

Comment 34

[Bobby Holley (:bholley)]

Attachment: Part 1 - Enter a request for all of OnJSContextNew. v2

Hm, good catch gabor, this is indeed somewhat suspicious. But I'm also not
convinced it's still relevant, especially because the call before it to
JS_SetGCParameterForThread is now a no-op. Peter, do you remember what this
was about? Is this change ok?

Comment 35

[Masatoshi Kimura [:emk]]

Comment on attachment 750900 [details] [diff] [review]
Part 9 - Separate nsCxPusher an friends into their own file. v1

::: content/base/public/nsCxPusher.h
>+#ifndef nsCxPusher_h___
>+#define nsCxPusher_h___

Please do not add a new symbol containing consecutive underscores.

Comment 36

[Bobby Holley (:bholley)]

(In reply to Masatoshi Kimura [:emk] from comment #35)
> Comment on attachment 750900 [details] [diff] [review]
> Part 9 - Separate nsCxPusher an friends into their own file. v1
> 
> ::: content/base/public/nsCxPusher.h
> >+#ifndef nsCxPusher_h___
> >+#define nsCxPusher_h___
> 
> Please do not add a new symbol containing consecutive underscores.

What is the preferred style? If there's some consensus on it, please add it to the style guide.

Comment 37

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

(In reply to Bobby Holley (:bholley) from comment #36)
> (In reply to Masatoshi Kimura [:emk] from comment #35)
> > Comment on attachment 750900 [details] [diff] [review]
> > Part 9 - Separate nsCxPusher an friends into their own file. v1
> > 
> > ::: content/base/public/nsCxPusher.h
> > >+#ifndef nsCxPusher_h___
> > >+#define nsCxPusher_h___
> > 
> > Please do not add a new symbol containing consecutive underscores.
> 
> What is the preferred style? If there's some consensus on it, please add it
> to the style guide.

The way you include it with punctuation replaced by underscores:

  nsCxPusher_h
  mozilla_dom_Foo_h

Comment 38

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #29)
> Created attachment 750906 [details] [diff] [review]
> Part 14 - Remove the lion's share of JSAutoRequests in gecko. v2
> 

These are the ones where I think we need a cx push:

nsXPConnect::CheckForDebugMode
BluetoothService::Notify
BroadcastSystemMessage

And these are the ones where I'm not sure... I might be missing something obvious, but I don't see the reason why we shouldn't

TabChild::DispatchMessageManagerMessage
NPStringIdentifierIsPermanent
PluginScriptableObjectParent::AnswerEnumerate

The rest looks good, so if you can fix/explain me these I'll r+ it.

Comment 39

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #38)
> (In reply to Bobby Holley (:bholley) from comment #29)
> > Created attachment 750906 [details] [diff] [review]
> > Part 14 - Remove the lion's share of JSAutoRequests in gecko. v2
> > 
> 
> These are the ones where I think we need a cx push:
> 
> nsXPConnect::CheckForDebugMode

This gets called by the pushing machinery, so we can't.

> BluetoothService::Notify
> BroadcastSystemMessage

I will add these.

> 
> And these are the ones where I'm not sure... I might be missing something
> obvious, but I don't see the reason why we shouldn't
> 
> TabChild::DispatchMessageManagerMessage

Hm, you're right. We probably need an AutoPushJSContext for mCx there.

> NPStringIdentifierIsPermanent

We should just be using an AutoJSContext there. The cx is only used in that call to access the runtime.

> PluginScriptableObjectParent::AnswerEnumerate

Should probably use an AutoPushJSContext there.

Great catches! I'll throw those into a patch and flag you for review.

Comment 40

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #39)
> > TabChild::DispatchMessageManagerMessage
> 
> Hm, you're right. We probably need an AutoPushJSContext for mCx there.

Actually, TabChild does a lot of funny stuff with the JSContext. For the usage right here, I think we should just use the SafeJSContext.

Comment 41

[Bobby Holley (:bholley)]

Attachment: Part 13.5 - Fix sketchy cx consumers identified by gabor. v1

Comment 42

[Bobby Holley (:bholley)]

Attachment: Part 14 - Remove the lion's share of JSAutoRequests in gecko. v4

Comment 43

[Andrew McCreight [:mccr8]]

Is there some kind of documentation about how this is supposed to work after these patches?

Comment 44

[Bobby Holley (:bholley)]

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

Comment 45

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #43)
> Is there some kind of documentation about how this is supposed to work after
> these patches?

What is "this"?

In general, this stuff is all changing rapidly, and the invariants are in flux with the various patches I have in my repos. So for the mean time, the best documentation is probably "ask bholley". :-)

Comment 46

[Masatoshi Kimura [:emk]]

Comment on attachment 750903 [details] [diff] [review]
Part 12 - Remove the dependencies of the nsCxPusher machinery on nsContentUtils, use nsCxPusher in xpcshell, and privatize APIs. v1

Is bug 864363 still needed?

Comment 47

[Bobby Holley (:bholley)]

(In reply to Masatoshi Kimura [:emk] from comment #46)
> Comment on attachment 750903 [details] [diff] [review]
> Part 12 - Remove the dependencies of the nsCxPusher machinery on
> nsContentUtils, use nsCxPusher in xpcshell, and privatize APIs. v1
> 
> Is bug 864363 still needed?

Yeah, I think so. The issue in that bug was that Layout (which includes both XPConnect and nsContentUtils) hadn't been spun up yet. nsCxPusher still depends on XPConnect, so the same issue applies.

Note that Trevor is making XPConnect initialization non-lazy in bug 873622, so it'll hopefully start up predictably in nsLayoutStatics::Init.

Comment 48

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 752442 [details] [diff] [review]
Part 13.5 - Fix sketchy cx consumers identified by gabor. v1

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

+    AutoSafeJSContext cx;
+    JSAutoRequest ar(cx);

You forgot JSAutoRequest in a few of places. Since AutoSafeJSContext always pushes, those could/should go.

Comment 49

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 752444 [details] [diff] [review]
Part 14 - Remove the lion's share of JSAutoRequests in gecko. v4

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

(In reply to Bobby Holley (:bholley) from comment #39)
> > nsXPConnect::CheckForDebugMode
> 
> This gets called by the pushing machinery, so we can't.

I guess I got a bit mechanical when I got there, didn't realize it. This one still slightly worries me. I don't see any good reason here why we can just remove that JSAutoRequest. This should at very least go into a separate patch and some comment would be nice too in case it causes something weird (also since this is one of the few cases where we call a function with a JSContext* argument that is not on the stack). I have no idea what else we could do here. r+ the rest, but this one might be a good idea to be checked by someone other than me, unless you know exactly why it's safe to remove that request.

Comment 50

[Peter Van der Beken [:peterv]]

Comment on attachment 751710 [details] [diff] [review]
Part 1 - Enter a request for all of OnJSContextNew. v2

I don't know why we did it that way. DefineStaticJSVals and InternStaticDictionaryJSVals both have a JSAutoRequest which isn't really needed anymore, but might be safer to keep them.

Comment 51

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #49)
> This should at very least go into a separate patch

Bug 872135 addressed the problem.

Comment 52

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #48)
> +    AutoSafeJSContext cx;
> +    JSAutoRequest ar(cx);
> 
> You forgot JSAutoRequest in a few of places. Since AutoSafeJSContext always
> pushes, those could/should go.

Yeah, they're removed in the next patch. This patch goes underneath patch 14.

Comment 53

[Bobby Holley (:bholley)]

This seems to bitrot every 10 minutes. Pushing to inbound and crossing my fingers it's still green like it was in comment 44.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6869a88c9461

Comment 54

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a5959b273b65
https://hg.mozilla.org/mozilla-central/rev/a0e74ae591bd
https://hg.mozilla.org/mozilla-central/rev/f684730b3678
https://hg.mozilla.org/mozilla-central/rev/6d4aaf1758b3
https://hg.mozilla.org/mozilla-central/rev/a90a8cda1805
https://hg.mozilla.org/mozilla-central/rev/fe05e84ac7a0
https://hg.mozilla.org/mozilla-central/rev/71a10dfafaad
https://hg.mozilla.org/mozilla-central/rev/fa8a26e562b7
https://hg.mozilla.org/mozilla-central/rev/06f68e1253c3
https://hg.mozilla.org/mozilla-central/rev/4d80fdd4a55e
https://hg.mozilla.org/mozilla-central/rev/a8c7766cd507
https://hg.mozilla.org/mozilla-central/rev/21786120e0ea
https://hg.mozilla.org/mozilla-central/rev/3e1d6dd0b814
https://hg.mozilla.org/mozilla-central/rev/91c863ec3051
https://hg.mozilla.org/mozilla-central/rev/bd355364bc33
https://hg.mozilla.org/mozilla-central/rev/b96fa35bafe8

Comment 55

[Gabor Krizsanits (INACTIVE)]

So the assumption that if you call a function and pass a JSContext* cx argument, you always make sure that cx is pushed to the stack, seems to be broken all over the place. In Bug 875251 it seems it's a GetContextForEventHandlers + sc->GetNativeContext() pair in BluetoothAdapter that is used un-pushed and causes a crash, but there are many other places as well... Just a quick search on mxr "JScontext* cx =" shows quite a lot of suspicious cases :(