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

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

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

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace nsCxPusher in nsJSObjWrapper::NP_HasMethod v1

Given the AutoJSExceptionReporter seems like this needs the legacy error reporting.
I've just split the GetJSContext function, so we can get the intermediate object for our purposes.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace nsCxPusher in nsJSObjWrapper::NP_HasProperty v1

Very similar to part 1.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 3: Replace nsCxPusher in nsJSObjWrapper::NP_GetProperty v1

Only slight concern here was whether JS_GetPropertyById could cause script to run.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 4: Replace nsCxPusher in nsJSObjWrapper::NP_SetProperty v1

Same concern over JS_SetPropertyById possibly running script?

Comment 6

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace nsCxPusher in nsJSObjWrapper::NP_RemoveProperty v1

Variation on a theme. :)

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 6: Replace nsCxPusher in nsJSObjWrapper::NP_Enumerate v1

Last one in the set (apart from the one in doInvoke, which would seem to point to an AutoEntryScript, so I've left it for now).

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8456183 [details] [diff] [review]
Part 1: Replace nsCxPusher in nsJSObjWrapper::NP_HasMethod v1

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +292,5 @@
>  namespace plugins {
>  namespace parent {
>  
> +static nsIScriptGlobalObject*
> +GetScriptGlobalObject(NPP npp)

Please have this function return an nsIGlobalObject and call it GetGlobalObject. nsIScriptGlobalObject is deprecated.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8456188 [details] [diff] [review]
Part 3: Replace nsCxPusher in nsJSObjWrapper::NP_GetProperty v1

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

Yeah, I think we need an AutoEntryScript here. :-(

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8456191 [details] [diff] [review]
Part 4: Replace nsCxPusher in nsJSObjWrapper::NP_SetProperty v1

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

And here. I'm guessing that there is plugin code that relies on triggering getters/setters.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8456192 [details] [diff] [review]
Part 5: Replace nsCxPusher in nsJSObjWrapper::NP_RemoveProperty v1

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

The rest of these would only trigger script in the case of a proxy, which ye olde NPAPI plugins are unlikely to depend on.

Comment 12

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #8)

Thanks for all the reviews.

> Please have this function return an nsIGlobalObject and call it
> GetGlobalObject. nsIScriptGlobalObject is deprecated.

Well I needed an nsIScriptGlobalObject for the GetJSContext, so do you just want me to add another do_QueryInterface to the top of that?
That doesn't seem to gain us much.


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

> Yeah, I think we need an AutoEntryScript here. :-(

OK, I'll obsolete Parts 3 and 4 and leave them until after Bug 1027553.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #12)
> (In reply to Bobby Holley (:bholley) from comment #8)
> 
> Thanks for all the reviews.
> 
> > Please have this function return an nsIGlobalObject and call it
> > GetGlobalObject. nsIScriptGlobalObject is deprecated.
> 
> Well I needed an nsIScriptGlobalObject for the GetJSContext, so do you just
> want me to add another do_QueryInterface to the top of that?
> That doesn't seem to gain us much.

Ah ok. GetJSContext will go away at some point, right? Anyway, I don't care all that much. Do what makes sense.

Comment 14

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #13)

> Ah ok. GetJSContext will go away at some point, right? Anyway, I don't care
> all that much. Do what makes sense.

Yeah, sorry being dim.
It means we won't need to touch GetGlobalObject when we do that, I'll sort it.

Comment 15

[Bob Owen (:bobowen)]

Attachment: Part 1: Replace nsCxPusher in nsJSObjWrapper::NP_HasMethod v2

(In reply to Bobby Holley (:bholley) from comment #8)

> > +static nsIScriptGlobalObject*
> > +GetScriptGlobalObject(NPP npp)
> 
> Please have this function return an nsIGlobalObject and call it
> GetGlobalObject. nsIScriptGlobalObject is deprecated.

I've changed this and I've used |doc->GetScopeObject();| to get the nsIGlobalObject*.

From debugging it looks like this gives us the inner window, whereas before we were getting the outer window.

I think the inner window is actually what we want for AutoJSAPI and as |GetContext()| (used in |GetJSContext|) delegates to the outer window the result there should be the same.

Sorry, to r? again, but I wanted to make sure that this made sense for plugins.

Comment 16

[Bob Owen (:bobowen)]

Attachment: Part 2: Replace nsCxPusher in nsJSObjWrapper::NP_HasProperty v2

r=bholley - from https://bugzilla.mozilla.org/show_bug.cgi?id=1037904#h8

GetScriptGlobalObject changed to GetGlobalObject.

Comment 17

[Bob Owen (:bobowen)]

Attachment: Part 5: Replace nsCxPusher in nsJSObjWrapper::NP_RemoveProperty v2

r=bholley - from comment 11

GetScriptGlobalObject changed to GetGlobalObject.

Comment 18

[Bob Owen (:bobowen)]

Attachment: Part 6: Replace nsCxPusher in nsJSObjWrapper::NP_Enumerate v2

r=bholley - from https://bugzilla.mozilla.org/show_bug.cgi?id=1037904#h12

GetScriptGlobalObject changed to GetGlobalObject.

Comment 19

[Bob Owen (:bobowen)]

Seems like Bugzilla doesn't pick up the fragment identifier.

Try push with the latest patches:
https://tbpl.mozilla.org/?tree=Try&rev=70ca3f151742

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8457934 [details] [diff] [review]
Part 1: Replace nsCxPusher in nsJSObjWrapper::NP_HasMethod v2

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

(In reply to Bob Owen (:bobowen) from comment #15)
> From debugging it looks like this gives us the inner window, whereas before
> we were getting the outer window.
> 
> I think the inner window is actually what we want for AutoJSAPI and as
> |GetContext()| (used in |GetJSContext|) delegates to the outer window the
> result there should be the same.
> 
> Sorry, to r? again, but I wanted to make sure that this made sense for
> plugins.

Yeah, I think we want the inner that's actually associated with the document here.

Comment 21

[Bob Owen (:bobowen)]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=70ca3f151742

Landing in part number order, patches 3 and 4 have been obsoleted.
Thanks.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/632730473b53
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7954ed4d0bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d7be299557
https://hg.mozilla.org/integration/mozilla-inbound/rev/9701d7726c62

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/632730473b53
https://hg.mozilla.org/mozilla-central/rev/b7954ed4d0bd
https://hg.mozilla.org/mozilla-central/rev/53d7be299557
https://hg.mozilla.org/mozilla-central/rev/9701d7726c62