Bug 55326 – Implement WTF::randomNumber in terms of WTF::cryptographicallyRandomNumber when possible

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Implement WTF::randomNumber in terms of WTF::cryptographicallyRandomNumber when possible

[Adam Barth]

Created attachment 83987 [details]
Patch

[WebKit Review Bot]

Attachment 83987 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8074086

[Collabora GTK+ EWS bot]

Attachment 83987 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8070180

[Early Warning System Bot]

Attachment 83987 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8075069

[WebKit Review Bot]

Attachment 83987 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8071142

[WebKit Review Bot]

Attachment 83987 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8024889

[Build Bot]

Attachment 83987 [details] did not build on win:
Build output: http://queues.webkit.org/results/8070191

[Build Bot]

Attachment 83987 [details] did not build on win:
Build output: http://queues.webkit.org/results/8075089

[Adam Barth]

Comment on attachment 83987 [details]
Patch

Turns out coding is hard.  Let's go shopping!

[Adam Barth]

Created attachment 83996 [details]
Patch

[WebKit Review Bot]

Attachment 83996 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8075091

[Collabora GTK+ EWS bot]

Attachment 83996 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8073143

[Early Warning System Bot]

Attachment 83996 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8024891

[Eric Seidel (no email)]

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.

[Adam Barth]

Created attachment 84003 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 84003 [details]
Patch

LGTM.

[WebKit Commit Bot]

Comment on attachment 84003 [details]
Patch

Clearing flags on attachment: 84003

Committed r79835: <http://trac.webkit.org/changeset/79835>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alejandro G. Castro]

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

[Alejandro G. Castro]

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

[Oliver Hunt]

It would appear that the cryptographically secure source is not threadsafe, abarth?

[Adam Barth]

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?

[WebKit Review Bot]

http://trac.webkit.org/changeset/79835 might have broken GTK Linux 32-bit Debug

[Csaba Osztrogonác]

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.

[Adam Barth]

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

[Alejandro G. Castro]

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.

[Csaba Osztrogonác]

Zoltan and/or Gabor, could check ( and maybe fix :) ) the Qt asserts?

[Alejandro G. Castro]

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

[Csaba Osztrogonác]

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.

[Adam Barth]

The top part of that call-stack doesn't have symbols, but that crash is somewhat mysterious to me.

[Csaba Osztrogonác]

It seems http://trac.webkit.org/changeset/80746 solved the crashes on Qt bots.
(https://bugs.webkit.org/show_bug.cgi?id=33008)