1045646 - Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 12

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bob Owen (:bobowen)]

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

Comment 1

[Bob Owen (:bobowen)]

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

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace AutoPushJSContext in nsXBLProtoImplAnonymousMethod::Execute v1

Try push is looking pretty good.

Seems like a straight forward case for an AutoEntryScript.

My only concern is that I've switched |global| to the inner window for the AutoEntryScript.
As far as I can tell GetScopeForXBLExecution cares about the compartment, so I think the inner window will work just as well.

Over the nsCxPusher, looks like the ScriptEvaluated call was removed from AutoCxPusher a while ago and would have been called even without this.
Anyway, it appears this just ends up pushing the same context again, so I think this can go.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace AutoPushJSContext in nsCryptoRunnable::Run v1

From what I gleaned from bug 1025230 comment 15, holding a pointer to the nsIGlobalObject appears to be the best approach here.
The only nagging doubt I have is whether |aContext| can ever be for a different global than our parent?

I believe the death grip is only required because we're holding a JSContext* and I think that we can use the JS global instead of m_scope (in fact I think they would effectively be the same).

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace AutoPushJSContext in nsHTTPIndex::OnFTPControlLog v1

I think this is Gecko specific.

My main concern is whether |mRequestor| can provide an nsIGlobalObject.
I was half expecting this to fail, but I'm not sure if it is tested.

I'm not entirely sure how to trigger this code.
I thought it might be to do with a directory listing on an FTP site, but it doesn't seem to hit any breakpoints.

I could put this back to an nsIScriptGlobalObject to be safe.
Or, given that I think the interface requester is a docshell, maybe use nsPIDOMWindow and then static cast to nsGlobalWindow.
I'm not all that keen on the hand waving magic that is do_GetInterface.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 4: Replace AutoPushJSContext in nsHTTPIndex::OnStartRequest v1

I assume that it is possible for this to run script.

Same comments from Part 3 apply over nsIGlobalObject.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 5: Remove AutoPushJSContext v1

\o/

(Did this in a separate patch in case we need to back something out.)

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8465281 [details] [diff] [review]
Part 2: Replace AutoPushJSContext in nsCryptoRunnable::Run v1

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

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2173,5 @@
> +  // We're going to run script via JS_EvaluateScript, so we need an
> +  // AutoEntryScript. This is Gecko specific and not on a standards track.
> +  AutoEntryScript aes(m_args->m_globalObject);
> +  JSContext* cx = aes.cx();
> +  JS::Rooted<JSObject*> scope(cx, m_args->m_globalObject->GetGlobalJSObject());

I'd just do JS::CurrentGlobalOrNull instead.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8465298 [details] [diff] [review]
Part 3: Replace AutoPushJSContext in nsHTTPIndex::OnFTPControlLog v1

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

I'm pretty sure that the only place we support getInterface of nsIScriptGlobalObject is in nsDocShell, here:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1010

So just add nsIGlobalObject to that list, and then this should work. r=me with that change.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8465303 [details] [diff] [review]
Part 5: Remove AutoPushJSContext v1

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

:-) :-) :-)

You rock Bob.

Comment 10

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace AutoPushJSContext in nsCryptoRunnable::Run v2

r=bholley - from comment 7

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

> > +  JS::Rooted<JSObject*> scope(cx, m_args->m_globalObject->GetGlobalJSObject());
> 
> I'd just do JS::CurrentGlobalOrNull instead.

Done.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace AutoPushJSContext in nsHTTPIndex::OnFTPControlLog v2

r=bholley - from comment 8

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

> I'm pretty sure that the only place we support getInterface of
> nsIScriptGlobalObject is in nsDocShell, here:
> 
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#1010
> 
> So just add nsIGlobalObject to that list, and then this should work. r=me
> with that change.

Yeah, I'm fairly certain the interface requester (can't help thinking that should be provider, but anyway) is a docshell, wasn't sure whether adding to those lists of interfaces was OK (or required).

Small try push with this change and the one to Part 2:
https://tbpl.mozilla.org/?tree=Try&rev=cb9876f3dd3b

Comment 12

[Bob Owen (:bobowen)]

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

> :-) :-) :-)
> 
> You rock Bob.

Thanks ... I've got to admit, that patch felt pretty good.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 4: Replace AutoPushJSContext in nsHTTPIndex::OnStartRequest v2

r=bholley - from https://bugzilla.mozilla.org/show_bug.cgi?id=1045646#h9

Doing a last check and just noticed a copy and paste blunder.
The old code for this was returning an NS_ERROR_FAILURE on null scriptGlobal, but I'd copied NS_OK from Part 3.
Now fixed.

Another try push:
https://tbpl.mozilla.org/?tree=Try&rev=473b0c6d856f

Comment 14

[Bob Owen (:bobowen)]

Please land in part number order.
Thanks.

Initial try push:
https://tbpl.mozilla.org/?tree=Try&rev=5a914690a5ee

Try push after addressing reviews comments:
https://tbpl.mozilla.org/?tree=Try&rev=473b0c6d856f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5f31798021
https://hg.mozilla.org/integration/mozilla-inbound/rev/3341ea7debc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/38afc229dd0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9e27a92626
https://hg.mozilla.org/integration/mozilla-inbound/rev/f55d6b51c957

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I'm pretty sure that the only place we support getInterface of nsIScriptGlobalObject is
> in nsDocShell

And more to the point, this nsIInterfaceRequestor comes from the aContainer argument of nsDirectoryViewerFactory::CreateInstance, which is totally a docshell.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a5f31798021
https://hg.mozilla.org/mozilla-central/rev/3341ea7debc4
https://hg.mozilla.org/mozilla-central/rev/38afc229dd0a
https://hg.mozilla.org/mozilla-central/rev/fa9e27a92626
https://hg.mozilla.org/mozilla-central/rev/f55d6b51c957