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

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

Attachment: 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.

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace nsCxPusher in nsXBLBinding::ChangeDocument v2

r=bholley - from comment 2

Changed to AutoJSAPI::Init instead of AutoJSAPI::InitWithLegacyErrorReporting

Comment 4

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 5

[Bob Owen (:bobowen)]

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

Comment 6

[Bobby Holley (:bholley)]

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()?

Comment 7

[Bobby Holley (:bholley)]

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.

Comment 8

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 9

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 10

[Bobby Holley (:bholley)]

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!

Comment 11

[Bob Owen (:bobowen)]

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

Comment 12

[Bob Owen (:bobowen)]

Try push below.

Landing in part number order, thanks.

Comment 13

[Carsten Book [:Tomcat]]

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!

Comment 14

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 15

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace nsCxPusher in nsWindowWatcher::OpenWindowInternal v3

r=bholley - from comment 7

Fixed bitrot due to recent #include change.

Comment 16

[Bob Owen (:bobowen)]

Only rebased due to a #include change on 23/7.

Compiled locally, but I don't think it warrants a new try push.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/258e6048d585
https://hg.mozilla.org/mozilla-central/rev/21264d92a9d8
https://hg.mozilla.org/mozilla-central/rev/b1bdbcfa69c1