Bug 27628 – [v8] Simplify weak handlers callback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[anton muhin]

As current wisdom is Nodes should be only managed from main thread, that simplifies management of near to death DOM nodes notably.

[anton muhin]

Created attachment 33390 [details]
Initial version

I'd say management of DOM node map should be simplidied, esp. fi Adam's patcj is in.

[anton muhin]

Comment on attachment 33390 [details]
Initial version

Sorry, forgot to mark as a patch

[anton muhin]

Comment on attachment 33390 [details]
Initial version

> Index: third_party/WebKit/WebCore/ChangeLog
> ===================================================================
> --- third_party/WebKit/WebCore/ChangeLog	(revision 46295)
> +++ third_party/WebKit/WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> [phone]  Anton Muhin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Simplify management of Nodes in weak handles callbacks.
> +        https://bugs.webkit.org/show_bug.cgi?id=27628
> +
> +        * bindings/v8/V8DOMMap.cpp:
> +        (WebCore::weakNodeCallback):
> +
>  2009-07-23  Dan Bernstein  <[e-mail]>
>  
>          Reviewed by Dave Hyatt.
> Index: third_party/WebKit/WebCore/bindings/v8/V8DOMMap.cpp
> ===================================================================
> --- third_party/WebKit/WebCore/bindings/v8/V8DOMMap.cpp	(revision 46194)
> +++ third_party/WebKit/WebCore/bindings/v8/V8DOMMap.cpp	(working copy)
> @@ -504,9 +504,12 @@ void weakActiveDOMObjectCallback(v8::Per
>  
>  static void weakNodeCallback(v8::Persistent<v8::Value> v8Object, void* domObject)
>  {
> -    v8::HandleScope scope;
> -    ASSERT(v8Object->IsObject());
> -    DOMData::handleWeakObject<Node>(DOMDataStore::DOMNodeMap, v8::Handle<v8::Object>::Cast(v8Object), static_cast<Node*>(domObject));
> +    DOMDataStore* store = DOMDataStore::allStores()[0];
> +    DOMDataStore::InternalDOMWrapperMap<Node>& domMap = store->domNodeMap();
> +    Node* node = static_cast<Node*>(domObject);
> +    v8Object.Dispose();
> +    domMap.impl().remove(node);
> +    node->deref();  // Nobody overrides Node::deref so it's safe
>  }
>  
>  #if ENABLE(SVG)

The code to retrieve DOM Node map should be merged with Adam's patch obviously.

[Adam Barth]

Comment on attachment 33390 [details]
Initial version

+    DOMDataStore* store = DOMDataStore::allStores()[0];

Why is it sufficient only to look in the zeroth DOMDataStore?  What about the DOMDataStore for isolated worlds?

[Christian Plesner Hansen]

I'm still a little unclear on how the dom node mappings work.  This only works for nodes on the main thread right?  So with Adam's changes you would use DOMData::getCurrentMainThread()->getStore().domNodeMap() instead?

[Adam Barth]

(In reply to comment #5)
> I'm still a little unclear on how the dom node mappings work.  This only works
> for nodes on the main thread right?  So with Adam's changes you would use
> DOMData::getCurrentMainThread()->getStore().domNodeMap() instead?

You can try that, but it will crash because we're in the middle of a GC.  :)  That's why we iterate currently.

[Christian Plesner Hansen]

Okay.  Maybe not such a good idea then.

[anton muhin]

Created attachment 33415 [details]
Addressing Adam's comments

Thanks a lot, Adam.  May you have another look?

[Adam Barth]

Comment on attachment 33415 [details]
Addressing Adam's comments

This patch is still incorrect.

-    v8::HandleScope scope;

Don't we need a handle scope before talking to handles?

-    ASSERT(v8Object->IsObject());

Why drop this assert?

+        ASSERT(store->domData()->owningThread() == WTF::currentThread());

This assert is bogus.  Some of DOMDataStores belong to worker threads.

Here's what you should do:

1) Acquire the mutex before iterating through the list because items might be inserted or removed on another thread.

2) Move the ASSERT to after

+        if (it == domMapImpl.end() || it->second != *v8Object)

At that point, you've found a DOMDataStore that holds a Node and it must be on the main thread.

[anton muhin]

Created attachment 33424 [details]
Second round

Thanks a lot for another round, Adam.  Answering in comments.

[anton muhin]

(In reply to comment #9)
> (From update of attachment 33415 [details])
> This patch is still incorrect.
> 
> -    v8::HandleScope scope;
> 
> Don't we need a handle scope before talking to handles?

We don't create any handles and thus I don't think we need it, but, please, double check.

> 
> -    ASSERT(v8Object->IsObject());
> 
> Why drop this assert?

I can easily revive it.  My thought was it was originally necessary due to the cast below (which would check the type anyway, btw).  No I don't do that cast and hence why ASSERT?

> 
> +        ASSERT(store->domData()->owningThread() == WTF::currentThread());
> 
> This assert is bogus.  Some of DOMDataStores belong to worker threads.
> 
> Here's what you should do:
> 
> 1) Acquire the mutex before iterating through the list because items might be
> inserted or removed on another thread.
> 
> 2) Move the ASSERT to after
> 
> +        if (it == domMapImpl.end() || it->second != *v8Object)
> 
> At that point, you've found a DOMDataStore that holds a Node and it must be on
> the main thread.

You're absolutely right, I wasn't attentive enough.

[Adam Barth]

Comment on attachment 33424 [details]
Second round

I think you mistakenly uploaded the old version of the patch.

[anton muhin]

Created attachment 33518 [details]
The right one

[anton muhin]

Created attachment 33519 [details]
Fixing a minor type (should have used plural)

[anton muhin]

(In reply to comment #12)
> (From update of attachment 33424 [details])
> I think you mistakenly uploaded the old version of the patch.

I am very sorry, Adam, may you have a look now?

[Adam Barth]

Comment on attachment 33519 [details]
Fixing a minor type (should have used plural)

This is great.  Thanks for iterating on this.

[anton muhin]

(In reply to comment #16)
> (From update of attachment 33519 [details])
> This is great.  Thanks for iterating on this.

Thanks a lot for review, Adam!

[Adam Barth]

I would land this, except the canary isn't compiling.

[anton muhin]

(In reply to comment #18)
> I would land this, except the canary isn't compiling.

It used to compile.  Let me sync to the most recent version and update the patch.

[Adam Barth]

Oh, the patch is probably fine.  The tree is red:

http://build.chromium.org/buildbot/waterfall.fyi/waterfall?builder=Webkit%20(webkit.org)&builder=Webkit%20Mac%20(webkit.org)&builder=Webkit%20Linux%20(webkit.org)

[Adam Barth]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/v8/DOMDataStore.cpp
Committed r46462
	M	WebCore/ChangeLog
	M	WebCore/bindings/v8/DOMDataStore.cpp
r46462 = f0f290bb233ca9ad7eef395908089c6c4f307085 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46462