Closed
Bug 1037936
Opened 10 years ago
Closed 10 years ago
Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 11
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: bobowen, Assigned: bobowen)
References
Details
Attachments
(3 files, 5 obsolete files)
(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
|
||
Most of this (after the |if(scriptObject)|) is just indentation changes. I've gone for legacy error reporting to be safe, as JS_SetPrototype at seems like it can report them.
Attachment #8455515 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
Comment on attachment 8455515 [details] [diff] [review] Part 1: Replace nsCxPusher in nsXBLBinding::ChangeDocument v1 Review of attachment 8455515 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xbl/nsXBLBinding.cpp @@ +734,5 @@ > + // Init might fail here if we've cycle-collected the global object, since > + // the Unlink phase of cycle collection happens after JS GC finalization. > + // But in that case, we don't care about fixing the prototype chain, since > + // everything's going away immediately. > + if (jsapi.InitWithLegacyErrorReporting(aOldDocument->GetScopeObject())) { Hm, I think we actually don't want to report XBL prototype munging errors to the DOM Window - let's drop the WithLegacyErrorReporting.
Attachment #8455515 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 3•10 years ago
|
||
r=bholley - from comment 2 Changed to AutoJSAPI::Init instead of AutoJSAPI::InitWithLegacyErrorReporting
Attachment #8455515 -
Attachment is obsolete: true
Attachment #8456201 -
Flags: review+
Assignee | ||
Comment 4•10 years ago
|
||
This should be identical behaviour I think, it gets rid of one nsCxPusher and prepares for the removal of the other. I think moving all of this to the same place makes it easier to understand what is actually happening, even if we're not entirely sure why. :) Here's my reasoning ... If we go into the if statement at old line 565 the old behaviour was: * push the context * at line 816, we would have a current JS context so we'd call SubjectPrincipal * at line 890, again we'd have a current JS context and the GetDynamicScriptGlobal would get us back to aParent. If the if is false the behaviour was: * at line 816 get the SubjectPrincipal if we have a current JS context. * at line 890 use current context if not null, otherwise use parent context (giving aParent), otherwise we'd select the SafeJSContext, which would never give us an nsPIDOMWindow anyway.
Attachment #8456228 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•10 years ago
|
||
The functions we call further down only seem to care about the compartment to get the principal, so we should be fine with the SafeJSContext I think. The only other slight difference is that we'd set the referrer URI with aParent even if we couldn't get a context, but I'm not entirely sure that would ever happen anyway. Just a small set to finish off the work I did at the weekend, here's a try push: https://tbpl.mozilla.org/?tree=Try&rev=28eaf60c2625
Attachment #8456238 -
Flags: review?(bobbyholley)
Comment 6•10 years ago
|
||
Comment on attachment 8456228 [details] [diff] [review] Part 2: Move all of the JSContext craziness in nsWindowWatcher::OpenWindowInternal into one place v1 Review of attachment 8456228 [details] [diff] [review]: ----------------------------------------------------------------- This is excellent detective work. Sorry for taking so long to review it - it was hard to find an uninterrupted 45 minutes to put my nsWindowWatcher goggles on. ;-) r=bholley with comments addressed. ::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp @@ +580,5 @@ > // based on whether the docshell type is chrome or content. > > + referrerWindow = do_QueryInterface(aParent); > + callerContextGuard.Push(parentCx); > + subjectPrincipal = nsContentUtils::SubjectPrincipal(); I'm not a fan of setting subjectPrincipal in 3 places (1 of them implicit). Why is this preferable to the old version? I'd prefer that you revert this part, unless there's something I'm missing. @@ +588,5 @@ > + // Note: Only using nsContentUtils::SubjectPrincipal if we have a current > + // context doesn't necessarily make sense. > + // It's just designed to preserve old semantics. > + subjectPrincipal = nsContentUtils::SubjectPrincipal(); > + referrerWindow = do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx)); Can you switch this to BrokenGetEntryGlobal()?
Attachment #8456228 -
Flags: review?(bobbyholley) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8456238 [details] [diff] [review] Part 3: Replace nsCxPusher in nsWindowWatcher::OpenWindowInternal v1 Review of attachment 8456238 [details] [diff] [review]: ----------------------------------------------------------------- \o/ ::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp @@ -558,5 @@ > sm(do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID)); > > bool isCallerChrome = nsContentUtils::IsCallerChrome() && !openedFromRemoteTab; > > - JSContext* parentCx = GetJSContextFromWindow(aParent); I think you can remove GetJSContextFromWindow now.
Attachment #8456238 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #6) > This is excellent detective work. Sorry for taking so long to review it - it > was hard to find an uninterrupted 45 minutes to put my nsWindowWatcher > goggles on. ;-) Thanks, and no problem, I know what you mean, it took a while to work out what was happening here. :) > ::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp > @@ +580,5 @@ > > // based on whether the docshell type is chrome or content. > > > > + referrerWindow = do_QueryInterface(aParent); > > + callerContextGuard.Push(parentCx); > > + subjectPrincipal = nsContentUtils::SubjectPrincipal(); > > I'm not a fan of setting subjectPrincipal in 3 places (1 of them implicit). > Why is this preferable to the old version? I'd prefer that you revert this > part, unless there's something I'm missing. Well, it was just that in the one case we already knew that we had a current context, but I see what you mean about the implicit case. The only other thing that confused me was that where it was, was nothing to do with the code immediately below, even though it looks like it is. Anyway, it's not really relevant to this bug, so I've reverted it. > @@ +588,5 @@ > > + // Note: Only using nsContentUtils::SubjectPrincipal if we have a current > > + // context doesn't necessarily make sense. > > + // It's just designed to preserve old semantics. > > + subjectPrincipal = nsContentUtils::SubjectPrincipal(); > > + referrerWindow = do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx)); > > Can you switch this to BrokenGetEntryGlobal()? Well, with this I could refactor a bit as BrokenGetGlobalEntry does the null check. Then with the SubjectPrincipal change above it seemed to make sense to move this back down to where it's actually used. I only really moved everything to try and see what was going on, but I think this makes more sense and means that most of the patch is in the same area.
Attachment #8456228 -
Attachment is obsolete: true
Attachment #8462690 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 9•10 years ago
|
||
r=bholley - from comment 7 (In reply to Bobby Holley (:bholley) from comment #7) > > - JSContext* parentCx = GetJSContextFromWindow(aParent); > > I think you can remove GetJSContextFromWindow now. So I can, thanks. It meant I could get rid of the nsIScriptGlobalObject.h and js/TypeDecls.h includes as well.
Attachment #8456238 -
Attachment is obsolete: true
Attachment #8462699 -
Flags: review+
Comment 10•10 years ago
|
||
Comment on attachment 8462690 [details] [diff] [review] Part 2: Move all of the JSContext craziness in nsWindowWatcher::OpenWindowInternal into one place v2 Review of attachment 8462690 [details] [diff] [review]: ----------------------------------------------------------------- Yes, this is much more grokable. Thank you!
Attachment #8462690 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 11•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fce3d0a6004b
Assignee | ||
Comment 12•10 years ago
|
||
Try push below. Landing in part number order, thanks.
Keywords: checkin-needed
Comment 13•10 years ago
|
||
Hi Bob, part2 failed to apply applying bug1037936part2.patch patching file embedding/components/windowwatcher/src/nsWindowWatcher.cpp Hunk #1 FAILED at 54 1 out of 2 hunks FAILED -- saving rejects to file embedding/components/windowwatcher/src/nsWindowWatcher.cpp.rej could you maybe look into this and rebase to a current tree ? thanks!
Keywords: checkin-needed
Assignee | ||
Comment 14•10 years ago
|
||
r=bholley - from comment 10 Fixed bitrot from subsequent #include change. Also, change checkin comment, which I forgot to do last time.
Attachment #8462690 -
Attachment is obsolete: true
Attachment #8463250 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
r=bholley - from comment 7 Fixed bitrot due to recent #include change.
Attachment #8462699 -
Attachment is obsolete: true
Attachment #8463251 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
Only rebased due to a #include change on 23/7. Compiled locally, but I don't think it warrants a new try push.
Keywords: checkin-needed
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/258e6048d585 https://hg.mozilla.org/integration/mozilla-inbound/rev/21264d92a9d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/b1bdbcfa69c1
Keywords: checkin-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/258e6048d585 https://hg.mozilla.org/mozilla-central/rev/21264d92a9d8 https://hg.mozilla.org/mozilla-central/rev/b1bdbcfa69c1
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
•