[webkit-reviews] review denied: [Bug 27628] [v8] Simplify weak handlers callback : [Attachment 33415] Addressing Adam's comments

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied anton muhin <antonm at chromium.org>'s
request for review:
Bug 27628: [v8] Simplify weak handlers callback
https://bugs.webkit.org/show_bug.cgi?id=27628

Attachment 33415: Addressing Adam's comments
https://bugs.webkit.org/attachment.cgi?id=33415&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.