680937 - don't leak wrapped native WeakMap keys

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

Marking them black causes leaks.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Don't mark wrapped native weak map keys during black marking

Comment 2

[Andrew McCreight [:mccr8]]

Peter pointed out that if the GC marks keys gray instead of black, they will end up being freed by the CC, causing the original problem of XWN keys disappearing to recur.  We must still mark them gray instead of black to allow the CC to collect the XWN, but we must do additional work to keep the CC from doing its wrapper optimization for WWN keys.

He suggested using an existing preserve-wrapper interface, which would add an edge from the JS Object to its corresponding native.  This cycle would ensure that the wrapper will persist until the native is no longer live.

This is probably not an ideal solution, as it would keep alive the wrapper even if it is removed from the WeakMap.  I think we can hack something into the CC WM scanning to deal with this, without keeping them alive later.

Comment 3

[Andrew McCreight [:mccr8]]

This is trickier than I thought it would be.  When the CC identifies the wrappee of a wrapped native as garbage, it unlinks it, but there's still a pointer from its wrapper, so it doesn't die.  FYI, this means that everything can be double unlinked.  All the unlink of the wrapper does is expire the wrapper (because the wrapper needs the wrappee to tear down various structures).  Then when the next GC comes around, the wrapper won't be reachable, and the GC collects it.

But if we always mark XPCWN WeakMap keys, no matter the color, they will never be collected.  I'm not sure how to fix this.  Preserving all wrappers that are keys might work, but I was envisioning doing that in a CC, which is too late if we want it to survive a GC.  But if we do it in the GC, I'm not sure how we can distinguish a wrapped native that is new from one that has been "unpreserved" by the CC.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: use wrapper preservation, not marking, to keep wrapped native weak map keys alive, WIP

This patch removes the wrapped native key marking introduced in bug 655297.  Instead, when we store a new wrapped native key into a weak map, we wrapper-preserve it.  This will ensure that the GC will free the key only after the CC has decided it is dead.  We avoid leaking keys by never marking them, and relying on the existing wrapper preservation mechanism.

I removed the old simple wrapped native key leak test and replaced it with a loopy one, as that is the existing test that seems to catch the most problems.  I also added a new test that ensures that the a live wrapped native key won't be removed, and confirmed that without the wrapper preservation that the key will be removed.  I also manually confirmed by looking at the CC logs that it is actually the CC that is freeing the garbage loop, as expected.

This patch passes the tests, but the actual implementation is a little gross.  We have to do wrapper preservation in the JS engine, in the implementation of WeakMap_set, and I'm not aware of any existing way to do that, so I had to add a new callback hook for wrapper preservation.  I also don't really know how to do QI AddRef stuff properly, so the way I QI to nsWrapperCache in PreserveWrappee is probably horribly wrong.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1: don't mark wrapped native keys during a GC

This just undos the wrapped native key marking so we don't leak.  It requires later patches to ensure we don't revert bug 655297.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2: add wrapper preserve hook, invoke it when adding weak map binding

Also includes new tests.  One checks that a garbage loop involving WN keys will get collected, the other ensures that wrapped native keys don't disappear.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 3: implement wrapper preservation callback, register it

Comment 8

[Andrew McCreight [:mccr8]]

pushed to try https://tbpl.mozilla.org/?tree=Try&rev=c5e90d9bdeee

Comment 9

[Andrew McCreight [:mccr8]]

Part 1 is pretty mechanical and shouldn't need to change, so I just put it up for review in case you have some spare cycles, Bill.

Part 2 and 3 seem okay, but I want to see how they do on try.  I also need to figure out who should review.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 581107 [details] [diff] [review]
part 2: add wrapper preserve hook, invoke it when adding weak map binding

>--- a/js/src/jsfriendapi.h
>+++ b/js/src/jsfriendapi.h
>+JS_FRIEND_API(void)
>+SetPreserveWrapperCallback(JSRuntime *rt, js::PreserveWrapperCallback callback);

No need for the js:: here (not that it's left off consistently).

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Ms2ger from comment #10)
> Comment on attachment 581107 [details] [diff] [review]
> No need for the js:: here (not that it's left off consistently).
Thanks, I've changed that in my local copy of the patch.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 2: add wrapper preserve hook, invoke it when adding weak map binding

Comment 13

[Andrew McCreight [:mccr8]]

The try run looked good.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 581298 [details] [diff] [review]
part 2: add wrapper preserve hook, invoke it when adding weak map binding

Here's a couple of drive-by comments:

+    js::PreserveWrapperCallback preserveWrapperCallback;

Looks like this new member isn't initialized in the JSRuntime constructor (Luke tells me this didn't use to be necessary since we used to memset JSRuntimes on construction, but no longer do that).

- In WeakMap_set():

+    if (key->getClass()->ext.isWrappedNative) {
+        if (!cx->runtime->preserveWrapperCallback(cx, key))

This should probably null-check preserveWrapperCallback before calling it. Might not matter for us, but other embedders could crash here w/o the check.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 2: add wrapper preserve hook, invoke it when adding weak map binding

Addressed jst's comments: initialized hook to null, null check before callback.

Comment 16

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

Comment on attachment 581786 [details] [diff] [review]
part 2: add wrapper preserve hook, invoke it when adding weak map binding

Review of attachment 581786 [details] [diff] [review]:
-----------------------------------------------------------------

Cool test! Please test the ReportWarning thing.

::: js/xpconnect/tests/chrome/test_weakmaps.xul
@@ +189,5 @@
> +  let wn_chained_garbage_maps = function () {
> +    let dom0 = document.createElement("div");
> +    let dom = dom0;
> +    for(let i = 0; i < num_chains; i++) {
> +    let new_dom = document.createElement("div");

I'm not familiar with this code, but the indentation looks off to me.

Comment 17

[Andrew McCreight [:mccr8]]

Thanks for the quick review!

(In reply to Bill McCloskey (:billm) from comment #16)
> Cool test! Please test the ReportWarning thing.

Will do.

> I'm not familiar with this code, but the indentation looks off to me.

Ah, yes, that is bad.  I need to set up Emacs with a less terrible mode for XUL.

Comment 18

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bd2420e111
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a966139c3fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/21e2e05a8d8e

Comment 19

[Andrew McCreight [:mccr8]]

I tested the report warning thing with window.navigator (which is a wrapped native, but not a cache wrapper), a DOM node, and a JS key, and they all worked as expected (warning in the first case, none in the latter two).

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b9bd2420e111
https://hg.mozilla.org/mozilla-central/rev/9a966139c3fe
https://hg.mozilla.org/mozilla-central/rev/21e2e05a8d8e

Comment 21

[Wladimir Palant]

Did this end up regressing bug 655297? I see most of my WeakMap entries being garbage collected despite of the nodes that I use as keys being very alive (merely the wrappers are unused). This is reproducible in Gecko 12.0 but seems to be gone in Gecko 15.0a1.

Comment 22

[Wladimir Palant]

Never mind, things seem to get garbage collected regardless of whether I use XPCNativeWrapper.unwrap() - I guess that I am observing bug 673468 then.