Closed Bug 1481467 Opened Last year Closed Last year

Remove remaining JSAutoRealmAllowCCW uses in XPConnect

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(3 files)

Just a few relatively minor things left.
The targetVal argument is always same-compartment with the JSContext, so we only need to use JSAutoRealm in the FindTargetObject case.
Attachment #8998189 - Flags: review?(kmaglione+bmo)
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.
Attachment #8998191 - Flags: review?(bzbarsky)
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.
Attachment #8998195 - Flags: review?(bzbarsky)
And that's all for XPConnect. These patches (and the ones in the dom/ bug) are green on Linux64 on Try, FWIW.
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
Attachment #8998191 - Flags: review?(bzbarsky) → review+
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?
Attachment #8998195 - Flags: review?(bzbarsky) → review+
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.
Attachment #8998189 - Flags: review?(kmaglione+bmo) → review+
(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).
(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.
Blocks: 1481772
Pushed by jandemooij@gmail.com:
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
Pushed with comments addressed. Thanks for the quick reviews!
You need to log in before you can comment on or make changes to this bug.