1642974 - WeakRef should not expose DOM wrappers whose target has been cycle collected

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Jon Coppeard (:jonco)]

Using WeakRef, it's possible to get hold of a DOM wrapper whose target has been cycle collected, and which would otherwise be garbage collected. This can be demonstrated with the following code which causes a crash (this requires weak refs to be enabled to work):

// createHTMLDocument() create a document which has couple of elements in it, forming a cycle.
var doc = document.implementation.createHTMLDocument();
doc.expando = true;
var w = new WeakRef(doc);
doc = null;

// Run GC and CC (but not further GC)

w.deref(); // crashes

Comment 1

[Andrew McCreight [:mccr8]]

Maybe NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER could call some kind of drop weak ref method? That seems cheaper than doing something like checking if the object you are handing out is a wrapper for an unlinked object. We do that in unlinking for various other weak references.

Comment 2

[Andrew McCreight [:mccr8]]

I'm not sure how to deal with the indirect case. eg where you have a weak ref to a plain JS object x which becomes garbage, and which has a reference to a DOM wrapper. Right now, we don't do anything for regular JS objects in unlinking.

Comment 3

[Andrew McCreight [:mccr8]]

"which becomes garbage" should be "gets identified as garbage by the CC", not actually collected.

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #1)

Maybe NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER could call some kind of drop weak ref method?

The difficulty with that is that there's no trivial way to check whether something is the target of a WeakRef. We'd have to pass it to the JS engine and look it up in a table.

Checking the object you're handing out in deref() seems easier, if there's a good way to check whether something is a wrapper for an unlinked object. It looks like you could check whether the wrapper is still preserved (since it gets released by unlinking), but I can't see how to do this for WebIDL classes.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1642974 - Don't expose WeakRef targets which are DOM wrappers whose target has been collected r=smaug!,sfink!

Requesting feedback for this approach.

I can fix this instance by checking whether the wrapper has been released, which happens in unlink. Does that sound like a reasonable approach?

I'm not sure how to do this for WebIDL wrappers though as there doesn't seem to be an easy way to get a pointer to the wrapper cache. Would I need to add some kind of hook for these?

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Why the check for preserved wrapper?
What about the testcase

let doc = document.implementation.createHTMLDocument();
doc.body.innerHTML = "<div></div>";
let wr = new WeakRef(doc.body.lastChild);

There is still a strong ref to doc in JS, so also lastChild stays alive, but its wrapper might die.

Or is there some code which forces wrapper preservation with WeakRefs (I thought there isn't).

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

I think there should be spare bits in nsWrapperCache object (mFlags), so if needed, it could tell whether it has been unlinked.
And if wrapper was preserved, unlinking should happen always.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Olli Pettay [:smaug] from comment #6)

Or is there some code which forces wrapper preservation with WeakRefs (I thought there isn't).

Yes, I added this after our conversation yesterday in bug 1642685. This is the same as happens currently for weak keys and fixes the other problem you found ;)

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

peterv and edgar might have some feedback too.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bff5c47fa32
Don't expose WeakRef targets which are DOM wrappers whose target has been collected r=smaug,sfink

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/7bff5c47fa32