If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Event listeners/handlers should stop tracing their target if its compartment has been nuked

NEW
Unassigned

Status

()

Core
XPConnect
5 years ago
4 years ago

People

(Reporter: bholley, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

See the discussion in [1].

Basically, when we do Cu.nukeSandbox, we remove all the cross-compartment wrappers point into the sandbox, which generally allows the sandbox to go away. However, because we don't re-wrap event handlers/listeners into the same compartment as the target node (or at least won't bug 862629), we can still end up tracing the sandbox via the trace hook on the C++ EventListener object. This pegs the lifetime of the sandbox to the lifetime of the associated DOM.

I still think we shouldn't be rewrapping event listeners here. However, I have an alternative proposal to solve this issue:

1 - Add some state to the compartment private tracking whether the compartment has been nuked - that is to say, whether we want it to go away as soon as possible. This is necessary because nuking can happen between arbitrary source and destination compartments, and doesn't necessarily mean that the target needs to go away. We'd set this bit in Cu.nukeSandbox (and, later on, possibly in other places if we decide to expand hueyfix).

2 - Modify the trace hook of the C++ EventListener/EventHandler code to check for this bit on its underlying object, and if so, replace it with a DeadObjectProxy instead of tracing it.

Testing this should actually be pretty straightforward. We just have to set up the conditions for this to happen, and make sure that calling the event listener after the sandbox is nuked results in a DeadObjectProxy exception.

[1] https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.platform/PBwUUrXDQa8
Whiteboard: [MemShrink]

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]

Comment 1

4 years ago
Sounds bad if we need to have such special case for event listeners/handlers.
What about other callbacks?
(In reply to Olli Pettay [:smaug] from comment #1)
> Sounds bad if we need to have such special case for event listeners/handlers.
> What about other callbacks?

This logic would live in the generic Callback code, so I think it should be fine.
You need to log in before you can comment on or make changes to this bug.