Fixing another case of multiple upgrades of a custom element

Fixing another case of multiple upgrades of a custom element

The issue here is that when we upgrade a custom element that we create a second wrapper, but when the first wrapper gets GC'd it's clearing out the lookup tables for the new wrapper. The fix is to not clear out the tables if it's no longer associated with the element.

The test for this is in https://codereview.chromium.org/55153002/

BUG=14483
R=asiva@google.com, vsm@google.com

Committed: https://src.chromium.org/viewvc/multivm?view=rev&revision=1514

[blois, 2013-10-31 18:43:07]

https://codereview.chromium.org/54833003/diff/30001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://codereview.chromium.org/54833003/diff/30001/Source/bindings/dart/Dart...
Source/bindings/dart/DartDOMWrapper.h:282: if
(!Dart_IdentityEquals(Dart_HandleFromWeakPersistent(currentWrapper),
Dart_HandleFromWeakPersistent(wrapper))) {
Changing the check to:
if (currentWrapper != wrapper) {}

Appears to fix the issue as well. Not sure which one exactly is preferred here.

[vsm, 2013-10-31 19:28:49]

lgtm - siva: can you comment on the better pattern for comparing weak persistent
handles?

[siva, 2013-10-31 19:42:40]

lgtm

https://codereview.chromium.org/54833003/diff/30001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://codereview.chromium.org/54833003/diff/30001/Source/bindings/dart/Dart...
Source/bindings/dart/DartDOMWrapper.h:282: if
(!Dart_IdentityEquals(Dart_HandleFromWeakPersistent(currentWrapper),
Dart_HandleFromWeakPersistent(wrapper))) {
On 2013/10/31 18:43:07, blois wrote:
> Changing the check to:
> if (currentWrapper != wrapper) {}
> 
> Appears to fix the issue as well. Not sure which one exactly is preferred
here.

if (currentWrapper != wrapper) {} is preferable for efficiency purposes. You
could write it as:

if (currentWrapper != wrapper) {
  ASSERT(!Dart_IdentityEquals(Dart_HandleFromWeakPersistent......);
  return;
}


I am assuming that currentWrapper was also created in the same path using
associateWrapper and it is a PrologueWeakPersistentHandle

[blois, 2013-10-31 20:24:17]

https://codereview.chromium.org/54833003/diff/30001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://codereview.chromium.org/54833003/diff/30001/Source/bindings/dart/Dart...
Source/bindings/dart/DartDOMWrapper.h:282: if
(!Dart_IdentityEquals(Dart_HandleFromWeakPersistent(currentWrapper),
Dart_HandleFromWeakPersistent(wrapper))) {
On 2013/10/31 19:42:40, siva wrote:
> On 2013/10/31 18:43:07, blois wrote:
> > Changing the check to:
> > if (currentWrapper != wrapper) {}
> > 
> > Appears to fix the issue as well. Not sure which one exactly is preferred
> here.
> 
> if (currentWrapper != wrapper) {} is preferable for efficiency purposes. You
> could write it as:
> 
> if (currentWrapper != wrapper) {
>   ASSERT(!Dart_IdentityEquals(Dart_HandleFromWeakPersistent......);
>   return;
> }
> 
> 
> I am assuming that currentWrapper was also created in the same path using
> associateWrapper and it is a PrologueWeakPersistentHandle

Done.

[blois, 2013-10-31 20:38:36]

Committed patchset #2 manually as r1514 (presubmit successful).

[vsm, 2013-10-31 23:19:43]

https://codereview.chromium.org/54833003/diff/70001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://codereview.chromium.org/54833003/diff/70001/Source/bindings/dart/Dart...
Source/bindings/dart/DartDOMWrapper.h:280: Dart_WeakPersistentHandle
currentWrapper = Traits::MapTraits::domMap(domData)->get(domObject);
Looking the IDB failures - if they are from this - you'll need a
DartIsolate/ApiScope:

../../dart/runtime/vm/handles_impl.h:99: error: expected:
isolate->no_handle_scope_depth() == 0