WeakMap drops some entries |
||||||
Issue description
Chrome Version: 65.0.3325.181 (Official Build) (64-bit)
OS: Linux
What steps will reproduce the problem?
Load the following page in Chrome:
<html>
<head>
<style>
.doesntexit { display: none }
</style>
<script>
const map = new WeakMap();
map.set(document.styleSheets[0].cssRules[0], 'foo');
setInterval(() =>
console.log(map.get(document.styleSheets[0].cssRules[0])),
1000);
</script>
<body>
</body>
</html>
What is the expected result?
'foo' is printed every second (forever)
What happens instead?
'foo' is printed ~8 times. Then 'undefined' is printed forever.
Note that I'm not 100% sure if this is a bug or an odd behavior from the spec. The spec is rather concise on WeakMap (https://www.ecma-international.org/ecma-262/6.0/#sec-weakmap-objects):
"""
If an object that is being used as the key of a WeakMap key/value pair is only reachable by following a chain of references that start within that WeakMap, then that key/value pair is inaccessible and is automatically removed from the WeakMap. WeakMap implementations must detect and remove such key/value pairs and any associated resources.
"""
It's not entirely clear to me how that interacts with native objects (especially ones that expose their internal data through getters). However, in this particular case, when doing (note the additional reference to the CSSRulesList):
const map = new WeakMap();
const rules = document.styleSheets[0].cssRules;
map.set(document.styleSheets[0].cssRules[0], 'foo');
setInterval(() => console.log(map.get(document.styleSheets[0].cssRules[0])), 1000);
The WeakMap entry is still dropped. '0' is a value (i.e. non-getter) property of the CSSRulesList, so I don't see why the WeakMap entry should be dropped.
,
May 14 2018
The initial repro prints 'foo' forever in both Firefox and Safari, even after explicitly collecting garbage. I think this is a V8 bug. mlippautz@ can you please take a look? Note: For the second repro, it just follows from the first repro that the entry is dropped. You don't actually set the key in the map using this indirection. `rules` can be GC'ed as there are no references to it, which then makes this the same as the first repro.
,
May 14 2018
Thanks for having a look gsathya@ > `rules` can be GC'ed as there are no references to it, which then makes this the same as the first repro. I'm surprised V8 is smart enough to do this. I would expect `rules` to be in the scope record for the anonymous function and V8 not to GC since presumably the anonymous function could just call `eval` to access `rules`. Obviously this isn't happening here, but I didn't think V8 would be able to do this. Regardless, the second example isn't important if you believe the first one really is a bug.
,
May 15 2018
Looks like the wrapper for document.styleSheets[0].cssRules[0] is dropped and consequently 'foo' is also dropped from the WeakMap. This should not happen. Will take a loop.
,
May 15 2018
Minimal repro that requires
--js-flags="--expose-natives-syntax --expose-gc"
<html>
<head>
<style>
.doesntexit { display: none }
</style>
<script>
document.styleSheets[0].cssRules[0].prop = 'asdf';
function log_it() {
%DebugPrint('start');
console.log(document.styleSheets[0].cssRules[0].prop);
%DebugPrint('end');
}
log_it();
gc();
log_it();
</script>
<body>
</body>
</html>
We drop the JS object and get a new one on the second invocation of 'log_it()'.
It looks to me like blink::CSSStyleSheet [1] lacks the wrapper tracing annotations.
The reason this is also observable through WeakMap is that WeakMap adds an internal property for hashing and that one is gone (different) when we do the access after GC.
[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/css_style_sheet.h?l=49
,
May 15 2018
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/44d897b0a752f08649e686fc6106e6130a6beca2 commit 44d897b0a752f08649e686fc6106e6130a6beca2 Author: Michael Lippautz <mlippautz@chromium.org> Date: Tue May 15 16:38:11 2018 [wrapper-tracing] Properly trace rule lists of StyleSheet Otherwise the JS wrappers will get lost. Bug: chromium:830910 Change-Id: I7f746698b6ce05d9b2e3ce26f778a2e08d0c3cd6 Reviewed-on: https://chromium-review.googlesource.com/1059251 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Cr-Commit-Position: refs/heads/master@{#558742} [modify] https://crrev.com/44d897b0a752f08649e686fc6106e6130a6beca2/third_party/WebKit/LayoutTests/fast/dom/StyleSheet/gc-rule-children-wrappers-expected.txt [modify] https://crrev.com/44d897b0a752f08649e686fc6106e6130a6beca2/third_party/WebKit/LayoutTests/fast/dom/gc-9-expected.txt [add] https://crrev.com/44d897b0a752f08649e686fc6106e6130a6beca2/third_party/WebKit/LayoutTests/fast/dom/gc-stylesheet-rule-property.html [modify] https://crrev.com/44d897b0a752f08649e686fc6106e6130a6beca2/third_party/blink/renderer/core/css/css_style_sheet.cc [modify] https://crrev.com/44d897b0a752f08649e686fc6106e6130a6beca2/third_party/blink/renderer/core/css/css_style_sheet.h
,
May 16 2018
We actually did have layout test coverage for the DOM path used in the repro and it was known that properties were dropped on GC. All such issues will be fixed with the unfified heap that's under development. For now, the specific issue here and quite a few related ones are fixed.
,
May 16 2018
Thanks mlippautz@. Is there a bug I can follow for the unified heap? We are traversing the whole window object and storing all values we encounter in a WeakMap. So any leftover issue will still likely be a problem for us (this results in hard-to-debug failures in our project). For now we're simply using a regular Map. It's not memory efficient, but it's an acceptable workaround. We would like to remove the workaround once it's no longer needed though.
,
May 17 2018
> Thanks mlippautz@. Is there a bug I can follow for the unified heap? We are landing individual pieces right now. I created issue 843903 to track the overall progress; still need to block on the sub projects. Expect something in the range of months. > We are traversing the whole window object and storing all values we encounter in a WeakMap. So any leftover issue will still likely be a problem for us (this results in hard-to-debug failures in our project). This is one of the reasons we are unifying this. Keeping code correct across components using separate garbage collections is otherwise error prone. That said, while we know of corner cases (or stuff that was just never cleaned up like this one which was hard to get right in the old architecture), in general it should work and it would be awesome if you could report back on things that are broken. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by hablich@chromium.org
, Apr 19 2018Owner: gsat...@chromium.org
Status: Assigned (was: Untriaged)