Closed
Bug 1045646
Opened 10 years ago
Closed 10 years ago
Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 12
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(5 files, 3 obsolete files)
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc. See bug 951991 for details.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5a914690a5ee
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
\o/ (Did this in a separate patch in case we need to back something out.)
Attachment #8465303 -
Flags: review?(bobbyholley)
Updated•10 years ago
|
Attachment #8465262 -
Flags: review?(bobbyholley) → review+
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8465301 -
Flags: review?(bobbyholley) → review+
Comment 9•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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
Comment 15•10 years ago
|
||
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
Keywords: checkin-needed
Comment 16•10 years ago
|
||
> 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•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•