1481467 - Remove remaining JSAutoRealmAllowCCW uses in XPConnect

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Jan de Mooij [:jandem]]

Just a few relatively minor things left.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in mozJSComponentLoader::ImportInto

The targetVal argument is always same-compartment with the JSContext, so we only need to use JSAutoRealm in the FindTargetObject case.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in JSXrayTraits::getOwnPropertyFromTargetIfSafe

Because getOwnPropertyFromTargetIfSafe operates in the Xray target realm/compartment and we cannot use the CCW with JSAutoRealm, we pass the caller's global as wrapperGlobal and use that.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in XrayTraits::attachExpandoObject

Because XrayTraits::attachExpandoObject operates in the Xray target realm/compartment and we cannot use the CCW with JSAutoRealm, we pass the caller's global as exclusiveWrapperGlobal and use that.

This also changes XrayWrapper<Base, Traits>::defineProperty to call ensureExpandoObject in the wrapper (instead of target) realm. This didn't matter before, because ensureExpandoObject immediately entered the target realm anyway.

Comment 4

[Jan de Mooij [:jandem]]

And that's all for XPConnect. These patches (and the ones in the dom/ bug) are green on Linux64 on Try, FWIW.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8998191 [details] [diff] [review]
Part 2 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in JSXrayTraits::getOwnPropertyFromTargetIfSafe

> Because getOwnPropertyFromTargetIfSafe operates in the Xray target realm/compartment and we cannot use the CCW with JSAutoRealm, we pass the caller's global as wrapperGlobal and use that.

s/CCW/Xray wrapper/, maybe?

r=me

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8998195 [details] [diff] [review]
Part 3 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in XrayTraits::attachExpandoObject

> Because XrayTraits::attachExpandoObject operates in the Xray target realm/compartment and we cannot use the CCW

Again, s/CCW/Xray wrapper/?

>-    // We're placing an expando. The expando objects live in the target

Why move this JSAutoRealm?  I know ensureExpandoObject does its own compartment-entering, but why not enter it up front here if we know we're going to need it?

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8998189 [details] [diff] [review]
Part 1 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in mozJSComponentLoader::ImportInto

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

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +1018,5 @@
>          }
>      } else {
>          FindTargetObject(cx, &targetObject);
> +        if (targetObject) {
> +            ar.emplace(cx, targetObject);

I'm pretty sure we can just get rid of this and keep the AssertSameCompartment assertion. FindTargetObject should always return either the current global or an NSVO object on the current scripts scope chain, either of which should be guaranteed to be in the caller's compartment. If they ever aren't, I'd like to know about it.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> Why move this JSAutoRealm?  I know ensureExpandoObject does its own
> compartment-entering, but why not enter it up front here if we know we're
> going to need it?

The commit message has this:

"This also changes XrayWrapper<Base, Traits>::defineProperty to call ensureExpandoObject in the wrapper (instead of target) realm. This didn't matter before, because ensureExpandoObject immediately entered the target realm anyway."

ensureExpandoObject must be called in the wrapper compartment now so we can use JS::CurrentGlobalOrNull(cx).

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Kris Maglione [:kmag] from comment #7)
> I'm pretty sure we can just get rid of this and keep the
> AssertSameCompartment assertion. FindTargetObject should always return
> either the current global or an NSVO object on the current scripts scope
> chain, either of which should be guaranteed to be in the caller's
> compartment. If they ever aren't, I'd like to know about it.

Ah. I'll file a follow-up bug and post a patch there before I land this (probably tomorrow), just to do the most minimal/safe thing in this bug.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/980f42a5b1bc
part 1 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in mozJSComponentLoader::ImportInto. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b38554d067f
part 2 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in JSXrayTraits::getOwnPropertyFromTargetIfSafe. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3ee665001f
part 3 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in XrayTraits::attachExpandoObject. r=bz

Comment 11

[Jan de Mooij [:jandem]]

Pushed with comments addressed. Thanks for the quick reviews!

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/980f42a5b1bc
https://hg.mozilla.org/mozilla-central/rev/8b38554d067f
https://hg.mozilla.org/mozilla-central/rev/dc3ee665001f