Bug 55326 - Implement WTF::randomNumber in terms of WTF::cryptographicallyRandomNumber when possible
Summary: Implement WTF::randomNumber in terms of WTF::cryptographicallyRandomNumber wh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-27 12:55 PST by Adam Barth
Modified: 2011-03-10 14:11 PST (History)
17 users (show)

See Also:


Attachments
Patch (3.91 KB, patch)
2011-02-27 12:57 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (4.07 KB, patch)
2011-02-27 14:39 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (4.07 KB, patch)
2011-02-27 16:58 PST, Adam Barth
no flags Details | Formatted Diff | Diff
crashlog for shared-worker-constructor (6.05 KB, text/plain)
2011-03-01 04:48 PST, Csaba Osztrogonác
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-02-27 12:55:41 PST
Implement WTF::randomNumber in terms of WTF::cryptographicallyRandomNumber when possible
Comment 1 Adam Barth 2011-02-27 12:57:43 PST
Created attachment 83987 [details]
Patch
Comment 2 WebKit Review Bot 2011-02-27 13:04:56 PST
Attachment 83987 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8074086
Comment 3 Collabora GTK+ EWS bot 2011-02-27 13:10:27 PST
Attachment 83987 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8070180
Comment 4 Early Warning System Bot 2011-02-27 13:17:45 PST
Attachment 83987 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8075069
Comment 5 WebKit Review Bot 2011-02-27 13:51:43 PST
Attachment 83987 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8071142
Comment 6 WebKit Review Bot 2011-02-27 13:56:01 PST
Attachment 83987 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8024889
Comment 7 Build Bot 2011-02-27 14:22:00 PST
Attachment 83987 [details] did not build on win:
Build output: http://queues.webkit.org/results/8070191
Comment 8 Build Bot 2011-02-27 14:33:48 PST
Attachment 83987 [details] did not build on win:
Build output: http://queues.webkit.org/results/8075089
Comment 9 Adam Barth 2011-02-27 14:35:00 PST
Comment on attachment 83987 [details]
Patch

Turns out coding is hard.  Let's go shopping!
Comment 10 Adam Barth 2011-02-27 14:39:20 PST
Created attachment 83996 [details]
Patch
Comment 11 WebKit Review Bot 2011-02-27 14:42:43 PST
Attachment 83996 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8075091
Comment 12 Collabora GTK+ EWS bot 2011-02-27 14:43:16 PST
Attachment 83996 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8073143
Comment 13 Early Warning System Bot 2011-02-27 15:01:26 PST
Attachment 83996 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8024891
Comment 14 Eric Seidel (no email) 2011-02-27 15:21:12 PST
Comment on attachment 83996 [details]
Patch

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

> Source/JavaScriptCore/wtf/RandomNumber.cpp:31
> +#include "cryptographicallyRandomNumber.h"

Probably your caps here.
Comment 15 Adam Barth 2011-02-27 16:58:27 PST
Created attachment 84003 [details]
Patch
Comment 16 Eric Seidel (no email) 2011-02-27 18:37:57 PST
Comment on attachment 84003 [details]
Patch

LGTM.
Comment 17 WebKit Commit Bot 2011-02-27 19:12:04 PST
Comment on attachment 84003 [details]
Patch

Clearing flags on attachment: 84003

Committed r79835: <http://trac.webkit.org/changeset/79835>
Comment 18 WebKit Commit Bot 2011-02-27 19:12:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Alejandro G. Castro 2011-02-28 04:51:48 PST
It seems this patch broke the jscore-tests in the gtk debug bots, it fails in an assertion:

ASSERTION FAILED: isMainThread()<br>
../../Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp(146) : uint32_t WTF::<unnamed>::ARC4RandomNumberGenerator::randomNumber()<br>
</tt><br>

http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Debug/builds/14100/steps/jscore-test/logs/results
Comment 20 Alejandro G. Castro 2011-02-28 04:55:51 PST
More information about the assertion failure:

Thread 1 (Thread 11402):
#0  0x00007f7c7de5d175 in WTF::(anonymous namespace)::ARC4RandomNumberGenerator::randomNumber (this=0xd70340) at ../../Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:146
#1  0x00007f7c7de5d2ef in WTF::cryptographicallyRandomNumber () at ../../Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:181
#2  0x00007f7c7de6b171 in WTF::randomNumber () at ../../Source/JavaScriptCore/wtf/RandomNumber.cpp:57
#3  0x00007f7c7cfff3d9 in JSC::JSGlobalObject::JSGlobalObjectData::JSGlobalObjectData (this=0x7f7c6c009780, destructor=0x7f7c7cffa2d6 <WebCore::JSDOMGlobalObject::destroyJSDOMGlobalObjectData(void*)>) at ../../Source/JavaScriptCore/runtime/JSGlobalObject.h:73
#4  0x00007f7c7d03d83b in WebCore::JSDOMGlobalObject::JSDOMGlobalObjectData::JSDOMGlobalObjectData (this=0x7f7c6c009780, world=0x7f7c6c005ff0, destructor=0x7f7c7cffa2d6 <WebCore::JSDOMGlobalObject::destroyJSDOMGlobalObjectData(void*)>) at ../../Source/WebCore/bindings/js/JSDOMGlobalObject.h:82
#5  0x00007f7c7d03d34e in WebCore::JSWorkerContextBase::JSWorkerContextBase (this=0x7f7c2acc10d0, structure=..., impl=...) at ../../Source/WebCore/bindings/js/JSWorkerContextBase.cpp:49
#6  0x00007f7c7dbba6af in WebCore::JSWorkerContext::JSWorkerContext (this=0x7f7c2acc10d0, structure=..., impl=...) at DerivedSources/WebCore/JSWorkerContext.cpp:163
#7  0x00007f7c7db83dbb in WebCore::JSSharedWorkerContext::JSSharedWorkerContext (this=0x7f7c2acc10d0, structure=..., impl=...) at DerivedSources/WebCore/JSSharedWorkerContext.cpp:88
#8  0x00007f7c7d072fdb in WebCore::WorkerScriptController::initScript (this=0x778190) at ../../Source/WebCore/bindings/js/WorkerScriptController.cpp:94
#9  0x00007f7c7cffb20c in WebCore::WorkerScriptController::initScriptIfNeeded (this=0x778190) at ../../Source/WebCore/bindings/js/WorkerScriptController.h:74
#10 0x00007f7c7d073323 in WebCore::WorkerScriptController::evaluate (this=0x778190, sourceCode=..., exception=0x7f7c2b946ad0) at ../../Source/WebCore/bindings/js/WorkerScriptController.cpp:125
#11 0x00007f7c7d0731dc in WebCore::WorkerScriptController::evaluate (this=0x778190, sourceCode=...) at ../../Source/WebCore/bindings/js/WorkerScriptController.cpp:109
#12 0x00007f7c7d9aabe5 in WebCore::WorkerThread::workerThread (this=0xd523d0) at ../../Source/WebCore/workers/WorkerThread.cpp:135
#13 0x00007f7c7d9aaa40 in WebCore::WorkerThread::workerThreadStart (thread=0xd523d0) at ../../Source/WebCore/workers/WorkerThread.cpp:118
#14 0x00007f7c7de78d11 in WTF::threadEntryPoint (contextData=0x778190) at ../../Source/JavaScriptCore/wtf/Threading.cpp:67
#15 0x00007f7c78f158ba in start_thread () from /lib/libpthread.so.0
#16 0x00007f7c78c7d02d in clone () from /lib/libc.so.6

http://webkit-bots.igalia.com/amd64/svn_79853.core-when_1298894990-_-who_DumpRenderTree-_-why_11.trace.html
Comment 21 Oliver Hunt 2011-02-28 08:36:26 PST
It would appear that the cryptographically secure source is not threadsafe, abarth?
Comment 22 Adam Barth 2011-02-28 10:19:55 PST
It's supposed to be thread-safe.  Maybe I screwed up the ifdefs?  I can look at this this afternoon.  Perhaps the threading ifdefs on gtk are different than on Mac?
Comment 23 WebKit Review Bot 2011-02-28 17:28:03 PST
http://trac.webkit.org/changeset/79835 might have broken GTK Linux 32-bit Debug
Comment 24 Csaba Osztrogonác 2011-02-28 23:56:35 PST
There are similar assertions on Qt debug bots:

ASSERTION FAILED: isMainThread()
../../../Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp(146) : uint32_t WTF::<unnamed>::ARC4RandomNumberGenerator::randomNumber()

All fast/workers tests assert.

Reopen to fix the bug.
Comment 25 Adam Barth 2011-03-01 01:29:48 PST
> There are similar assertions on Qt debug bots:

There's been a bunch of discussion on the GTK version of this bug.  Basically, ENABLE(WORKERS) needs to imply ENABLE(JSC_MULTIPLE_THREADS) or else you'll have a bunch of race conditions.  This code just ASSERTS that you don't have race conditions, which is catching the underlying error.
Comment 26 Alejandro G. Castro 2011-03-01 03:37:06 PST
FYI yesterday we landed http://trac.webkit.org/changeset/79952 trying to fix this for the ports using PTHREAD, we had an old bug with a outdated patch and we reviewed it to have an initial solution that we have to review because the situation it is still not clear.
Comment 27 Csaba Osztrogonác 2011-03-01 03:57:35 PST
Zoltan and/or Gabor, could check ( and maybe fix :) ) the Qt asserts?
Comment 28 Alejandro G. Castro 2011-03-01 04:02:55 PST
(In reply to comment #25)
> > There are similar assertions on Qt debug bots:
> 
> There's been a bunch of discussion on the GTK version of this bug.  Basically, ENABLE(WORKERS) needs to imply ENABLE(JSC_MULTIPLE_THREADS) or else you'll have a bunch of race conditions.  This code just ASSERTS that you don't have race conditions, which is catching the underlying error.

Yeah, yesterday we still did not add this condition to the patch we landed because we did not want to cause more trouble to other ports with this initial patch if they were not detecting it, but basically it seems we should add something like this:

diff --git a/Source/JavaScriptCore/wtf/Platform.h b/Source/JavaScriptCore/wtf/Platform.h
index 2f1f592..f554756 100644
--- a/Source/JavaScriptCore/wtf/Platform.h
+++ b/Source/JavaScriptCore/wtf/Platform.h
@@ -563,7 +563,7 @@
 #define WTF_USE_MERSENNE_TWISTER_19937 1
 #endif
 
-#if (PLATFORM(GTK) || PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || (PLATFORM(QT) && OS(DARWIN) && !ENABLE(SINGLE_THREADED))) && !defined(ENABLE_JSC_MULTIPLE_THREADS)
+#if (ENABLE(WORKERS) || PLATFORM(GTK) || PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || (PLATFORM(QT) && OS(DARWIN) && !ENABLE(SINGLE_THREADED))) && !defined(ENABLE_JSC_MULTIPLE_THREADS)
 #define ENABLE_JSC_MULTIPLE_THREADS 1
 #endif

Ossy could you check if that fixes the problem? Thanks.
Comment 29 Csaba Osztrogonác 2011-03-01 04:48:56 PST
Created attachment 84211 [details]
crashlog for shared-worker-constructor

I tried the patch in Comment #28, but tests still crash ( but don't assert :) )
I attached one of the crash logs.
Comment 30 Adam Barth 2011-03-01 13:31:48 PST
The top part of that call-stack doesn't have symbols, but that crash is somewhat mysterious to me.
Comment 31 Csaba Osztrogonác 2011-03-10 14:11:05 PST
It seems http://trac.webkit.org/changeset/80746 solved the crashes on Qt bots.
(https://bugs.webkit.org/show_bug.cgi?id=33008)