Bug 49613 - [V8] ASSERT(WTF::isMainThread()) fails in V8Binding::int32ToWebCoreString in workers
Summary: [V8] ASSERT(WTF::isMainThread()) fails in V8Binding::int32ToWebCoreString in ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-16 12:52 PST by Kinuko Yasuda
Modified: 2010-11-18 17:32 PST (History)
3 users (show)

See Also:


Attachments
Proposing patch (2.20 KB, patch)
2010-11-16 12:57 PST, Kinuko Yasuda
japhet: review+
japhet: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2010-11-16 12:52:10 PST
ASSERT(WTF::isMainThread()) fails in V8Binding::int32ToWebCoreString in workers.

In worker case, javascript is executed by a worker thread but not by the main thread so this assertion always fails.
For example, running the following test in chromium hits the assertion failure.
fast/workers/storage/execute-sql-args-worker.html
Comment 1 Kinuko Yasuda 2010-11-16 12:57:59 PST
Created attachment 74028 [details]
Proposing patch
Comment 2 Nate Chapin 2010-11-16 13:10:32 PST
Comment on attachment 74028 [details]
Proposing patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review

Other than the style nits, this looks ok.  However, I'd appreciate it if you waited for antonm to agree before committing :)

> WebCore/bindings/v8/V8Binding.cpp:390
>      } else
> -        webCoreString = String::number(value);
> +      webCoreString = String::number(value);

Indent 4.

> WebCore/bindings/v8/V8Binding.cpp:399
> +    if (WTF::isMainThread())
> +      return int32ToWebCoreStringFast(value);

Indent 4.
Comment 3 anton muhin 2010-11-16 23:19:33 PST
Comment on attachment 74028 [details]
Proposing patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review

> WebCore/bindings/v8/V8Binding.cpp:398
> +    if (WTF::isMainThread())

Please, check by running http://dromaeo.com/?dom performance implications.

I haven't looked into this code for some time, but WTF::isMainThread apparently used to be pretty expensive.
Comment 4 Nate Chapin 2010-11-17 09:57:25 PST
(In reply to comment #3)
> (From update of attachment 74028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74028&action=review
> 
> > WebCore/bindings/v8/V8Binding.cpp:398
> > +    if (WTF::isMainThread())
> 
> Please, check by running http://dromaeo.com/?dom performance implications.
> 
> I haven't looked into this code for some time, but WTF::isMainThread apparently used to be pretty expensive.

IIRC, globalObjectPrototypeIsDOMWindow() in V8DOMWrapper.cpp is a fairly cheap check for whether we're in a worker context or not.  Should we try to generalize that and put it somewhere public?
Comment 5 Kinuko Yasuda 2010-11-17 19:50:15 PST
I ran the dromaeo test with and w/o this patch (on my local linux box).
I didn't re-run the test more than twice for each, but the results looked ok/good to me, i.e. there doesn't seem a visible performance impact.

Before (w/o this patch):

http://dromaeo.com/?id=123148 
1441.23runs/s (Total)
http://dromaeo.com/?id=123179 
1418.11runs/s (Total)

After (with this patch):

http://dromaeo.com/?id=123150 
1437.06runs/s (Total)
http://dromaeo.com/?id=123177 
1426.23runs/s (Total)
Comment 6 anton muhin 2010-11-17 23:27:33 PST
Thank you very much, Kinuko!

(In reply to comment #5)
> I ran the dromaeo test with and w/o this patch (on my local linux box).
> I didn't re-run the test more than twice for each, but the results looked ok/good to me, i.e. there doesn't seem a visible performance impact.
> 
> Before (w/o this patch):
> 
> http://dromaeo.com/?id=123148 
> 1441.23runs/s (Total)
> http://dromaeo.com/?id=123179 
> 1418.11runs/s (Total)
> 
> After (with this patch):
> 
> http://dromaeo.com/?id=123150 
> 1437.06runs/s (Total)
> http://dromaeo.com/?id=123177 
> 1426.23runs/s (Total)
Comment 7 Kinuko Yasuda 2010-11-18 17:32:26 PST
Committed r72354: <http://trac.webkit.org/changeset/72354>