Bug 137651 – WeakMap reference w/ DOM element as key does not survive long enough

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tim Guan-tin Chien [:timdream]]

STR:

1. Open the reduced test case, open up the Inspector
2. Click Run
3. Observer the console output

Expected:

1. console keep printing the object from our setInterval call

Actual:

1. After GC (?) the console start to porint out 'undefined'

Note:

Originally submitted as: https://github.com/timdream/gaia-keyboard-demo/issues/7

This is easier to reproduce on iOS 8 Simulator than on Safari 7.1, and harder on WebKit Nightly.

Does not affect previous iOS or Safari versions because WeakMap was not implemented.

[Tim Guan-tin Chien [:timdream]]

Created attachment 239710 [details]
Testcase

[Radar WebKit Bug Importer]

<rdar://problem/18645174>

[Geoffrey Garen]

In this test case, the JavaScript wrapper for the p element is garbage collected because there are no explicit references to it -- even though its underlying C++ DOM object is still present in the document.

I guess the behavior you expected was for the JavaScript wrapper's lifetime to be identical to its underlying DOM object, as observed by a WeakMap. 

Ironically, implementing that feature would require a WeakMap to strongly reference a DOM object.

I'm not sure what the correct behavior is in this case. Perhaps the optimization to throw away unobservable DOM wrappers is invalid in a world where WeakMap can observe the lifetime of any object.

[Tim Guan-tin Chien [:timdream]]

Thanks for the comment,

(In reply to comment #3)
> In this test case, the JavaScript wrapper for the p element is garbage collected because there are no explicit references to it -- even though its underlying C++ DOM object is still present in the document.
> 

Web Developers shouldn't be worry about how the DOM object is implemented right :) ?

> I guess the behavior you expected was for the JavaScript wrapper's lifetime to be identical to its underlying DOM object, as observed by a WeakMap. 
> 
> Ironically, implementing that feature would require a WeakMap to strongly reference a DOM object.
> 
> I'm not sure what the correct behavior is in this case. Perhaps the optimization to throw away unobservable DOM wrappers is invalid in a world where WeakMap can observe the lifetime of any object.

FWIW Gecko and Chrome does not fail when I run this test case.

[Alexey Proskuryakov]

I think that adding a custom property on each node in WeakMap may be a reasonable workaround. Geoff, does this have any serious downsides?

  while (i--) {
    document.body.appendChild(document.createElement('p'));
    document.body.lastElementChild.textContent = i;
    document.body.lastElementChild.webkitWeakMapWorkaround = 1;  // <-- This
    wmap.set(document.body.lastElementChild, { foo: 'bar', id: i });
  }

Perhaps we can use a similar mechanism to make WeakMap work with nodes. Possibly a side HashCountedSet, or even a counter in node rare data.

[Geoffrey Garen]

> I think that adding a custom property on each node in WeakMap may be a reasonable workaround. Geoff, does this have any serious downsides?
> 
>   while (i--) {
>     document.body.appendChild(document.createElement('p'));
>     document.body.lastElementChild.textContent = i;
>     document.body.lastElementChild.webkitWeakMapWorkaround = 1;  // <-- This
>     wmap.set(document.body.lastElementChild, { foo: 'bar', id: i });
>   }

I think that's a reasonable workaround.

> Perhaps we can use a similar mechanism to make WeakMap work with nodes. Possibly a side HashCountedSet, or even a counter in node rare data.

I think we could. Ultimately, though, it might just be time to retire the "unobservable wrapper" optimization. Now that WeakMap and WeakSet expose GC semantics, everything is logically observable.

[Alexey Proskuryakov]

> even a counter in node rare data

Geoff pointed out that this won't work, because we likely have the same issue with some non-node objects (such as probably CSS OM).

> Now that WeakMap and WeakSet expose GC semantics

I don't think that they do - there is no way to iterate them, so you always need a reachable object for lookup.

[Geoffrey Garen]

> > Now that WeakMap and WeakSet expose GC semantics
> 
> I don't think that they do - there is no way to iterate them, so you always need a reachable object for lookup.

What I mean by this is that the expose, in a programmer-visible way for the first-time, whether the engine decided to garbage collect a JavaScript wrapper that was not visible through any other API.

[Joseph Pecoraro]

I just ran into this with WeakSet. It was unexpected to me to see a node disappear from  a WeakSet. It is inconvenient and unexpected to have to work around this.

[Keith Miller]

Created attachment 253904 [details]
The patch

[Filip Pizlo]

Comment on attachment 253904 [details]
The patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253904&action=review

> Source/WebCore/bindings/js/JSNodeCustom.cpp:79
> -static inline bool isObservable(JSNode* jsNode, Node* node)
> -{
> -    // The root node keeps the tree intact.
> -    if (!node->parentNode())
> -        return true;
> -
> -    if (jsNode->hasCustomProperties())
> -        return true;
> -
> -    // A node's JS wrapper is responsible for marking its JS event listeners.
> -    if (node->hasEventListeners())
> -        return true;
> -
> -    return false;
> -}
> -
> -static inline bool isReachableFromDOM(JSNode* jsNode, Node* node, SlotVisitor& visitor)
> +static inline bool isReachableFromDOM(Node* node, SlotVisitor& visitor)

Hmmm, it seems like we should do some performance tests to make sure that this isn't too severe.

[Geoffrey Garen]

Comment on attachment 253904 [details]
The patch

Please add a comment in your ChangeLog to explain your performance testing methodology.

[Keith Miller]

Created attachment 253909 [details]
Patch 2

Updated the ChangeLog to reflect the performance testing strategy for this patch.

[Geoffrey Garen]

Comment on attachment 253909 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=253909&action=review

Looks like you have some accidental whitespace changes throughout the ChangeLog. Did you reflow the document?

> Source/WebCore/ChangeLog:11
> +        will be done after the code is rolled out. If major
> +        performance issues occur the patch will be rolled back.

We use "rolled out" to mean removing the patch. So, you are going to test by monitor bots after committing the change, and then you will roll out if there is a problem.

[Keith Miller]

Created attachment 253929 [details]
Patch Nomenclature

Undid the whitespace problem and changed the wording to be in line with the rest of the team.

[Geoffrey Garen]

Comment on attachment 253929 [details]
Patch Nomenclature

Oops! Didn't notice this before: Regression test needs expected results.

[Keith Miller]

Created attachment 253933 [details]
Patch Added Expected

Added the expected test results file.

[Keith Miller]

Created attachment 253934 [details]
Patch Expected File

[Geoffrey Garen]

Comment on attachment 253934 [details]
Patch Expected File

r=me

[WebKit Commit Bot]

Comment on attachment 253934 [details]
Patch Expected File

Clearing flags on attachment: 253934

Committed r185023: <http://trac.webkit.org/changeset/185023>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke bindings tests, which need to be updated. Is anyone around to do that now? I can only do it later in the evening.

[Joseph Pecoraro]

Bindings test updated: http://trac.webkit.org/changeset/185029