don't leak wrapped native WeakMap keys

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Marking them black causes leaks.
(Assignee)

Updated

6 years ago
Summary: only marked XPCWrappedNative WeakMap keys during grey marking → only mark XPCWrappedNative WeakMap keys during grey marking

Updated

6 years ago
Blocks: 669240
(Assignee)

Comment 1

6 years ago
Created attachment 558159 [details] [diff] [review]
Don't mark wrapped native weak map keys during black marking
Assignee: general → continuation
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
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.
Summary: only mark XPCWrappedNative WeakMap keys during grey marking → don't leak XPCWrappedNative WeakMap keys
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 579879 [details] [diff] [review]
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.
Attachment #558159 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 547941
(Assignee)

Updated

6 years ago
Summary: don't leak XPCWrappedNative WeakMap keys → don't leak wrapped native WeakMap keys
(Assignee)

Updated

6 years ago
Blocks: 706281
(Assignee)

Comment 5

6 years ago
Created attachment 581103 [details] [diff] [review]
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.
Attachment #579879 - Attachment is obsolete: true
Attachment #581103 - Flags: review?(wmccloskey)
(Assignee)

Comment 6

6 years ago
Created attachment 581107 [details] [diff] [review]
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.
(Assignee)

Comment 7

6 years ago
Created attachment 581110 [details] [diff] [review]
part 3: implement wrapper preservation callback, register it
(Assignee)

Comment 8

6 years ago
pushed to try https://tbpl.mozilla.org/?tree=Try&rev=c5e90d9bdeee
(Assignee)

Comment 9

6 years ago
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 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).
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 581298 [details] [diff] [review]
part 2: add wrapper preserve hook, invoke it when adding weak map binding
Attachment #581107 - Attachment is obsolete: true
Attachment #581103 - Flags: review?(wmccloskey) → review+
(Assignee)

Updated

5 years ago
Attachment #581110 - Flags: review?(jst)
(Assignee)

Updated

5 years ago
Attachment #581298 - Flags: review?(wmccloskey)
(Assignee)

Comment 13

5 years ago
The try run looked good.

Updated

5 years ago
Attachment #581110 - Flags: review?(jst) → review+
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.
(Assignee)

Comment 15

5 years ago
Created attachment 581786 [details] [diff] [review]
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.
Attachment #581298 - Attachment is obsolete: true
Attachment #581298 - Flags: review?(wmccloskey)
Attachment #581786 - Flags: review?(wmccloskey)
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.
Attachment #581786 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Comment 18

5 years ago
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
Target Milestone: --- → mozilla11
(Assignee)

Comment 19

5 years ago
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).
https://hg.mozilla.org/mozilla-central/rev/b9bd2420e111
https://hg.mozilla.org/mozilla-central/rev/9a966139c3fe
https://hg.mozilla.org/mozilla-central/rev/21e2e05a8d8e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 711616
(Assignee)

Updated

5 years ago
Flags: in-testsuite+

Comment 21

5 years ago
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

5 years ago
Never mind, things seem to get garbage collected regardless of whether I use XPCNativeWrapper.unwrap() - I guess that I am observing bug 673468 then.
You need to log in before you can comment on or make changes to this bug.