1273251 - Remove cross-compartment event handlers and observers when nuking wrappers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kris Maglione [:kmag]]

When we nuke a compartment, we don't currently do anything about DOM event listeners registered by code in that compartment, for nodes in other compartments:

  let sandbox = Cu.Sandbox(window, { sandboxPrototype: window });

  Cu.evalInSandbox(`
    document.body.addEventListener("click", event => {
      document.body.insertAdjacentHTML("beforeend", "<div>Clicked.</div>");
    });
  `, sandbox);

  Cu.nukeSandbox(sandbox);

The events continue to fire, and objects held alive by the closure continue to fire.

The wrappers aren't cut because the CallbackObject class intentionally doesn't re-wrap the listener for the target compartment, so the simplest solution to this would probably be finding a way to wrap the listeners without creating security issues.

It would be nicer, though, if we could explicitly unregister the listeners, rather than have them continue to fire on dead wrappers.

Comment 1

[Andrew Overholt [:overholt]]

How urgent is this, Kris? Basically I want to know if you'll be angry if I say this is "backlog" (meaning > ~3 months) :)

Comment 2

[Kris Maglione [:kmag]]

I don't think it's urgent. We triaged the bug it blocks as P3 for the moment, so it definitely doesn't need to be fixed immediately, but I think we're going to have to fix it some time in the next few months.

Comment 3

[Andrew Overholt [:overholt]]

Ok, thanks! (fixlater = "ideally in the next few months")

CCing some relevant people.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I wonder how to do this efficiently. Going through all the possible event targets on which some sandbox JS may have added listeners...
Some rewrapping might be simpler.

bz might have ideas what kind of wrapper CallbackObject might be able to have.
That of course applies to all sorts of callbacks, not only event listeners.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, so... CallbackObject very purposefully doesn't do wrappers.  Here's a simple example why (not the only example, just off the top of my head): say chrome code does addEventListener on a content node and passes in an object with a handleEvent method.  If we wrapped that into the compartment of the content node, then trying to fire the listener would fail, because the Get(object, "handleEvent") would throw.

Here's a question, though: is it possible to efficiently detect a nuked compartment?  Because what we could do is just have the tracing code in CallbackObject check whether the thing it's pointing to is nuked and if so just drop the ref instead of tracing it or something.  We'd need to add some annoying null-checks in CallSetup, but it shouldn't be too horrible...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

That would be doable. I means going through all the jsholders and I guess clearing all the
js refs pointing to nuked compartments. Would need to go through all the
jsholders and ensure our null checks are right.
Still, doable.


(I wasn't thinking cross-compartment wrappers here, but some dummy wrappers where we can kill the edge from dummy wrapper -> actual object)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I means going through all the jsholders and I guess clearing all the js refs pointing to nuked compartments.

I was thinking something more like this:

1)  Add a way to mark a compartment as nuked (e.g. mccr8 suggested just putting it on the compartment private).
2)  CallbackObject's trace method (c.f. NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(CallbackObject)) checks whether mCallback's compartment is nuked.  If it is, then instead of tracing stuff we just null out mCallback/mCreationStack/etc.  In other words, make the nuking lazy so you don't have to hunt down all the stuff at nuke time.  This does slow down tracing of CallbackObject somewhat, of course...

Comment 8

[Kris Maglione [:kmag]]

I think that would solve my use cases, as long as we also check for nuked compartments before calling the callback normally (since there's no way of knowing how soon we'll do a full CC run after nuking the compartment).

I wonder if we could add a flag to CallbackObject when the callback belongs to a different compartment than the WebIDL object it was created for, and only check for nuked compartments in that case. I'm not sure the overhead of the nuked compartment checks would be enough to warrant it, but that should make the overhead pretty minimal.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I wonder if we could add a flag to CallbackObject when the callback belongs
> to a different compartment than the WebIDL object it was created for

We could, but since the compartment of a WebIDL object can change (c.f. adoptNode), we would need logic for updating that flag or something.  Furthermore, the "object we're created for" is not that well defined in some cases, and is not easily available at the point when the CallbackObject is constructed...

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Kris Maglione [:kmag] from comment #8)
> I think that would solve my use cases, as long as we also check for nuked
> compartments before calling the callback normally (since there's no way of
> knowing how soon we'll do a full CC run after nuking the compartment).

Do you really need these to be clipped immediately? It would be easier to have this as more of a "best effort" kind of thing. The CC runs every 10 seconds usually so it isn't like things will be around for long.

CAN_SKIP might be a better place for this, as it only runs once per CC, not during the CC, rather than every time we trace these objects (in the CC and GC). But maybe the perf overhead is not a big deal (which is probably a couple of loads and a branch?).

Comment 11

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> Do you really need these to be clipped immediately? It would be easier to
> have this as more of a "best effort" kind of thing. The CC runs every 10
> seconds usually so it isn't like things will be around for long.

In the case of content scripts, having code run after we've nuked the sandbox is a pretty big deal, especially for developers who are reloading extensions to test changes.

I'm planning to make some changes to sterilize the sandbox as much as possible after we nuke it, but I can only do so much. At the moment, the sandbox can still make API calls that interfere with a new instance of a reloaded add-on, which I can fix. It can also run code that adds more listeners, or creates more cross-compartment proxies, which I can't fix.

> CAN_SKIP might be a better place for this, as it only runs once per CC, not
> during the CC, rather than every time we trace these objects (in the CC and
> GC). But maybe the perf overhead is not a big deal (which is probably a
> couple of loads and a branch?).

If we're only checking the callback, a couple of loads and a branch sounds right. Possibly not even a branch for the normal case, if we annotate with MOZ_UNLIKELY.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Do you really need these to be clipped immediately?

It's preferable to do it that way, because otherwise you get inconsistent behavior where your callbacks will _sometimes_ run after nuking and sometimes not and you can accidentally depend on them running.

On the main thread we already check that the unwrapped object of the callback doesn't have script disabled (in the nsScriptSecurityManager::ScriptAllowed sense).  Depending on the precise semantics we want here we could probably do that check on the actual object of the callback too, I guess.

What _do_ we want the semantics to be in the case when code inside a sandbox adds some function from outside the sandbox as an event listener to some DOM node and then the sandbox gets nuked?  Note that what the node is storing in this case is the cross-compartment wrapper from inside the sandbox to the function involved...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and just to make sure I properly understand the setup... once we nuke a sandbox we do not reuse that sandbox for anything else, right?  The "extension is reloaded" case creates a new sandbox?  I've been assuming that all through the discussion above, but wanted to make that explicit.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> What _do_ we want the semantics to be in the case when code inside a sandbox
> adds some function from outside the sandbox as an event listener to some DOM
> node and then the sandbox gets nuked?  Note that what the node is storing in
> this case is the cross-compartment wrapper from inside the sandbox to the
> function involved...

Hm. That's a good question. I think that in the case of a sandbox, nuking the
wrapper makes sense. And I'm pretty sure that already happens, since we nuke
wrappers both into and out of the compartment when we nuke the sandbox.

In other cases, I'm not sure. For example, I'd expect this to work:

    window.opener.addEventListener("hashchange", window.opener.onAuthHathChange);

I think that should work now, since I'm pretty sure we don't currently nuke
content->content proxies. If we added a nuked compartment check, it might
break.

(In reply to Boris Zbarsky [:bz] from comment #13)
> Oh, and just to make sure I properly understand the setup... once we nuke a
> sandbox we do not reuse that sandbox for anything else, right?  The
> "extension is reloaded" case creates a new sandbox?  I've been assuming that
> all through the discussion above, but wanted to make that explicit.

Right, we never reuse sandboxes. We do use the same IDs for message passing,
though, so messages to/from the sandboxes have the same effect as if they
belonged to the new instance.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> In other cases, I'm not sure. For example, I'd expect this to work:
>    window.opener.addEventListener("hashchange", window.opener.onAuthHathChange);

This code is running in a sandbox?  And this sandbox has Xrays to "window.opener"?  Or you're talking about running this on the web?

On the web, there is no issue at all with that code: If that call succeeds then there is no cross-compartment wrapper involved at all and hence no problem.  What would be more interesting is this (still on the web):

  window.addEventListener.call(window.opener, "hashchange", window.opener.onAuthHathChange);

which does store a cross-compartment wrapper in the event listener.  But we don't nuke web compartments anyway, right?

If the code being cited above is running in a sandbox with Xrays to window.opener, then it already breaks when we nuke the wrapper, if we nuke edges going out of the sandbox...

Comment 16

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> This code is running in a sandbox?  And this sandbox has Xrays to
> "window.opener"?  Or you're talking about running this on the web?

I was talking about the web.

> On the web, there is no issue at all with that code: If that call succeeds
> then there is no cross-compartment wrapper involved at all and hence no
> problem.  What would be more interesting is this (still on the web):
> 
>   window.addEventListener.call(window.opener, "hashchange",
> window.opener.onAuthHathChange);
> 
> which does store a cross-compartment wrapper in the event listener.  But we
> don't nuke web compartments anyway, right?

We do nuke web compartments, but we only nuke wrappers into that compartment
that come from compartments with the system principal. So web content isn't
affected. If we added a nuked flag to the compartment, and checked it here, then
it would probably have some visible effect on web content.

But I guess we could just add the flag to nuked sandboxes, rather than any
nuked compartment.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> But I guess we could just add the flag to nuked sandboxes, rather than any nuked compartment.

Oh, yes, very much so!

Comment 18

[Andrew McCreight [:mccr8]]

(In reply to Kris Maglione [:kmag] from comment #16)
> We do nuke web compartments, but we only nuke wrappers into that compartment
> that come from compartments with the system principal.

As a pedantic correction, we now also nuke wrappers that come from compartments with expanded principals (bug 1174950), to fix leaks where addon sandboxes hold onto content.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, so it sounds like the implementation plan is:

1)  Disable script on nuked sandbox compartments.  This, plus cutting wrappers
    in/out of the sandbox means that callbacks that are in the sandbox 
    compartment will not get invoked: they will either fail the script-enabled
    check or will be cut wrappers.
2)  Mark nuked sandbox compartments somehow (e.g. on the compartment private).
3)  In CallbackObject's tracing code, if the callback's compartment is a nuked sandbox,
    just DropJSObjects and null out mIncumbentGlobal (basically, do whatever unlink does).
    Need to make sure this is safe to do while being traced....

Comment 20

[Kris Maglione [:kmag]]

Sounds good to me.

Comment 21

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #19)
> 3)  In CallbackObject's tracing code, if the callback's compartment is a
>     nuked sandbox, just DropJSObjects and null out mIncumbentGlobal
>     (basically, do whatever unlink does).
>     Need to make sure this is safe to do while being traced....

It turns out that not only is it not safe to drop the references while being traced, but it's also not safe to even inspect the JS objects, since the tracing code gets called during compaction, while the objects are halfway through being relocated.

I moved this step to CanSkip, as Andrew suggested in comment 10 (which also has the benefit of making the objects skippable after they're nuked), but it turns out that dropping the references there is unsafe too. Deferring it in a runnable worked, but caused warnings during shutdown when it triggered extra CC rounds. So I wound up having to use a DeferredFinalizer instead, and call DropJSObjects from its destructor.

Comment 22

[Kris Maglione [:kmag]]

Attachment: Bug 1273251: Part 1 - Mark nuked sandboxes as nuked and non-scriptable.

Review commit: https://reviewboard.mozilla.org/r/93832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93832/

Comment 23

[Kris Maglione [:kmag]]

Attachment: Bug 1273251: Part 2 - Mark extension window compartments as nuked, and nuke all wrappers.

Review commit: https://reviewboard.mozilla.org/r/93834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93834/

Comment 24

[Kris Maglione [:kmag]]

Attachment: Bug 1273251: Part 3 - Allow CallbackObject to contain a null callable.

Review commit: https://reviewboard.mozilla.org/r/93836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93836/

Comment 25

[Kris Maglione [:kmag]]

Attachment: Bug 1273251: Part 4 - Drop CallbackObject's JS objects for nuked compartments during CC.

Review commit: https://reviewboard.mozilla.org/r/93838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93838/

Comment 26

[Kris Maglione [:kmag]]

Attachment: Bug 1273251: Part 5 - Add tests for event listeners in nuked sandboxes.

Review commit: https://reviewboard.mozilla.org/r/93840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93840/

Comment 27

[Kris Maglione [:kmag]]

Attachment: Bug 1273251: Part 6 - Test that extension window wrappers are nuked on close.

Review commit: https://reviewboard.mozilla.org/r/93842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93842/

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

I guess this is related to Bug 1277376

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Or not with that bug, but the patch there.

Comment 30

[Kris Maglione [:kmag]]

(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #29)
> Or not with that bug, but the patch there.

Yeah, there's definitely some overlap. This has some extra bits to prevent us from creating new wrappers for nuked sandbox compartments, and also immediately prevents listeners from firing in any nuked sandboxes or closed extension windows, but otherwise they solve a lot of the same problems.

Comment 31

[Andrew McCreight [:mccr8]]

Comment on attachment 8811967 [details]
Bug 1273251: Part 2 - Mark extension window compartments as nuked, and nuke all wrappers.

https://reviewboard.mozilla.org/r/93834/#review94320

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8811971 [details]
Bug 1273251: Part 6 - Test that extension window wrappers are nuked on close.

https://reviewboard.mozilla.org/r/93842/#review94334

::: js/xpconnect/tests/unit/test_nuke_webextension_wrappers.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

I think tests are now supposed to be public domain.

::: js/xpconnect/tests/unit/test_nuke_webextension_wrappers.js:22
(Diff revision 1)
> +  let oldAddonIdCallback = aps.setExtensionURIToAddonIdCallback(uri => uri.host);
> +  do_register_cleanup(() => {
> +    aps.setExtensionURIToAddonIdCallback(oldAddonIdCallback);
> +  });

Kinda weird to put this in a task. Maybe just at the top level?

Comment 33

[Bobby Holley (:bholley)]

Comment on attachment 8811966 [details]
Bug 1273251: Part 1 - Mark nuked sandboxes as nuked and non-scriptable.

https://reviewboard.mozilla.org/r/93832/#review94704

::: js/xpconnect/wrappers/WrapperFactory.cpp:449
(Diff revision 1)
> +    // If we've somehow gotten to this point after either the source or target
> +    // compartment has been nuked, then nuke the newly-created wrapper.
> +    if (CompartmentPrivate::Get(origin)->wasNuked ||
> +        CompartmentPrivate::Get(target)->wasNuked) {
> +        NS_WARNING("Trying to create a wrapper into or out of a nuked compartment");

This isn't really a nuked wrapper - a nuked wrapper is no wrapper at all, but rather a DeadObjectProxy, which allows the target to be GCed. This will not.

Do we really need this, or is it just belt-and-suspenders? I'm not necessarily opposed to it, but additional cases like this in the core wrapper logic have a way of piling up...

Comment 34

[Kris Maglione [:kmag]]

Comment on attachment 8811966 [details]
Bug 1273251: Part 1 - Mark nuked sandboxes as nuked and non-scriptable.

https://reviewboard.mozilla.org/r/93832/#review94704

> This isn't really a nuked wrapper - a nuked wrapper is no wrapper at all, but rather a DeadObjectProxy, which allows the target to be GCed. This will not.
> 
> Do we really need this, or is it just belt-and-suspenders? I'm not necessarily opposed to it, but additional cases like this in the core wrapper logic have a way of piling up...

I know, but returning an actual nuked wrapper from here seemed like a much bigger change than I wanted to make in this bug.

I think we do really need this, though, yes. Part of the original problem with event listeners being run after a sandbox has been nuked is that the code that runs from it is still able to create new wrappers after it's been nuked, which allows more code with external side-effects to run. Ideally, I'd like to have just returned false, but that caused failed assertions, or to return a nuked wrapper instead, but that seemed better for a follow-up bug.

Comment 35

[Bobby Holley (:bholley)]

Comment on attachment 8811966 [details]
Bug 1273251: Part 1 - Mark nuked sandboxes as nuked and non-scriptable.

https://reviewboard.mozilla.org/r/93832/#review94728

Ok, r=me with the comment updated.

We definitely don't want to introduce paths for JS_WrapObject to fail aside from OOM (since we call that during delicate times like mid-brain-transplant). But a mechanism for the wrap callback to return null and have the JS engine use a DeadObjectProxy instead could work. Agreed a followup is better.

Comment 36

[Andrew McCreight [:mccr8]]

Comment on attachment 8811969 [details]
Bug 1273251: Part 4 - Drop CallbackObject's JS objects for nuked compartments during CC.

https://reviewboard.mozilla.org/r/93838/#review94348

::: dom/bindings/CallbackObject.h:185
(Diff revision 1)
>    }
>  
> +  class JSObjectsDropper final : public nsISupports
> +  {
> +  public:
> +    explicit JSObjectsDropper(CallbackObject *aHolder)

* to the left

::: dom/bindings/CallbackObject.cpp:45
(Diff revision 1)
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(CallbackObject)
> +  // If our callback has been cleared, we can't be part of a garbage cycle.
> +  return !tmp->mCallback;
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END
> +
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(CallbackObject)

You should swap the CAN_SKIP and CAN_SKIP_IN_CC. CAN_SKIP is called at the start of a CC, before it runs, while IN_CC is called during the CC, and we generally want to avoid touching random GC/CC'd objects then.

::: dom/bindings/CallbackObject.cpp:59
(Diff revision 1)
> +  JSObject* callback = tmp->CallbackPreserveColor();
> +  if (MOZ_UNLIKELY(!callback)) {
> +    return true;
> +  }
> +  auto pvt = xpc::CompartmentPrivate::Get(callback);
> +  if (tmp->mIncumbentGlobal && MOZ_UNLIKELY(pvt && pvt->wasNuked)) {

I don't suppose it matter much, but the pvt shouldn't be in the MOZ_UNLIKELY as I think most compartments do have a private.

::: dom/bindings/CallbackObject.cpp:61
(Diff revision 1)
> +    return true;
> +  }
> +  auto pvt = xpc::CompartmentPrivate::Get(callback);
> +  if (tmp->mIncumbentGlobal && MOZ_UNLIKELY(pvt && pvt->wasNuked)) {
> +    // It's not safe to release our global reference or drop our JS objects at
> +    // this point, so defer their finalization until GC is finished.

Should this say "until CC is finished"? This is called during CC and not GC.

Comment 37

[Andrew McCreight [:mccr8]]

Comment on attachment 8811969 [details]
Bug 1273251: Part 4 - Drop CallbackObject's JS objects for nuked compartments during CC.

https://reviewboard.mozilla.org/r/93838/#review97520

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8811969 [details]
Bug 1273251: Part 4 - Drop CallbackObject's JS objects for nuked compartments during CC.

https://reviewboard.mozilla.org/r/93838/#review94348

> You should swap the CAN_SKIP and CAN_SKIP_IN_CC. CAN_SKIP is called at the start of a CC, before it runs, while IN_CC is called during the CC, and we generally want to avoid touching random GC/CC'd objects then.

I think there was a reason I want with CAN_SKIP_IN_CC rather than CAN_SKIP. But checking `aRemovingAllowed` would probably serve the same purpose.

> Should this say "until CC is finished"? This is called during CC and not GC.

Yeah, typo. My fingers tend to automatically type GC.

Comment 39

[Peter Van der Beken [:peterv]]

Comment on attachment 8811968 [details]
Bug 1273251: Part 3 - Allow CallbackObject to contain a null callable.

https://reviewboard.mozilla.org/r/93836/#review98012

This is a bit of a scary change. I looked at most of the callers to make sure they can deal with null, I hope I didn't miss anything (and that nobody adds new code that assumes non-null).

::: dom/bindings/Codegen.py:4161
(Diff revision 1)
>                  // XXXbz Wish we could check for a JS-implemented object
>                  // that already has a content reflection...
>                  if (!IsDOMObject(js::UncheckedUnwrap(${source}))) {
>                    nsCOMPtr<nsIGlobalObject> contentGlobal;
> -                  if (!GetContentGlobalForJSImplementedObject(cx, Callback(), getter_AddRefs(contentGlobal))) {
> +                  JS::Handle<JSObject*> callback = CallbackOrNull();
> +                  if (!(callback && GetContentGlobalForJSImplementedObject(cx, callback, getter_AddRefs(contentGlobal)))) {

I think I'd prefer:

if (!callback ||
    !GetContentGlobalForJSImplementedObject(cx, callback, getter_AddRefs(contentGlobal))) {

Comment 40

[Peter Van der Beken [:peterv]]

Comment on attachment 8811969 [details]
Bug 1273251: Part 4 - Drop CallbackObject's JS objects for nuked compartments during CC.

https://reviewboard.mozilla.org/r/93838/#review98014

I'll defer to mccr8 for the skip stuff, he knows more about that than I do.

::: dom/bindings/CallbackObject.h:182
(Diff revision 1)
>      JSObject* thisObj = js::UncheckedUnwrap(wrappedThis);
>      JSObject* otherObj = js::UncheckedUnwrap(wrappedOther);
>      return thisObj == otherObj;
>    }
>  
> +  class JSObjectsDropper final : public nsISupports

Couldn't this just be a plain non-refcounted class? I think you should be able to make that work with AddForDeferredFinalization.

Comment 41

[Kris Maglione [:kmag]]

Comment on attachment 8811968 [details]
Bug 1273251: Part 3 - Allow CallbackObject to contain a null callable.

https://reviewboard.mozilla.org/r/93836/#review98012

I agree that it's a bit scary, which is why I wound up changing the method name to CallbackOrNull. I didn't change the BindingUtils helper names (though I considered it), but from what I remember of the code that I looked at, they're only used in places where the callback is guaranteed to be non-null.

Either way, it seems like we're going to need this change, if not for this, than for bug 1277376, so I think it's just a matter of being as careful about it as possible.

> I think I'd prefer:
> 
> if (!callback ||
>     !GetContentGlobalForJSImplementedObject(cx, callback, getter_AddRefs(contentGlobal))) {

Yeah, I agree.

Comment 42

[Kris Maglione [:kmag]]

Comment on attachment 8811969 [details]
Bug 1273251: Part 4 - Drop CallbackObject's JS objects for nuked compartments during CC.

https://reviewboard.mozilla.org/r/93838/#review98014

> Couldn't this just be a plain non-refcounted class? I think you should be able to make that work with AddForDeferredFinalization.

It could. I'm not sure it would make much difference either way, given how infrequently we should need to do this, but I guess it may make the code a bit simpler.

Comment 43

[Peter Van der Beken [:peterv]]

Comment on attachment 8811970 [details]
Bug 1273251: Part 5 - Add tests for event listeners in nuked sandboxes.

https://reviewboard.mozilla.org/r/93840/#review103780

Comment 44

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89c459cabd70cb46e5fad838f6b65105f610e0d3
Bug 1273251: Part 1 - Mark nuked sandboxes as nuked and non-scriptable. r=bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/3782cf885346adb952de0f720bb48b7ea9557ed9
Bug 1273251: Part 2 - Mark extension window compartments as nuked, and nuke all wrappers. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d378d9b9a910f2aa6e3fea032c707dcdccecd91
Bug 1273251: Part 3 - Allow CallbackObject to contain a null callable. r=peterv

https://hg.mozilla.org/integration/mozilla-inbound/rev/27c422b6b825c6889e4ecb17737d5c16ef9fe859
Bug 1273251: Part 4 - Drop CallbackObject's JS objects for nuked compartments during CC. r=peterv,mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/d406d6d99be9294f04ee0f7090e8b93f6f882a96
Bug 1273251: Part 5 - Add tests for event listeners in nuked sandboxes. r=peterv

https://hg.mozilla.org/integration/mozilla-inbound/rev/00519f64027bb8dabf6da2e08c8956eee0fe1d20
Bug 1273251: Part 6 - Test that extension window wrappers are nuked on close. r=billm

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89c459cabd70
https://hg.mozilla.org/mozilla-central/rev/3782cf885346
https://hg.mozilla.org/mozilla-central/rev/5d378d9b9a91
https://hg.mozilla.org/mozilla-central/rev/27c422b6b825
https://hg.mozilla.org/mozilla-central/rev/d406d6d99be9
https://hg.mozilla.org/mozilla-central/rev/00519f64027b