Closed Bug 1037904 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(4 files, 6 obsolete files)

2.83 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.49 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
1.45 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
1.40 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
https://tbpl.mozilla.org/?tree=Try&rev=dac42309f90b
Assignee: nobody → bobowencode
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.
Attachment #8456183 - Flags: review?(bobbyholley)
Very similar to part 1.
Attachment #8456186 - Flags: review?(bobbyholley)
Only slight concern here was whether JS_GetPropertyById could cause script to run.
Attachment #8456188 - Flags: review?(bobbyholley)
Same concern over JS_SetPropertyById possibly running script?
Attachment #8456191 - Flags: review?(bobbyholley)
Variation on a theme. :)
Attachment #8456192 - Flags: review?(bobbyholley)
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).
Attachment #8456194 - Flags: review?(bobbyholley)
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.
Attachment #8456183 - Flags: review?(bobbyholley) → review+
Attachment #8456186 - Flags: review?(bobbyholley) → review+
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. :-(
Attachment #8456188 - Flags: review?(bobbyholley) → review-
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.
Attachment #8456191 - Flags: review?(bobbyholley) → review-
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.
Attachment #8456192 - Flags: review?(bobbyholley) → review+
Attachment #8456194 - Flags: review?(bobbyholley) → review+
(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.
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
(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.
Attachment #8456188 - Attachment is obsolete: true
Attachment #8456191 - Attachment is obsolete: true
(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.
Attachment #8456183 - Attachment is obsolete: true
Attachment #8457934 - Flags: review?(bobbyholley)
r=bholley - from https://bugzilla.mozilla.org/show_bug.cgi?id=1037904#h8

GetScriptGlobalObject changed to GetGlobalObject.
Attachment #8456186 - Attachment is obsolete: true
Attachment #8457935 - Flags: review+
r=bholley - from comment 11

GetScriptGlobalObject changed to GetGlobalObject.
Attachment #8456192 - Attachment is obsolete: true
Attachment #8457936 - Flags: review+
r=bholley - from https://bugzilla.mozilla.org/show_bug.cgi?id=1037904#h12

GetScriptGlobalObject changed to GetGlobalObject.
Attachment #8456194 - Attachment is obsolete: true
Attachment #8457937 - Flags: review+
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 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.
Attachment #8457934 - Flags: review?(bobbyholley) → review+
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=70ca3f151742

Landing in part number order, patches 3 and 4 have been obsoleted.
Thanks.
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.