Use JS::GetNonCCWObjectGlobal in xpc::WindowGlobalOrNull

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

11 months ago
Instead of js::GetGlobalForObjectCrossCompartment.

This will require auditing all callers (and I want to do some clean up as well).
Comment on attachment 8990343 [details] [diff] [review]
Part 1 - Remove unused nsJSUtils::GetStaticScriptContext

Excellent.  ;)
Attachment #8990343 - Flags: review?(bzbarsky) → review+
Comment on attachment 8990344 [details] [diff] [review]
Part 2 - Remove unused nsContentUtils::GetDocumentFromCaller

Even more excellent.  ;)
Attachment #8990344 - Flags: review?(bzbarsky) → review+
Comment on attachment 8990346 [details] [diff] [review]
Part 3 - Inline nsJSUtils::GetStaticScriptGlobal into its callers

>+++ b/dom/base/Navigator.cpp
>+  nsCOMPtr<nsPIDOMWindowInner> win = xpc::WindowGlobalOrNull(aGlobal);

We know aGlobal is an actual global here, so this can be xpc::WindowOrNull, not xpc::WindowGlobalOrNull.

>+++ b/dom/workers/WorkerError.cpp
>+        else if ((sgo = xpc::WindowGlobalOrNull(global)))

Again, WindowOrNull should work.   

Also, while you're here, can we push the decl of sgo into the if condition?  And maybe rename sgo to win and make it be nsGlobalWindowInner?

In any case, we should include nsGlobalWindowInner.h here.

Same thing at the other callsite in this file.

>+++ b/dom/workers/WorkerPrivate.cpp
>+      if (JSObject* global = JS::CurrentGlobalOrNull(aCx)) {
>+        globalWindow = xpc::WindowGlobalOrNull(global);
>       }

Given that globalWindow is null before this runs, we can replace all that with:

   globalWindow = xpc::CurrentWindowOrNull(aCx);

r=me
Attachment #8990346 - Flags: review?(bzbarsky) → review+
Assignee

Comment 7

11 months ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> We know aGlobal is an actual global here, so this can be xpc::WindowOrNull,
> not xpc::WindowGlobalOrNull.

Ah yes, nice. There are more callers that pass a known-global to WindowGlobalOrNull, I'll convert them too.
Assignee

Comment 8

11 months ago
(In reply to Jan de Mooij [:jandem] from comment #7)
> There are more callers that pass a known-global to
> WindowGlobalOrNull, I'll convert them too.

Separate patch, of course..

Comment 11

11 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33dd02744963
part 1 - Remove unused nsJSUtils::GetStaticScriptContext. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf38bf7d87c6
part 2 - Remove unused nsContentUtils::GetDocumentFromCaller. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b42a006f52a
part 3 - Inline nsJSUtils::GetStaticScriptGlobal into its callers. r=bz
Assignee

Updated

11 months ago
Keywords: leave-open
Priority: -- → P2
Comment on attachment 8990673 [details] [diff] [review]
Part 4 - Use xpc::WindowOrNull instead of xpc::WindowGlobalOrNull in a few places

>+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
>+    nsGlobalWindowInner* win = WindowOrNull(CurrentGlobalOrNull(cx));

Why not CurrentWindowOrNull?

r=me
Attachment #8990673 - Flags: review?(bzbarsky) → review+
Comment on attachment 8990674 [details] [diff] [review]
Part 5 - Use JS::GetNonCCWObjectGlobal in xpc::WindowGlobalOrNull

r=me  but we should adjust the documentation in the header to make this restriction clear.
Attachment #8990674 - Flags: review?(bzbarsky) → review+
Assignee

Comment 15

11 months ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> >+    nsGlobalWindowInner* win = WindowOrNull(CurrentGlobalOrNull(cx));
> 
> Why not CurrentWindowOrNull?

WindowOrNull(CurrentGlobalOrNull(cx)) will assert if cx is not in a realm, whereas CurrentWindowOrNull will return nullptr... I'm happy to change it to CurrentWindowOrNull though.
Yeah, we're always in a realm when we land in that code.  CurrentWindowOrNull should be fine, and simpler to read.

Comment 17

11 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/794fbe337244
part 4 - Use xpc::WindowOrNull instead of xpc::WindowGlobalOrNull in a few places. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4f87857636
part 5 - Use JS::GetNonCCWObjectGlobal in xpc::WindowGlobalOrNull. r=bz
Assignee

Updated

11 months ago
Keywords: leave-open

Comment 18

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/794fbe337244
https://hg.mozilla.org/mozilla-central/rev/0e4f87857636
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.