Fold JSAutoRequest into nsCxPusher

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla24
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments, 2 obsolete attachments)

2.33 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.14 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.32 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.15 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.07 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
10.06 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.13 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
22.58 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
65.35 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
3.12 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
14.68 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
2.41 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
2.29 KB, patch
peterv
: review+
Details | Diff | Splinter Review
7.27 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
122.43 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
Assignee

Description

6 years ago
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?
Maybe performance? nsCxPusher isn't a cheap thing to use, but perhaps JSAutoRequest isn't either.
Assignee

Comment 2

6 years ago
(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.
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?
Assignee

Comment 4

6 years ago
(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.
(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!
Assignee

Comment 6

6 years ago
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?
A very good plan.
Assignee

Updated

6 years ago
Duplicate of this bug: 584673
Assignee

Comment 15

6 years ago
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.
Assignee

Comment 16

6 years ago
Attachment #750889 - Flags: review?(gkrizsanits)
Assignee

Comment 22

6 years ago
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.
Attachment #750897 - Flags: review?(gkrizsanits)
Assignee

Comment 24

6 years ago
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.
Attachment #750900 - Flags: review?(gkrizsanits)
Assignee

Comment 26

6 years ago
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.
Attachment #750902 - Flags: review?(gkrizsanits)
Assignee

Comment 29

6 years ago
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.
Attachment #750906 - Flags: review?(gkrizsanits)
(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 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...
Attachment #750892 - Flags: review?(gkrizsanits) → review+
Attachment #750893 - Flags: review?(gkrizsanits) → review+
Attachment #750894 - Flags: review?(gkrizsanits) → review+
Attachment #750895 - Flags: review?(gkrizsanits) → review+
Attachment #750896 - Flags: review?(gkrizsanits) → review+
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 :)
Attachment #750897 - Flags: review?(gkrizsanits) → review+
Attachment #750898 - Flags: review?(gkrizsanits) → review+
Attachment #750900 - Flags: review?(gkrizsanits) → review+
Attachment #750901 - Flags: review?(gkrizsanits) → review+
Attachment #750902 - Flags: review?(gkrizsanits) → review+
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 ;)
Attachment #750903 - Flags: review?(gkrizsanits) → review+
Attachment #750905 - Flags: review?(gkrizsanits) → review+
Assignee

Comment 34

6 years ago
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?
Attachment #750889 - Attachment is obsolete: true
Attachment #750889 - Flags: review?(gkrizsanits)
Attachment #751710 - Flags: review?(peterv)
Blocks: 872599
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.
Assignee

Comment 36

6 years ago
(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.
(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
(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.
Assignee

Comment 39

6 years ago
(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.
Assignee

Comment 40

6 years ago
(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.
Assignee

Comment 42

6 years ago
Attachment #750906 - Attachment is obsolete: true
Attachment #750906 - Flags: review?(gkrizsanits)
Attachment #752444 - Flags: review?(gkrizsanits)
Is there some kind of documentation about how this is supposed to work after these patches?
Assignee

Comment 45

6 years ago
(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 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?
Assignee

Comment 47

6 years ago
(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 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.
Attachment #752442 - Flags: review?(gkrizsanits) → review+
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.
Attachment #752444 - Flags: review?(gkrizsanits) → review+
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.
Attachment #751710 - Flags: review?(peterv) → review+
(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.
Assignee

Comment 52

6 years ago
(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.
Assignee

Comment 53

6 years ago
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
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 :(

Updated

6 years ago
Depends on: 875790

Updated

6 years ago
Assignee: nobody → bobbyholley+bmo
Depends on: 883301
Depends on: 882897
Depends on: 886174
You need to log in before you can comment on or make changes to this bug.