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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 + fixed
firefox54 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(2 files)

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.
No longer depends on: 1273251
Blocks: 1338563
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 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+
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
Blocks: 1341488
[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.
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)
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.
(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.
(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.
(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)
(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)
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.
(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 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-
(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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/8999082d8a89
https://hg.mozilla.org/mozilla-central/rev/d93ecd34da85
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Since bug 1338563 is supposed to be fixed by this and affects 53, can we get an uplift request?  Thanks
Flags: needinfo?(kmaglione+bmo)
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?
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 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+
Attachment #8836411 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1347523
You need to log in before you can comment on or make changes to this bug.