Bug 51683 – [v8] Minor cleanup: make 2nd argument of removeIfPresnt accept only a value type stored in the DOM map

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[anton muhin]

[v8] Minor cleanup: make 2nd argument of removeIfPresnt accept only a value type stored in the DOM map

[anton muhin]

Created attachment 77567 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77567 [details]
Patch

How do we test this?  Please explain what tests this affects in your ChagneLog or explain why testing is impossible.

[anton muhin]

Eric, that should be covered by the current test.

The change is mostly syntactic, all it requires an additional cast which is noop in Release mode.

(In reply to comment #2)
> (From update of attachment 77567 [details])
> How do we test this?  Please explain what tests this affects in your ChagneLog or explain why testing is impossible.

[anton muhin]

And just in case: this path is exercised by virtually any web page if it only touches a DOM

(In reply to comment #3)
> Eric, that should be covered by the current test.
> 
> The change is mostly syntactic, all it requires an additional cast which is noop in Release mode.
> 
> (In reply to comment #2)
> > (From update of attachment 77567 [details] [details])
> > How do we test this?  Please explain what tests this affects in your ChagneLog or explain why testing is impossible.

[anton muhin]

Created attachment 77602 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77602 [details]
Patch

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

> WebCore/bindings/v8/V8DOMMap.h:-63
> -        virtual bool removeIfPresent(KeyType* key, v8::Persistent<v8::Data> value) = 0;

Why is this virtual if Its never overridden?

[anton muhin]

(In reply to comment #6)
> (From update of attachment 77602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77602&action=review
> 
> > WebCore/bindings/v8/V8DOMMap.h:-63
> > -        virtual bool removeIfPresent(KeyType* key, v8::Persistent<v8::Data> value) = 0;
> 
> Why is this virtual if Its never overridden?

It's overridden (see two implementations of this 'interface'), otherwise it won't compile.

[Eric Seidel (no email)]

Is this patch supposed to be up for review?  If so, please flag with r? otherwise it won't get seen in the review queue and will likely be ignored.

You also should CC some v8-related reviewers (of which I'm not one).

[anton muhin]

I am dreadful sorry, I forgot to put the r?.  Done.  Sorry again.

(In reply to comment #8)
> Is this patch supposed to be up for review?  If so, please flag with r? otherwise it won't get seen in the review queue and will likely be ignored.
> 
> You also should CC some v8-related reviewers (of which I'm not one).

[Eric Seidel (no email)]

No problem at all.  I don't really know what reviewers to suggest for this, but webkit-patch suggest-reviewers can calculate some for you if you find this isn't reviewed in a timely manner.

[anton muhin]

(In reply to comment #10)
> No problem at all.  I don't really know what reviewers to suggest for this, but webkit-patch suggest-reviewers can calculate some for you if you find this isn't reviewed in a timely manner.

Let me add Mads and Nate.

[Jeremy Orlow]

Comment on attachment 77602 [details]
Patch

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

I don't understand how this is covered by existing tests.

> WebCore/bindings/v8/DOMDataStore.cpp:157
> +    v8::Persistent<v8::Object> v8Object = v8::Persistent<v8::Object>::Cast(value);

So it's not possible that it'd be empty/null/undefined?

[anton muhin]

(In reply to comment #12)
> (From update of attachment 77602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77602&action=review
> 
> I don't understand how this is covered by existing tests.

Every DOM Node is added into this map when it's wrapped into JS object.  And eventually, when this wrapper is garbage collected, method removeIfPresent is invoked for each domNodeMap we have.

Thus one need to kill renderer process before it does any GC to miss this path.  And we have plenty layout tests which exercise GC-related behaviour.

> 
> > WebCore/bindings/v8/DOMDataStore.cpp:157
> > +    v8::Persistent<v8::Object> v8Object = v8::Persistent<v8::Object>::Cast(value);
> 
> So it's not possible that it'd be empty/null/undefined?

No, it cannot be---it's a wrapper which is now only weakly reachable.  And it cannot be empty by design.  And null and undefined are never GCed (and never wrapped into persistent handles).

Thanks a lot for your comment, Jeremy.

[WebKit Commit Bot]

Comment on attachment 77602 [details]
Patch

Clearing flags on attachment: 77602

Committed r76104: <http://trac.webkit.org/changeset/76104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.