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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(6 files, 2 obsolete files)
10.28 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.93 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
4.88 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
9.00 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.27 KB,
patch
|
Details | Diff | Splinter Review | |
5.32 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Priority: -- → P2
![]() |
Assignee | |
Comment 1•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 2•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 3•7 years ago
|
||
This more closely matches what should happen with entry/incumbent globals in
the spec.
Attachment #8976772 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 4•7 years ago
|
||
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)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8976768 -
Attachment is obsolete: true
Attachment #8976768 -
Flags: review?(bugs)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8976769 -
Attachment is obsolete: true
Attachment #8976769 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 5•7 years ago
|
||
Attachment #8976774 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 6•7 years ago
|
||
There is no reason to do that, apart from allowing Xrays to shoot themselves in the foot....
Attachment #8976775 -
Flags: review?(bugs)
![]() |
Assignee | |
Comment 7•7 years ago
|
||
![]() |
Assignee | |
Comment 8•7 years ago
|
||
Updated•7 years ago
|
Attachment #8976772 -
Flags: review?(bugs) → review+
Comment 9•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8976774 -
Flags: review?(bugs) → review+
Updated•7 years ago
|
Attachment #8976775 -
Flags: review?(bugs) → review+
Comment 10•7 years ago
|
||
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
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c6c43a2b0c34
https://hg.mozilla.org/mozilla-central/rev/43022d000bc9
https://hg.mozilla.org/mozilla-central/rev/46ba22fc8306
https://hg.mozilla.org/mozilla-central/rev/53386e083d7b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•