RESOLVED INVALID 61963
bindings/v8/DOMData.h/.cpp has numerous errors in processing DOMDataStores for different threads 
https://bugs.webkit.org/show_bug.cgi?id=61963
Summary bindings/v8/DOMData.h/.cpp has numerous errors in processing DOMDataStores fo...
Dmitry Lomov
Reported 2011-06-02 14:31:05 PDT
if (!store->domData()->owningThread() == WTF::currentThread()) (misplaced boolean negation) and others
Attachments
Fixes (3.53 KB, patch)
2011-06-02 14:37 PDT, Dmitry Lomov 		no flags
DetailsFormatted DiffDiff
Style fixes (3.55 KB, patch)
2011-06-02 14:50 PDT, Dmitry Lomov 		levin: review-
abarth: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dmitry Lomov
Comment 1 2011-06-02 14:37:03 PDT
Created attachment 95812 [details] Fixes
WebKit Review Bot
Comment 2 2011-06-02 14:39:05 PDT
Attachment 95812 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/v8/DOMData.h:88: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dmitry Lomov
Comment 3 2011-06-02 14:50:37 PDT
Created attachment 95815 [details] Style fixes
Adam Barth
Comment 4 2011-06-02 16:13:06 PDT
Comment on attachment 95815 [details] Style fixes Did you run the Dromeo DOM core benchmark on this change?
Adam Barth
Comment 5 2011-06-02 16:13:55 PDT
Comment on attachment 95815 [details] Style fixes In particular, WTF::currentThread() is very slow.
David Levin
Comment 6 2011-06-02 16:23:05 PDT
Comment on attachment 95815 [details] Style fixes View in context: https://bugs.webkit.org/attachment.cgi?id=95815&action=review > Source/WebCore/bindings/v8/DOMData.h:88 > + if (store->domData()->owningThread() != WTF::currentThread()) Actually I take back my r+. Why is this change correct? How does this function get called on the wrong thread? And if it get called on the wrong thread, won't there be problems since removeIfPresent wasn't called?
Dmitry Lomov
Comment 7 2011-06-02 17:03:19 PDT
Comment on attachment 95815 [details] Style fixes View in context: https://bugs.webkit.org/attachment.cgi?id=95815&action=review >> Source/WebCore/bindings/v8/DOMData.h:88 >> + if (store->domData()->owningThread() != WTF::currentThread()) > > Actually I take back my r+. Why is this change correct? > > How does this function get called on the wrong thread? And if it get called on the wrong thread, won't there be problems since removeIfPresent wasn't called? This function only gets called on a wrong thread in (future) patch with Isolates, and currently DOM stores are affined to the threads. But it is not a good function indeed - it accesses the stores list without a mutex anyway, so doesn't really work in multithreaded situation; and the above has performance problems. I'll revert this bit, and restructure this for isolates in later fix.
Note You need to log in before you can comment on or make changes to this bug.