Bug 61963

Summary:

bindings/v8/DOMData.h/.cpp has numerous errors in processing DOMDataStores for different threads

Product:

WebKit

Reporter:

Dmitry Lomov <dslomov>

Component:

WebCore JavaScript

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

dslomov, levin, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

61016

Attachments:

Description	Flags
Fixes	none
Style fixes	levin: review-, abarth: commit-queue-

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	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
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.