Bob Owen (:bobowen) Assignee
	Description • 10 years ago

Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.

	Bob Owen (:bobowen) Assignee
	Comment 1 • 10 years ago

CreateBlobBuffer, does a WrapNative, which I think we established a while ago only requires the context for compartment tracking.
I believe the others calls could only report OOM errors, so I think we can get away with just AutoJSAPI.

	Bob Owen (:bobowen) Assignee
	Comment 2 • 10 years ago

I've convinced myself we can just get rid of this.
The context should already be pushed, so AutoPushJSContext will do nothing anyway.

Back in the very first emails about this stuff, bz requested that we add an assertion that |cx| is stack top.
In the same emails, there was also a thought that we should enter a compartment instead here, but as far as I can see, for the JS::CurrentGlobalOrNull to work we would already have to be in the global's compartment.
I don't see what other compartment we would need to enter.
Also, nothing much happens between here and a couple of calls down though WrapNativetoJSVal, where NativeInterface2JSObject grabs the current context and enters the scope's compartment anyway.

	Bob Owen (:bobowen) Assignee
	Comment 3 • 10 years ago

I'm thinking that compiling and executing regex, doesn't count as "running script", if that's wrong then clearly we need an AutoEntryScript here.

There's some error reporting going on in both of the calls.
The only thing I'm not sure about, is whether JS_ClearPendingException is always called when we have reported errors and, to be honest, I'm not sure that if it is, whether that means we could just get away with an AutoJSAPI.

	Bob Owen (:bobowen) Assignee
	Comment 4 • 10 years ago

This is almost identical to the change in Part 1.

Forgot to mention in the comment for that one, that I changed to the nsIGlobalObject interface as I'm assuming we'll be wanting to get rid of nsIScriptGlobalObject at some point.

	Bob Owen (:bobowen) Assignee
	Comment 5 • 10 years ago

js::StartOffThreadParseScript (called by JS::CompileOffThread) calls JS_NewGlobalObject with the parameter to fire the debugger hooks, so I think we need an AutoEntryScript here.
An alternative would be to put an AutoEntryScript in JS_NewGlobalObject, but as that's already in JSAPI would we want to do that?

As an aside, while looking around this, I was looking at the usages of JS_FireOnNewGlobalObject.
I'm probably missing some magic here, but ...
In JSRuntimeWrapper - Init() we seem to create a new JSContext and call JS_FireOnNewGlobalObject (and other things) without it being pushed.

Again in ThreadLocalJSRuntime - Init() we create a new JSContext without pushing and call JS_NewGlobalObject with the parameter set to call JS_FireOnNewGlobalObject.

There are some other examples, but they're in tests, so I'm not sure if they matter.

	Bob Owen (:bobowen) Assignee
	Comment 6 • 10 years ago

The last two statements would appear to be misinformed. :-)

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 7 • 10 years ago

Comment on attachment 8427590 [details] [diff] [review]
Part 1: Replace AutoPushJSContext in WebSocket::CreateAndDispatchMessageEvent. v1

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

::: content/base/src/WebSocket.cpp
@@ +866,5 @@
>    if (NS_FAILED(rv))
>      return NS_OK;
>  
> +  nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(GetOwner());
> +  if (NS_WARN_IF(!globalObject)) {

DOM uses the ENSURE macros.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 8 • 10 years ago

Comment on attachment 8427611 [details] [diff] [review]
Part 3: Replace AutoPushJSContext in nsContentUtils::IsPatternMatching v1

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

::: content/base/src/nsContentUtils.cpp
@@ +6114,5 @@
>    NS_ASSERTION(aDocument, "aDocument should be a valid pointer (not null)");
>    nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(aDocument->GetWindow());
>    NS_ENSURE_TRUE(sgo, true);
>  
> +  AutoJSAPIWithErrorsReportedToWindow jsapi(sgo->GetContext());

That's actually an interesting question... Should we report errors?

	Bob Owen (:bobowen) Assignee
	Comment 9 • 10 years ago

I think JS_DefineElement can cause other hooks to be fired, so I think we need AutoEntryScript.
I believe ArchiveReader is unique to Gecko at the moment.

try push with these patches:
https://tbpl.mozilla.org/?tree=Try&rev=601ecc7b43df

	Bob Owen (:bobowen) Assignee
	Comment 10 • 10 years ago

(In reply to Bob Owen (:bobowen) from comment #5)

> As an aside, while looking around this, I was looking at the usages of
> JS_FireOnNewGlobalObject.
> I'm probably missing some magic here, but ...
> In JSRuntimeWrapper - Init() we seem to create a new JSContext and call
> JS_FireOnNewGlobalObject (and other things) without it being pushed.
> 
> Again in ThreadLocalJSRuntime - Init() we create a new JSContext without
> pushing and call JS_NewGlobalObject with the parameter set to call
> JS_FireOnNewGlobalObject.
> 
> There are some other examples, but they're in tests, so I'm not sure if they
> matter.

Ah, I think this is probably because it appears we only push when on the main thread and neither of these cases will be, not so sure about the tests though.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 10 years ago

> I think JS_DefineElement can cause other hooks to be fired

Not any more so than JS_NewObject, I expect: it might trigger debugger hooks, but it won't run any web page script.  At least for this case, when we know we have a built-in Array object.

	Bobby Holley (:bholley)
	Comment 12 • 10 years ago

Comment on attachment 8427590 [details] [diff] [review]
Part 1: Replace AutoPushJSContext in WebSocket::CreateAndDispatchMessageEvent. v1

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

::: content/base/src/WebSocket.cpp
@@ +866,5 @@
>    if (NS_FAILED(rv))
>      return NS_OK;
>  
> +  nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(GetOwner());
> +  if (NS_WARN_IF(!globalObject)) {

The situation here is kind of in limbo - see bug 957201 comment 15. :-(

	Bobby Holley (:bholley)
	Comment 13 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #11)
> > I think JS_DefineElement can cause other hooks to be fired
> 
> Not any more so than JS_NewObject, I expect: it might trigger debugger
> hooks, but it won't run any web page script.  At least for this case, when
> we know we have a built-in Array object.

Yeah, I think we need to make the JS engine create an AutoEntryScript somehow when it decides to fire a debugger hook. See also the discussion in bug 971673.

	Bobby Holley (:bholley)
	Comment 14 • 10 years ago

Comment on attachment 8427611 [details] [diff] [review]
Part 3: Replace AutoPushJSContext in nsContentUtils::IsPatternMatching v1

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

Yeah, the JS_ClearPendingException seems pretty clearly designed to squelch errors. Let's just use an AutoJSAPI here.

r=me with that.

	Bobby Holley (:bholley)
	Comment 15 • 10 years ago

Comment on attachment 8427658 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in nsScriptLoader::AttemptAsyncScriptParse v1

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

(In reply to Bob Owen (:bobowen) from comment #5)
> js::StartOffThreadParseScript (called by JS::CompileOffThread) calls
> JS_NewGlobalObject with the parameter to fire the debugger hooks, so I think
> we need an AutoEntryScript here.
> An alternative would be to put an AutoEntryScript in JS_NewGlobalObject, but
> as that's already in JSAPI would we want to do that?

Yeah, I don't think we should be creating an AutoEntryScript every we do something to JSAPI that might trigger a debugger hook. It would explode the number of AutoEntyScript instances in Gecko.

> As an aside, while looking around this, I was looking at the usages of
> JS_FireOnNewGlobalObject.
> I'm probably missing some magic here, but ...
> In JSRuntimeWrapper - Init() we seem to create a new JSContext and call
> JS_FireOnNewGlobalObject (and other things) without it being pushed.
> 
> Again in ThreadLocalJSRuntime - Init() we create a new JSContext without
> pushing and call JS_NewGlobalObject with the parameter set to call
> JS_FireOnNewGlobalObject.

Yikes.

So yeah, we need to solve the debugger hook problem. In the mean time, I think this should just be an AUtoJSAPI.

	Bobby Holley (:bholley)
	Comment 16 • 10 years ago

Comment on attachment 8427669 [details] [diff] [review]
Part 7: Replace AutoPushJSContext in ArchiveRequest::ReaderReady v1

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

Per above, I don't think the embedding should have to worry about JS_DefineElement invoking script.

	Bob Owen (:bobowen) Assignee
	Comment 17 • 10 years ago

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

Thanks for all the reviews (and comments from others).

> > +  nsCOMPtr<nsIGlobalObject> globalObject = do_QueryInterface(GetOwner());
> > +  if (NS_WARN_IF(!globalObject)) {
> 
> The situation here is kind of in limbo - see bug 957201 comment 15. :-(

I gathered from Ms2ger that there were things afoot over this.
So, do you want me to change these to ENSURE macros?

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

> Yeah, I don't think we should be creating an AutoEntryScript every we do
> something to JSAPI that might trigger a debugger hook. It would explode the
> number of AutoEntyScript instances in Gecko.
> 
> > As an aside, while looking around this, I was looking at the usages of
> > JS_FireOnNewGlobalObject.
> > I'm probably missing some magic here, but ...
> > In JSRuntimeWrapper - Init() we seem to create a new JSContext and call
> > JS_FireOnNewGlobalObject (and other things) without it being pushed.
> > 
> > Again in ThreadLocalJSRuntime - Init() we create a new JSContext without
> > pushing and call JS_NewGlobalObject with the parameter set to call
> > JS_FireOnNewGlobalObject.
> 
> Yikes.
> 
> So yeah, we need to solve the debugger hook problem. In the mean time, I
> think this should just be an AUtoJSAPI.

Wouldn't we need an AutoJSAPIWithErrorsReportedToWindow for the CompileOffThread?

Over the other bits, I take it I was wrong in comment 10 where I thought this might be OK as we are off main thread?

	Bobby Holley (:bholley)
	Comment 18 • 10 years ago

(In reply to Bob Owen (:bobowen) from comment #17)
> I gathered from Ms2ger that there were things afoot over this.
> So, do you want me to change these to ENSURE macros?

I don't have an opinion. I'll bring it up with jst next time I talk to him.

> 
> (In reply to Bobby Holley (:bholley) from comment #15)
> 
> > Yeah, I don't think we should be creating an AutoEntryScript every we do
> > something to JSAPI that might trigger a debugger hook. It would explode the
> > number of AutoEntyScript instances in Gecko.
> > 
> > > As an aside, while looking around this, I was looking at the usages of
> > > JS_FireOnNewGlobalObject.
> > > I'm probably missing some magic here, but ...
> > > In JSRuntimeWrapper - Init() we seem to create a new JSContext and call
> > > JS_FireOnNewGlobalObject (and other things) without it being pushed.
> > > 
> > > Again in ThreadLocalJSRuntime - Init() we create a new JSContext without
> > > pushing and call JS_NewGlobalObject with the parameter set to call
> > > JS_FireOnNewGlobalObject.
> > 
> > Yikes.
> > 
> > So yeah, we need to solve the debugger hook problem. In the mean time, I
> > think this should just be an AUtoJSAPI.
> 
> Wouldn't we need an AutoJSAPIWithErrorsReportedToWindow for the
> CompileOffThread?

Yeah, I guess so.
 
> Over the other bits, I take it I was wrong in comment 10 where I thought
> this might be OK as we are off main thread?

Oh, right. For the stuff that happens internally in the JS-engine OMT, we should be fine, I think.

	Bob Owen (:bobowen) Assignee
	Comment 19 • 10 years ago

r=bholley from comment 14.

Changed to plain AutoJSAPI as we don't want to report errors here.

	Bob Owen (:bobowen) Assignee
	Comment 20 • 10 years ago

r=bholley from comment 14.

Changed to plain AutoJSAPI as we don't want to report errors here.
That'll teach me to upload the patch before even compiling.
I've also changed to nsIGlobalObject to get rid of another instance of nsIScriptGlobalObject.

	Bob Owen (:bobowen) Assignee
	Comment 21 • 10 years ago

Changed this from AutoEntryScript to AutoJSAPIWithErrorsReportedToWindow as discussed.

	Bob Owen (:bobowen) Assignee
	Comment 22 • 10 years ago

If we're going to deal with the debugger hooks separately then I don't think there's any significant error reporting here, so we can use AutoJSAPI.

Given the we can get away with not having the nsIScriptContext, I've used code that is effectively the same as is used in GetContextForEventHandlers to get the global object instead.

Here's a limited try push with all these changes.
https://tbpl.mozilla.org/?tree=Try&rev=9968f97f381c

I'll do a more extensive one (mainly because of the WrapNative change) once I'm ready to land.
Also, once I know what we want to do over NS_WARN_IF* vs NS_ENSURE*.

	Bobby Holley (:bholley)
	Comment 23 • 10 years ago

Comment on attachment 8429341 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in nsScriptLoader::AttemptAsyncScriptParse v2

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

::: content/base/src/nsScriptLoader.cpp
@@ +889,5 @@
>  
>    nsCOMPtr<nsIScriptContext> context = globalObject->GetScriptContext();
>    if (!context) {
>      return NS_ERROR_FAILURE;
>    }

Can this go away now?

	Bobby Holley (:bholley)
	Comment 24 • 10 years ago

(In reply to Bob Owen (:bobowen) from comment #22)
> Also, once I know what we want to do over NS_WARN_IF* vs NS_ENSURE*.

Please don't block yourself on this. Feel free to do either until we get it sorted out.

	Bob Owen (:bobowen) Assignee
	Comment 25 • 10 years ago

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

> ::: content/base/src/nsScriptLoader.cpp
> @@ +889,5 @@
> >  
> >    nsCOMPtr<nsIScriptContext> context = globalObject->GetScriptContext();
> >    if (!context) {
> >      return NS_ERROR_FAILURE;
> >    }
> 
> Can this go away now?

Er, do you not think that GetScriptContext could return null here then?
Otherwise we would crash constructing the AutoJSAPIWithErrorsReportedToWindow.

	Bobby Holley (:bholley)
	Comment 26 • 10 years ago

(In reply to Bob Owen (:bobowen) from comment #25)
> (In reply to Bobby Holley (:bholley) from comment #23)
> 
> > ::: content/base/src/nsScriptLoader.cpp
> > @@ +889,5 @@
> > >  
> > >    nsCOMPtr<nsIScriptContext> context = globalObject->GetScriptContext();
> > >    if (!context) {
> > >      return NS_ERROR_FAILURE;
> > >    }
> > 
> > Can this go away now?
> 
> Er, do you not think that GetScriptContext could return null here then?
> Otherwise we would crash constructing the
> AutoJSAPIWithErrorsReportedToWindow.

Ah, good point. Let's leave it for now.

	Bob Owen (:bobowen) Assignee
	Comment 27 • 10 years ago

Fairly full try push because of the WrapNative change:
https://tbpl.mozilla.org/?tree=Try&rev=3374afe30727

Landing in Part number order.
Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/26e10914de94
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7dc1566e6f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/50dfea2bc887
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a058999ec02
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6515ac1557
https://hg.mozilla.org/integration/mozilla-inbound/rev/1821dda98862
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf657095e2d

	Tim Taubert [:ttaubert] (inactive)
	Comment 29 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/26e10914de94
https://hg.mozilla.org/mozilla-central/rev/d7dc1566e6f8
https://hg.mozilla.org/mozilla-central/rev/50dfea2bc887
https://hg.mozilla.org/mozilla-central/rev/3a058999ec02
https://hg.mozilla.org/mozilla-central/rev/6d6515ac1557
https://hg.mozilla.org/mozilla-central/rev/1821dda98862
https://hg.mozilla.org/mozilla-central/rev/cbf657095e2d