Last Comment Bug 680937 - don't leak wrapped native WeakMap keys
: don't leak wrapped native WeakMap keys
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on: 655297 668855 711616
Blocks: WeakMap 669240 706281
  Show dependency treegraph
 
Reported: 2011-08-22 09:40 PDT by Andrew McCreight [:mccr8]
Modified: 2012-05-27 05:39 PDT (History)
7 users (show)
continuation: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't mark wrapped native weak map keys during black marking (2.24 KB, patch)
2011-09-04 09:33 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
use wrapper preservation, not marking, to keep wrapped native weak map keys alive, WIP (12.12 KB, patch)
2011-12-07 15:50 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 1: don't mark wrapped native keys during a GC (3.05 KB, patch)
2011-12-12 16:52 PST, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Splinter Review
part 2: add wrapper preserve hook, invoke it when adding weak map binding (7.41 KB, patch)
2011-12-12 16:56 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 3: implement wrapper preservation callback, register it (2.96 KB, patch)
2011-12-12 16:58 PST, Andrew McCreight [:mccr8]
jst: review+
Details | Diff | Splinter Review
part 2: add wrapper preserve hook, invoke it when adding weak map binding (7.42 KB, patch)
2011-12-13 09:24 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 2: add wrapper preserve hook, invoke it when adding weak map binding (8.08 KB, patch)
2011-12-14 14:17 PST, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2011-08-22 09:40:36 PDT
Marking them black causes leaks.
Comment 1 Andrew McCreight [:mccr8] 2011-09-04 09:33:37 PDT
Created attachment 558159 [details] [diff] [review]
Don't mark wrapped native weak map keys during black marking
Comment 2 Andrew McCreight [:mccr8] 2011-09-15 07:30:47 PDT
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] 2011-12-02 11:44:55 PST
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] 2011-12-07 15:50:34 PST
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.
Comment 5 Andrew McCreight [:mccr8] 2011-12-12 16:52:42 PST
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.
Comment 6 Andrew McCreight [:mccr8] 2011-12-12 16:56:22 PST
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.
Comment 7 Andrew McCreight [:mccr8] 2011-12-12 16:58:01 PST
Created attachment 581110 [details] [diff] [review]
part 3: implement wrapper preservation callback, register it
Comment 8 Andrew McCreight [:mccr8] 2011-12-12 17:02:17 PST
pushed to try https://tbpl.mozilla.org/?tree=Try&rev=c5e90d9bdeee
Comment 9 Andrew McCreight [:mccr8] 2011-12-12 17:11:44 PST
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 (⌚ UTC+1/+2) 2011-12-12 23:24:16 PST
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] 2011-12-13 09:23:35 PST
(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] 2011-12-13 09:24:48 PST
Created attachment 581298 [details] [diff] [review]
part 2: add wrapper preserve hook, invoke it when adding weak map binding
Comment 13 Andrew McCreight [:mccr8] 2011-12-14 09:31:10 PST
The try run looked good.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2011-12-14 13:38:15 PST
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] 2011-12-14 14:17:56 PST
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.
Comment 16 Bill McCloskey (:billm) 2011-12-14 15:57:45 PST
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] 2011-12-14 16:12:44 PST
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 19 Andrew McCreight [:mccr8] 2011-12-15 18:04:53 PST
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 21 Wladimir Palant 2012-05-27 05:19:45 PDT
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 2012-05-27 05:39:30 PDT
Never mind, things seem to get garbage collected regardless of whether I use XPCNativeWrapper.unwrap() - I guess that I am observing bug 673468 then.

Note You need to log in before you can comment on or make changes to this bug.