Bug 61963 - bindings/v8/DOMData.h/.cpp has numerous errors in processing DOMDataStores for different threads
Summary: bindings/v8/DOMData.h/.cpp has numerous errors in processing DOMDataStores fo...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61016
  Show dependency treegraph
 
Reported: 2011-06-02 14:31 PDT by Dmitry Lomov
Modified: 2011-06-06 15:51 PDT (History)
3 users (show)

See Also:


Attachments
Fixes (3.53 KB, patch)
2011-06-02 14:37 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff
Style fixes (3.55 KB, patch)
2011-06-02 14:50 PDT, Dmitry Lomov
levin: review-
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2011-06-02 14:31:05 PDT
if (!store->domData()->owningThread() == WTF::currentThread())

(misplaced boolean negation)

and others
Comment 1 Dmitry Lomov 2011-06-02 14:37:03 PDT
Created attachment 95812 [details]
Fixes
Comment 2 WebKit Review Bot 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.
Comment 3 Dmitry Lomov 2011-06-02 14:50:37 PDT
Created attachment 95815 [details]
Style fixes
Comment 4 Adam Barth 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?
Comment 5 Adam Barth 2011-06-02 16:13:55 PDT
Comment on attachment 95815 [details]
Style fixes

In particular, WTF::currentThread() is very slow.
Comment 6 David Levin 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?
Comment 7 Dmitry Lomov 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.