Closed Bug 1473865 Opened 2 years ago Closed 2 years ago

Use JS::GetNonCCWObjectGlobal in xpc::WindowGlobalOrNull

Categories

(Core :: XPConnect, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(5 files)

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+
(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.
(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..
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
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+
(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.
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/794fbe337244
https://hg.mozilla.org/mozilla-central/rev/0e4f87857636
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.