Closed Bug 982561 Opened 6 years ago Closed 6 years ago

FindZoneGroups does not take into account edges from weak map key delegates

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32

People

(Reporter: manvel, Assigned: jonco)

References

()

Details

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.117 Safari/537.36

Steps to reproduce:

1. Press Shift-F4 to open Scratchpad.
2. Go to menu Environment and switch to Browser (devtools.chrome.enabled preference has to be set to true for that).
3. Paste the attached script.
4. Open Execute menu and press Run.
5. Wait 10 seconds for the script to finish, then close Scratchpad and open Browser Console.

Additional information:
There is an issue related to the bug opened in AdBlock Plus issue tracker ( https://issues.adblockplus.org/ticket/47 ) and seams that the issue is still prevent Addon developers of using WeakMap instead of setUserData.
The Scratchpad script and reproduction steps was modified by Wladimir Palant.


Actual results:

The console shows the entries "fillMap: test" and "checkMap: undefined" - all WeakMap disappeared after a second page was opened and closed.


Expected results:

The console shows the entries "fillMap: test" and "checkMap: test" - WeakMap data is present on both occasions.
Attachment #8389699 - Attachment mime type: application/javascript → text/plain
Some additional information:

The script to reproduce this issue will put 1000 elements as keys into a WeakMap, then open another tab and close it after a few seconds. This should have no effect on the WeakMap yet all data disappears (the script checks only the first element but they are really all gone). It seems that the wrappers used as keys have been garbage-collected even though the DOM nodes they refer to are very much alive. This is very similar to bug 819131 with the exception that triggering garbage collection manually doesn't seem to have any effect. The issue only got reproducible reliably by increasing the number of elements in the map and adjusting timing.

We've hit this issue intermittently when we replaced getUserData()/setUserData() with WeakMap in Adblock Plus and had to revert that change because of it. I reproduced it in the current Firefox nightly (30.0a1 2014-03-11) on OS X but it affects all Firefox versions and platforms.
Blocks: abp, 749981
Status: UNCONFIRMED → NEW
Component: Untriaged → XPConnect
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Summary: WeakMap data disappeared → WeakMap data disappears when using cross-compartment DOM elements as keys
Version: 30 Branch → Trunk
Assignee: nobody → continuation
Using .wrappedJSObject on the keys makes this work, so the problem is Xray-specific.  Presumably the Xray is being collected?
Summary: WeakMap data disappears when using cross-compartment DOM elements as keys → WeakMap data disappears when using Xrays as keys
Flags: needinfo?(continuation)
I haven't looked into this yet, but that sounds reasonable.  I guess PreserveWrapper in XPCJSRuntime isn't working correctly on Xrays.
Flags: needinfo?(continuation)
(In reply to Andrew McCreight [:mccr8] from comment #3)
> I haven't looked into this yet, but that sounds reasonable.  I guess
> PreserveWrapper in XPCJSRuntime isn't working correctly on Xrays.

Do we need to preserve the CCW itself, or just the underlying target? If it's the latter, we can do that pretty easily. Though in that case, it's not clear to me why Xrays are a special-case. Shouldn't we just be detecting cross-compartment wrappers in general?
Flags: needinfo?(continuation)
Blocks: WeakMap
Flags: needinfo?(continuation)
To recap IRC, it seems likely there is some issue around ext.weakmapKeyDelegateOp for Xrays, because they are a different class than CCWs.  But they are a subclass, so bholley doesn't know why it wouldn't be ok.  I'll have to poke around in a debugger or something and figure out why we're not preserving the Xray wrapper.
Got any updates?
This is creating very annoying results to me too... I can't use weakmap because of this and using getUserData() throws the warning.
Flags: needinfo?(continuation)
This bug is important for Adblock, I will try to fix it.
Thanks!  Sorry I haven't dealt with this yet.  Let me know if you have any questions.
Assignee: continuation → evilpies
Flags: needinfo?(continuation)
Tom looked at this a bit, but it seems tricky, so I'll take it back.
Assignee: evilpies → continuation
This fails to reproduce when I enable cycle collector logging, which is quite odd and will make this trickier to diagnose.
I think the problem is the zone group calculator for sweeping does not take into account any intrazone edges induced by weak map key delegates.

In the test case, the weak map key is a cross-compartment wrapper in the system zone.  The wrappee (its delegate) is the reflector for a div element in the content zone.  The browser begins an incremental GC, and I think it puts the system zone in a group that is separate from and swept earlier than the group of the content zone.  (The system zone has to be done earlier or at the same time as the content zone because the wrapper points to the reflector.)  When that runs, it examines the weak map, looks at the key (the wrapper), and checks if the underlying delegate (the reflector) is marked.  We haven't swept the content zone yet, so it isn't marked yet, and so we decide to not mark the key, which eventually causes the entry to be thrown out.  Then later we decide reflector is actually alive (because it is held by a gray root?), so we end up in the weird situation where the reflector survives the GC, but the wrapper and weak map entry do not.

Always calling |finder.useOneComponent()| in FindZoneGroups seems to fix the problem, which I think supports my theory.

In order to fix this, we need to find all of the cross-zone edges from delegates to weak map keys. Of course, a delegate does not know that it is a delegate, and we only want to look at each zone a linear number of times, so this requires an additional pass in FindZoneGroups that examines every unmarked weak map key in every compartment in every zone Z, to see if it has a delegate in some other zone Z'. If so, we add Z to the set of zones that Z' points to. Then, in Zone::findOutgoingEdges, we look up the current zone in the map we just created, and add all of the other zones there as outgoing edges.

One thing I'm a little concerned about is the interaction of weak map discovery with this marking that seems to happen very late in the process.  We only add things to the per-compartment weak map list once they are traced somewhere, so can we end up discovering new weak maps after we call FindZoneGroups?  That would reveal previously unknown interzone edges from some other zone Z'.  I suppose there are three cases for Z': it could be in an earlier group, in which case we don't have to do any additional work; it could be in the same group, in which case we're already okay; it could be in a later group.  If it is scheduled for a later group, we basically need to merge the later group (and possibly other groups scheduled in between our current group and the later group) into our current group.  That's going to be annoying, but maybe it is okay.  It depends on how delicate the ordering of the subphases is in sweeping.

The good news is that I think the only bad effect is going to be that we'll throw away weak map entries that should be alive.
Component: XPConnect → JavaScript: GC
Summary: WeakMap data disappears when using Xrays as keys → FindZoneGroups does not take into account edges from weak map key delegates
(In reply to Andrew McCreight [:mccr8] from comment #11)
I think you're right.  We don't consider these edges at all when computing the zone graph.

Your proposed solution sounds like it will work, but I'm wondering if there is a simpler way.  For example, could we keep a map containing information about these additional edges on the compartment, updating it when we add an entry to a weak map?  We do something similar already for DebuggerWeakMap.
Yeah, that sounds reasonable, too.  Would you mind looking into this?  I have a bunch of other stuff I need to deal with.
This is a modified version of the script that works a little better for me.  Thanks a lot for the test case, it would have been impossible to identify the problem without it!

The steps for this version are similar to the other one.
1. Make sure that devtools.chrome.enabled is set to true.
2. Open Scratchpad.
3. Set Environment to Browser.
4. Click on "Open File..." on Scratchpad, and select the script file.
5. Click on "Run" on Scratchpad.
6. The test case will open two tabs. When they have both been closed, then the script has finished running, and you can look at the browser console.

The weak map in question will have a property "tagtag" set which helps for locating it in GC logs.

There's a timeFactor variable in the script that affects the delay between the various steps.  Increasing it may be needed if you add more logging that slows down execution.  Of course, now that we know what the problem is, it should be possible to create a shell or browser test case that does not need setTimeout.
Attachment #8389699 - Attachment is obsolete: true
Flags: needinfo?(jcoppeard)
Assignee: continuation → jcoppeard
Flags: needinfo?(jcoppeard)
This patch changes JSCompartment::gcWeakMapList to be list of all weakmaps in that compartment, not just those that were found to be live at the last GC.
Attachment #8423167 - Flags: review?(terrence)
This patch does the work of ensuring that zones containing key delegates finish marking before those containing the associated weak maps.

I originally tried to cache a count of of these edges so that we could use this without iterating all weak maps when we start sweeping.  However this was frustrated by the fact a key's delegate can disappear while it is still alive, which meant that my bookkeeping didn't add up.  This happens when proxies are used as keys: the delegate is the target of the proxy and this can be removed by nuking the proxy.

Instead this iterates all weak map entries for collecting compartments when we come to calculate zone groups.  This is annoying, but we already have to iterate weak maps (possibly repeatedly) when we come to mark them anyway, so it's not dramatically worse than the current situation.
Attachment #8423171 - Flags: review?(terrence)
Attached patch 3 - test codeSplinter Review
This adds debug-only way to get the index of the zone group in the last GC from a zone, which  as the best way I could come up with to verify that this is working correctly.
Attachment #8423172 - Flags: review?(terrence)
Comment on attachment 8423167 [details] [diff] [review]
1 - maintain per compartment list of all weakmaps

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

r=me  That makes a ton more sense to me than what we were doing before.
Attachment #8423167 - Flags: review?(terrence) → review+
Comment on attachment 8423171 [details] [diff] [review]
2 - take weakmaps into account when calcuating zone groups

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

Very nice! r=me
Attachment #8423171 - Flags: review?(terrence) → review+
Comment on attachment 8423172 [details] [diff] [review]
3 - test code

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

\o/ Brilliant test. r=me

::: js/src/gc/Zone.h
@@ +147,5 @@
>      bool                         gcScheduled;
>      GCState                      gcState;
>      bool                         gcPreserveCode;
> +#ifdef DEBUG
> +    unsigned                     gcLastZoneGroupIndex;

Given that just above and below we have a bool followed by a larger allocation, we're probably not too concerned about the size of the Zone struct :-/. Until we fix the larger packing issues, let's make this DebugOnly<unsigned> and get rid of most of the #ifdef ugliness.

::: js/src/jsgc.cpp
@@ +3811,5 @@
>              rt->sweepZoneCallback(zone);
> +
> +#ifdef DEBUG
> +        zone->gcLastZoneGroupIndex = zoneGroupIndex;
> +#endif

And here.
Attachment #8423172 - Flags: review?(terrence) → review+
Wladimir, could you verify that the ADP issue was fixed?
Flags: needinfo?(trev.moz)
Manvel Saroyan tested this on our end and confirms that the issue is gone in the current nightly.
Status: RESOLVED → VERIFIED
Flags: needinfo?(trev.moz)
For reference, we are now using weak maps in our development builds again: https://hg.adblockplus.org/adblockplus/rev/05d8110fe5ea
Depends on: CVE-2017-5410
Depends on: 1338383
You need to log in before you can comment on or make changes to this bug.