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
Created attachment 74028 [details] Proposing patch
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 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.
(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?
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)
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)
Committed r72354: <http://trac.webkit.org/changeset/72354>