1473865 - Use JS::GetNonCCWObjectGlobal in xpc::WindowGlobalOrNull

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

Instead of js::GetGlobalForObjectCrossCompartment.

This will require auditing all callers (and I want to do some clean up as well).

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove unused nsJSUtils::GetStaticScriptContext

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove unused nsContentUtils::GetDocumentFromCaller

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Inline nsJSUtils::GetStaticScriptGlobal into its callers

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8990343 [details] [diff] [review]
Part 1 - Remove unused nsJSUtils::GetStaticScriptContext

Excellent.  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8990344 [details] [diff] [review]
Part 2 - Remove unused nsContentUtils::GetDocumentFromCaller

Even more excellent.  ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Jan de Mooij [:jandem]]

(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.

Comment 8

[Jan de Mooij [:jandem]]

(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 9

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Use xpc::WindowOrNull instead of xpc::WindowGlobalOrNull in a few places

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Use JS::GetNonCCWObjectGlobal in xpc::WindowGlobalOrNull

At this point the WindowGlobalOrNull callers are:

* https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/dom/xbl/nsXBLProtoImplField.cpp#395

Caller asserts ValueHasISupportsPrivate.

* https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/js/xpconnect/src/XPCJSContext.cpp#651

Does a CheckedUnwrap first.

* https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/dom/base/WindowNamedPropertiesHandler.cpp#107

It's a WindowNamedPropertiesHandler proxy. Similar one in WindowNamedPropertiesHandler::ownPropNames.

* https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/dom/bindings/BindingUtils.cpp#4023

Does UncheckedUnwrap.

* https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/dom/bindings/CallbackObject.cpp#185

Does UncheckedUnwrap.

* https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/dom/promise/Promise.cpp#503

Asserts !IsWrapper.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/33dd02744963
https://hg.mozilla.org/mozilla-central/rev/bf38bf7d87c6
https://hg.mozilla.org/mozilla-central/rev/4b42a006f52a

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Jan de Mooij [:jandem]]

(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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, we're always in a realm when we land in that code.  CurrentWindowOrNull should be fine, and simpler to read.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/794fbe337244
https://hg.mozilla.org/mozilla-central/rev/0e4f87857636