Bug 27628

Summary: [v8] Simplify weak handlers callback
Product: WebKit Reporter: anton muhin <antonm@chromium.org>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, ager@chromium.org, christian.plesner.hansen@gmail.com, iposva@chromium.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Initial version
abarth: review-
Addressing Adam's comments
abarth: review-
Second round
abarth: review-
The right one
none
Fixing a minor type (should have used plural) abarth: review+

Description From 2009-07-23 15:52:22 PST
As current wisdom is Nodes should be only managed from main thread, that simplifies management of near to death DOM nodes notably.
------- Comment #1 From 2009-07-23 16:11:20 PST -------
Created an attachment (id=33390) [details]
Initial version

I'd say management of DOM node map should be simplidied, esp. fi Adam's patcj is in.
------- Comment #2 From 2009-07-23 19:34:54 PST -------
(From update of attachment 33390 [details])
Sorry, forgot to mark as a patch
------- Comment #3 From 2009-07-23 19:36:11 PST -------
(From update of attachment 33390 [details])
> Index: third_party/WebKit/WebCore/ChangeLog
> ===================================================================
> --- third_party/WebKit/WebCore/ChangeLog	(revision 46295)
> +++ third_party/WebKit/WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,13 @@
> +2009-07-23  Anton Muhin  <antonm@chromium.org>
> +
> +        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  <mitz@apple.com>
>  
>          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.
------- Comment #4 From 2009-07-23 23:47:26 PST -------
(From update of attachment 33390 [details])
+    DOMDataStore* store = DOMDataStore::allStores()[0];

Why is it sufficient only to look in the zeroth DOMDataStore?  What about the DOMDataStore for isolated worlds?
------- Comment #5 From 2009-07-24 01:03:27 PST -------
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?
------- Comment #6 From 2009-07-24 01:12:12 PST -------
(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.
------- Comment #7 From 2009-07-24 01:25:40 PST -------
Okay.  Maybe not such a good idea then.
------- Comment #8 From 2009-07-24 02:22:32 PST -------
Created an attachment (id=33415) [details]
Addressing Adam's comments

Thanks a lot, Adam.  May you have another look?
------- Comment #9 From 2009-07-24 02:45:59 PST -------
(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?

-    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.
------- Comment #10 From 2009-07-24 04:14:14 PST -------
Created an attachment (id=33424) [details]
Second round

Thanks a lot for another round, Adam.  Answering in comments.
------- Comment #11 From 2009-07-24 04:16:10 PST -------
(In reply to comment #9)
> (From update of attachment 33415 [details] [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.
------- Comment #12 From 2009-07-24 10:41:21 PST -------
(From update of attachment 33424 [details])
I think you mistakenly uploaded the old version of the patch.
------- Comment #13 From 2009-07-26 21:59:28 PST -------
Created an attachment (id=33518) [details]
The right one
------- Comment #14 From 2009-07-26 22:08:37 PST -------
Created an attachment (id=33519) [details]
Fixing a minor type (should have used plural)
------- Comment #15 From 2009-07-26 22:09:07 PST -------
(In reply to comment #12)
> (From update of attachment 33424 [details] [details])
> I think you mistakenly uploaded the old version of the patch.

I am very sorry, Adam, may you have a look now?
------- Comment #16 From 2009-07-26 22:19:21 PST -------
(From update of attachment 33519 [details])
This is great.  Thanks for iterating on this.
------- Comment #17 From 2009-07-27 01:34:18 PST -------
(In reply to comment #16)
> (From update of attachment 33519 [details] [details])
> This is great.  Thanks for iterating on this.

Thanks a lot for review, Adam!
------- Comment #18 From 2009-07-27 02:12:45 PST -------
I would land this, except the canary isn't compiling.
------- Comment #19 From 2009-07-27 02:36:48 PST -------
(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.
------- Comment #20 From 2009-07-27 02:37:56 PST -------
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)
------- Comment #21 From 2009-07-28 01:27:09 PST -------
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