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

RESOLVED FIXED in mozilla34

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

Trunk
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

6.66 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
4.12 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
6.98 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8455515 [details] [diff] [review]
Part 1: Replace nsCxPusher in nsXBLBinding::ChangeDocument v1

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

3 years ago
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
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

3 years ago
Created attachment 8456201 [details] [diff] [review]
Part 1: Replace nsCxPusher in nsXBLBinding::ChangeDocument v2

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

3 years ago
Created attachment 8456228 [details] [diff] [review]
Part 2: Move all of the JSContext craziness in nsWindowWatcher::OpenWindowInternal into one place v1

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

3 years ago
Created attachment 8456238 [details] [diff] [review]
Part 3: Replace nsCxPusher in nsWindowWatcher::OpenWindowInternal v1

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 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 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

3 years ago
Created attachment 8462690 [details] [diff] [review]
Part 2: Move all of the JSContext craziness in nsWindowWatcher::OpenWindowInternal into one place v2

(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

3 years ago
Created attachment 8462699 [details] [diff] [review]
Part 3: Replace nsCxPusher in nsWindowWatcher::OpenWindowInternal v2

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 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

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fce3d0a6004b
(Assignee)

Comment 12

3 years ago
Try push below.

Landing in part number order, thanks.
Keywords: checkin-needed
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

3 years ago
Created attachment 8463250 [details] [diff] [review]
Part 2: Replace second nsCxPusher in nsWindowWatcher::OpenWindowInternal v3

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

3 years ago
Created attachment 8463251 [details] [diff] [review]
Part 3: Replace nsCxPusher in nsWindowWatcher::OpenWindowInternal v3

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

3 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
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
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.