Closed Bug 1461711 Opened 7 years ago Closed 7 years ago

CustomElementRegistry::Define is doing some weird stuff with compartments

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(6 files, 2 obsolete files)

Specifically: 1) It's using an AutoJSAPI instead of just using the caller's cx. Per spec, it should really just use the calling context; this can become observable in various ways as we fix up our cross-compartment wrappers. 2) There's an unnecessary call to JS_WrapValue for "rootedv" as far as I can tell. 3) The checks for lifecycle callbacks enter the unwrapped proto compartment, which is probably not desirable. The big question is what happens if a JS Xray to a content-side callbacks object is passed. But arguably in that case we _should_ fail out instead of defining some random "we don't know what they are" callbacks... At least those are some issues at first glance.
Priority: -- → P2
Depends on: 1462453
Per spec dictionaries sort their members, but there are some cases where we use a dictionary as a convenience implementation detail. In those cases, sorting the members changes the observable behavior in a way that is not desired.
Attachment #8976768 - Flags: review?(bugs)
This check is already done by the dictionary init method. This function just makes us do extra non-spec get operations. To actually implement the spec properly, we need to also avoid sorting the dictionary members and not allow null members; the tests testing for that stuff were passing due to exceptions getting thrown from CheckLifeCycleCallbacks.
Attachment #8976769 - Flags: review?(bugs)
This more closely matches what should happen with entry/incumbent globals in the spec.
Attachment #8976772 - Flags: review?(bugs)
1) The passed-in constructor is already same-compartment with the passed-in aCx, so there is no need to enter its Realm to work with it. 2) aCx is already in the compartment of constructorProtoUnwrapped when we do JS_WrapValue on rootedv, which is initialized to constructorProtoUnwrapped. That JS_WrapValue call is not needed.
Attachment #8976773 - Flags: review?(bugs)
Attachment #8976768 - Attachment is obsolete: true
Attachment #8976768 - Flags: review?(bugs)
Attachment #8976769 - Attachment is obsolete: true
Attachment #8976769 - Flags: review?(bugs)
There is no reason to do that, apart from allowing Xrays to shoot themselves in the foot....
Attachment #8976775 - Flags: review?(bugs)
Attachment #8976772 - Flags: review?(bugs) → review+
Comment on attachment 8976773 [details] [diff] [review] part 2. Remove some unnecessary Realm machinery in CustomElementRegistry::Define Ah, right, we have JSAutoRealm ar(cx, constructorProtoUnwrapped); before if (!JS_WrapValue(aCx, &rootedv) || !callbacksHolder->Init(aCx, rootedv)) {
Attachment #8976773 - Flags: review?(bugs) → review+
Attachment #8976774 - Flags: review?(bugs) → review+
Attachment #8976775 - Flags: review?(bugs) → review+
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6c43a2b0c34 part 1. Change CustomElementRegistry::Define to just take a JSContext from the caller instead of setting up an AutoJSAPI itself. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/43022d000bc9 part 2. Remove some unnecessary Realm machinery in CustomElementRegistry::Define. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/46ba22fc8306 part 3. Fix potential leak of LifecycleCallbacks in CustomElementRegistry::Define. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/53386e083d7b part 4. Stop unwrapping the custom element prototype when getting the lifecycle callbacsk. r=smaug
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: