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

RESOLVED FIXED in mozilla34

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

Trunk
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
(Assignee)

Comment 2

4 years ago
Created attachment 8465262 [details] [diff] [review]
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.
Attachment #8465262 - Flags: review?(bobbyholley)
(Assignee)

Updated

4 years ago
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 8465281 [details] [diff] [review]
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).
Attachment #8465281 - Flags: review?(bobbyholley)
(Assignee)

Comment 4

4 years ago
Created attachment 8465298 [details] [diff] [review]
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.
Attachment #8465298 - Flags: review?(bobbyholley)
(Assignee)

Comment 5

4 years ago
Created attachment 8465301 [details] [diff] [review]
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.
Attachment #8465301 - Flags: review?(bobbyholley)
(Assignee)

Comment 6

4 years ago
Created attachment 8465303 [details] [diff] [review]
Part 5: Remove AutoPushJSContext v1

\o/

(Did this in a separate patch in case we need to back something out.)
Attachment #8465303 - Flags: review?(bobbyholley)
Attachment #8465262 - Flags: review?(bobbyholley) → review+
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.
Attachment #8465281 - Flags: review?(bobbyholley) → review+
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.
Attachment #8465298 - Flags: review?(bobbyholley) → review+
Attachment #8465301 - Flags: review?(bobbyholley) → review+
Comment on attachment 8465303 [details] [diff] [review]
Part 5: Remove AutoPushJSContext v1

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

:-) :-) :-)

You rock Bob.
Attachment #8465303 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 10

4 years ago
Created attachment 8466073 [details] [diff] [review]
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.
Attachment #8465281 - Attachment is obsolete: true
Attachment #8466073 - Flags: review+
(Assignee)

Comment 11

4 years ago
Created attachment 8466075 [details] [diff] [review]
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
Attachment #8465298 - Attachment is obsolete: true
Attachment #8466075 - Flags: review+
(Assignee)

Comment 12

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

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

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

Comment 13

4 years ago
Created attachment 8466091 [details] [diff] [review]
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
Attachment #8465301 - Attachment is obsolete: true
Attachment #8466091 - Flags: review+
(Assignee)

Comment 14

4 years ago
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
Keywords: checkin-needed
> 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.
You need to log in before you can comment on or make changes to this bug.