Summary: | [v8] Simplify weak handlers callback | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | anton muhin <antonm> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, ager, christian.plesner.hansen, iposva | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
anton muhin
2009-07-23 15:52:22 PDT
Created attachment 33390 [details]
Initial version
I'd say management of DOM node map should be simplidied, esp. fi Adam's patcj is in.
Comment on attachment 33390 [details]
Initial version
Sorry, forgot to mark as a patch
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 @@ > +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 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?
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? (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. Okay. Maybe not such a good idea then. Created attachment 33415 [details]
Addressing Adam's comments
Thanks a lot, Adam. May you have another look?
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.
Created attachment 33424 [details]
Second round
Thanks a lot for another round, Adam. Answering in comments.
(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. Comment on attachment 33424 [details]
Second round
I think you mistakenly uploaded the old version of the patch.
Created attachment 33518 [details]
The right one
Created attachment 33519 [details]
Fixing a minor type (should have used plural)
(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? Comment on attachment 33519 [details]
Fixing a minor type (should have used plural)
This is great. Thanks for iterating on this.
(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! I would land this, except the canary isn't compiling. (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. 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) 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 |