Closed
Bug 1322273
Opened 7 years ago
Closed 7 years ago
Return DeadObjectProxy when attempting to wrap object for a nuked compartment
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox53 | + | fixed |
firefox54 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
Details
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
bholley
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
59 bytes,
text/x-review-board-request
|
bholley
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
Bug 1273251 changes WrapObject to return an opaque wrapper when attempting to wrap an object from or for a nuked compartment. That prevents issues with nuked compartments continuing to have external effects, but it still holds a strong reference to those objects. We should instead return a DeadObjectProxy, which solves both issues.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8836411 [details] Bug 1322273: Return DeadObjectProxy when wrapping for nuked compartment. https://reviewboard.mozilla.org/r/111838/#review113808 So, looking at this a bit I think this belongs in the reification step of the prewrap callback. That's where we mint and return possibly-non-wrapper things.
Attachment #8836411 -
Flags: review?(bobbyholley) → review-
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8836411 [details] Bug 1322273: Return DeadObjectProxy when wrapping for nuked compartment. https://reviewboard.mozilla.org/r/111838/#review115746 r=me with that. ::: js/xpconnect/wrappers/WrapperFactory.cpp:193 (Diff revision 3) > + RootedObject ccw(cx, Wrapper::New(cx, obj, &CrossCompartmentWrapper::singleton)); > + > + NukeCrossCompartmentWrapper(cx, ccw); I would really prefer an explicit API to create dead object proxies directly - this seems pretty roundabout. Please do that in either this bug or a followup (probably with review from a more active JS peer).
Attachment #8836411 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8836411 [details] Bug 1322273: Return DeadObjectProxy when wrapping for nuked compartment. https://reviewboard.mozilla.org/r/111838/#review115746 > I would really prefer an explicit API to create dead object proxies directly - this seems pretty roundabout. Please do that in either this bug or a followup (probably with review from a more active JS peer). I'll do it as a follow-up. This needs to be uplifted before the merge, so I don't want to block on changing that encapsulation.
Pushed by maglione.k@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e85a00771f86 Return DeadObjectProxy when wrapping for nuked compartment. r=bholley
Assignee | ||
Comment 8•7 years ago
|
||
[Tracking Requested - why for this release]: Bug 1273251 caused a crash regression that this change fixes. It should bake on nightly for a couple of days, but we should uplift to Aurora before the merge.
Comment 9•7 years ago
|
||
Sorry had to back this out for Assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=79203068&repo=autoland&lineNumber=4790 https://hg.mozilla.org/integration/autoland/rev/e93cf8e37f4f
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 10•7 years ago
|
||
Hm. It looks like the special casing in RemapWrapper was still necessary, and my last try run was before I removed it.
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
Oh hm, I just realized that the actual brain transplanting of the window proxy doesn't go through this codepath. So I still don't understand why we had a wrapper between nuked compartments in the wrapper map.
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12) > Oh hm, I just realized that the actual brain transplanting of the window > proxy doesn't go through this codepath. So I still don't understand why we > had a wrapper between nuked compartments in the wrapper map. I don't remember all of the details. I looked into this when it happened with the previous implementation, and it happens when we're transplanting window proxies in SetNewDocument after a reload. The nuked compartment is always the target compartment (i.e., the one we're creating the new wrapper for). I'll look into it again today to try to get the rest of the details.
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12) > Oh hm, I just realized that the actual brain transplanting of the window > proxy doesn't go through this codepath. So I still don't understand why we > had a wrapper between nuked compartments in the wrapper map. It looks like what's happening is this: - The window that's triggering this is reloaded twice. - The first time it's reloaded, we update its window proxy to point to the new inner window. When we nuke the old inner window, we don't nuke that proxy. Which is apparently for two reasons, 1) since we pass js::DontNukeWindowReferences when nuking inner windows, and 2) we don't nuke wrappers out of the target compartment that we're nuking anyway, just wrappers into it. - The second time it's reloaded, we attempt to transplant the proxy that belongs to the first nuked window, but since it belongs to a nuked compartment, that fails. So I guess the two options are to either keep this special case, or to nuke objects, including window proxies, that belong to the compartment that we're nuking, even if we pass DontNukeWindowReferences. I think the latter makes more sense, but is a bit riskier for uplift.
Comment 15•7 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #14) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #12) > > Oh hm, I just realized that the actual brain transplanting of the window > > proxy doesn't go through this codepath. So I still don't understand why we > > had a wrapper between nuked compartments in the wrapper map. > > It looks like what's happening is this: > > - The window that's triggering this is reloaded twice. Ah, and I bet this is a window with an addon id. Normally during post-navigation nuking we don't set the bit on the compartment, because we may navigate back to the window. But we do for addon-created windows: http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/dom/base/nsGlobalWindow.cpp#9420 And it looks like that failing test is about addons. So I think the real issue here is that, in general, nuking nukes wrappers _into_ the nuke compartment, but not _from_ the nuked compartment, and the patch in this bug returns DeadObjectProxies for either direction. That sets us up for a mismatch. Do we actually need to clip wrappers _from_ the compartment? Or can we just make this consistent?
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15) > So I think the real issue here is that, in general, nuking nukes wrappers > _into_ the nuke compartment, but not _from_ the nuked compartment, and the > patch in this bug returns DeadObjectProxies for either direction. That sets > us up for a mismatch. > > Do we actually need to clip wrappers _from_ the compartment? Or can we just > make this consistent? We do. The main reason that I added the wasNuked flag is that sandboxes and frames that still have references to the pages they're embedded in after we destroy them often keep trying to access those external objects after they're destroyed. Marking the compartment unscriptable and cutting event listeners helps, but I think there are plenty of corner cases that still aren't covered.
Flags: needinfo?(kmaglione+bmo)
Comment 17•7 years ago
|
||
Ok. Then Maybe the callsites that set wasNuked should nuke wrappers in _both_ directions before they set the bit? We should also fix the addon window case to unconditionally pass NukeWindowReferences.
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #17) > We should also fix the addon window case to unconditionally pass > NukeWindowReferences. I think that we probably still want to skip nuking window references in other compartments when we're nuking the inner window, so the proxies continue to function when the window navigates. Although, it shouldn't actually matter in practice since by the time we nuke the window, they should have been transplanted to point to the new inner window.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8840270 [details] Bug 1322273: Cut wrappers that point out of a browser compartment when nuking it. https://reviewboard.mozilla.org/r/114758/#review116272 This can't live in NukeCrossCompartmentWrappers, because it has web-observables effects in navigated-away-from pages. That's why I suggested that we should simply have two nuke calls at the callsites that set the flag on the CompartmentPrivate. The alternative would be to redesign the API, but we'd need to make sure it stayed understandable.
Attachment #8840270 -
Flags: review?(bobbyholley) → review-
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #21) > This can't live in NukeCrossCompartmentWrappers, because it has > web-observables effects in navigated-away-from pages. That's why I suggested > that we should simply have two nuke calls at the callsites that set the flag > on the CompartmentPrivate. The alternative would be to redesign the API, but > we'd need to make sure it stayed understandable. There aren't any additional web-observable effects from this change. We already nuke wrappers from web compartments that point into sandboxes and extension windows when nuked (though in practice there should be no way for web compartments to have wrappers into either, since our sandboxes use expanded principals), which this doesn't change one way or the other. The only circumstance where this makes a difference is when the compartment matches both the source and target filter, which means it's either a sandbox compartment, or it's a window with a system or extension principal. Those changes are not web observable.
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8840270 [details] Bug 1322273: Cut wrappers that point out of a browser compartment when nuking it. https://reviewboard.mozilla.org/r/114758/#review116480 ::: js/src/proxy/CrossCompartmentWrapper.cpp:523 (Diff revision 1) > // Iterate through scopes looking for system cross compartment wrappers > // that point to an object that shares a global with obj. This comment is obsolete, please delete it while you're here. ::: js/src/proxy/CrossCompartmentWrapper.cpp:530 (Diff revision 1) > + // If the compartment matches both the source and target filter, it is > + // being destroyed and we are allowed to cut wrappers for it, so we cut > + // all wrappers in its wrapper table. > + bool isTarget = targetFilter.match(c); I think this behavior is too subtle, and would rather have callers be explicit. Let's pass js::NukeReferencesFromTarget as another option, and then have: bool nukeAll = targetFilter.match(c) && nukeReferencesFromTarget == NukeAllReferences; Or somesuch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8840270 [details] Bug 1322273: Cut wrappers that point out of a browser compartment when nuking it. https://reviewboard.mozilla.org/r/114758/#review116522 ::: dom/base/nsGlobalWindow.cpp:9412 (Diff revision 2) > // We want to nuke all references to the add-on compartment. > js::NukeCrossCompartmentWrappers(cx, js::AllCompartments(), > js::SingleCompartment(cpt), > win->IsInnerWindow() ? js::DontNukeWindowReferences > - : js::NukeWindowReferences); > + : js::NukeWindowReferences, > + js::NukeAllReferences); > > // Now mark the compartment as nuked and non-scriptable. > auto compartmentPrivate = xpc::CompartmentPrivate::Get(cpt); > compartmentPrivate->wasNuked = true; > compartmentPrivate->scriptability.Block(); please hoist all of this into a helper on xpcpublic: xpc::NukeAllWrappersForCompartment(cpt, NukeReferencesToWindow) That does the nuke call and then sets the bit on the compartment private, and has comments explaining how the bit on the compartment private causes WrapperFactory to refuse to create wrappers in either direction, and so we need to start with a bidirectional nuke to avoid wrappers transitioning from valid to dead during unrelated wrapper recomputation. ::: js/xpconnect/src/XPCComponents.cpp:3025 (Diff revision 2) > - NukeWindowReferences); > + NukeWindowReferences, > + NukeAllReferences); > > // Now mark the compartment as nuked and non-scriptable. > auto compartmentPrivate = xpc::CompartmentPrivate::Get(sb); > compartmentPrivate->wasNuked = true; > compartmentPrivate->scriptability.Block(); And invoke the helper here too.
Attachment #8840270 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 27•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8999082d8a89381727c2eee5e26ff4ab653cde4e Bug 1322273: Cut wrappers that point out of a browser compartment when nuking it. r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/d93ecd34da85f28e71dca30633dc5a14046f56fd Bug 1322273: Return DeadObjectProxy when wrapping for nuked compartment. r=bholley
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8999082d8a89 https://hg.mozilla.org/mozilla-central/rev/d93ecd34da85
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 29•7 years ago
|
||
Since bug 1338563 is supposed to be fixed by this and affects 53, can we get an uplift request? Thanks
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 30•7 years ago
|
||
Comment on attachment 8840270 [details] Bug 1322273: Cut wrappers that point out of a browser compartment when nuking it. Approval Request Comment [Feature/Bug causing the regression]: Bug 1273251 [User impact if declined]: This issue causes crashes under certain circumstances, primarily while debugging WebExtensions. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: No, automated testing should be sufficient. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: Somewhat, but not excessively. [Why is the change risky/not risky?]: This change is relatively simple, but affects particularly subtle and security-sensitive JS engine code. However, the change fixes a crash, so the risks are fairly balanced. [String changes made/needed]: None.
Flags: needinfo?(kmaglione+bmo)
Attachment #8840270 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 31•7 years ago
|
||
Comment on attachment 8836411 [details] Bug 1322273: Return DeadObjectProxy when wrapping for nuked compartment. Approval Request Comment: See comment 30.
Attachment #8836411 -
Flags: approval-mozilla-aurora?
Comment 32•7 years ago
|
||
Comment on attachment 8840270 [details] Bug 1322273: Cut wrappers that point out of a browser compartment when nuking it. Fixes a current crash, includes new tests, let's take this for 53.
Attachment #8840270 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 33•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/555b2ea881cf https://hg.mozilla.org/releases/mozilla-aurora/rev/9b921f750287
Flags: in-testsuite+
Updated•7 years ago
|
Updated•7 years ago
|
Attachment #8836411 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•