Closed
Bug 1514251
Opened 6 years ago
Closed 6 years ago
Fix ReparentWrapper for same-compartment realms
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: bzbarsky)
References
Details
Attachments
(2 files)
1.44 KB,
patch
|
peterv
:
review+
jandem
:
review+
|
Details | Diff | Splinter Review |
6.51 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
Bug 1514210 is failing this assert on try: JS::Compartment* oldCompartment = js::GetObjectCompartment(oldParent); JS::Compartment* newCompartment = js::GetObjectCompartment(newParent); if (oldCompartment == newCompartment) { MOZ_ASSERT(oldParent == newParent); return; } https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/bindings/BindingUtils.cpp#2157 This code relies on CPG and needs to be fixed somehow.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 1•6 years ago
|
||
This code dates back to when we had a concept of parent as distinct from the concept of global. It was comparing compartments back then because in the same-compartment case it would just JS_SetParent and return. When we got rid of the concept of parents, the code was left as-is, even though at that point we could just as easily compare the two globals. I believe that in the same-compartment-different-globals case this is safe, because in that case JS_TransplantObject will just keep using the original object allocation but JSObject::swap it with the new object, so that it will pick up the new global.
Attachment #9031512 -
Flags: review?(peterv)
Attachment #9031512 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•6 years ago
|
||
We're not changing parents; we're changing globals. Let's be clear about that.
Attachment #9031513 -
Flags: review?(peterv)
Updated•6 years ago
|
Priority: -- → P2
Reporter | ||
Comment 3•6 years ago
|
||
Comment on attachment 9031512 [details] [diff] [review] part 1. Stop relying on compartment-per-global in ReparentWrapper Review of attachment 9031512 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense. Thank you for fixing this and the other bug!
Attachment #9031512 -
Flags: review?(jdemooij) → review+
Updated•6 years ago
|
Attachment #9031512 -
Flags: review?(peterv) → review+
Comment 4•6 years ago
|
||
Comment on attachment 9031513 [details] [diff] [review] part 2. Update naming in ReparentWrapper to reflect reality Review of attachment 9031513 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.h @@ +2329,5 @@ > +// Given a DOM reflector aObj, give its underlying DOM object a reflector in > +// whatever global that underlying DOM object now thinks it should be in. If > +// this is in a different compartment from aObj, aObj will become a > +// cross-compatment wrapper for the new object. Otherwise, aObj will become the > +// new object. New object could be the same object, right? Not sure if we should specify that.
Attachment #9031513 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 5•6 years ago
|
||
> New object could be the same object, right?
Yes. Added:
If the new global is the same as the old global, we just keep using the same object.
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e52c32b7874 part 1. Stop relying on compartment-per-global in ReparentWrapper. r=peterv,jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f0ee812c88 part 2. Update naming in ReparentWrapper to reflect reality. r=peterv
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e52c32b7874 https://hg.mozilla.org/mozilla-central/rev/c8f0ee812c88
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
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
•