Bug 55728

Adding Jian Li, who may know.

(In reply to comment #0) > File API calls like FileEntry.file() fail when run in Worker threads due to an ASSERT(isMainThread()) in WTF::ARC4RandomNumberGenerator::randomNumber(), which is called by the UUID code utilized by BlobURL. > > It's not clear to me whether the fix is to enable JSC_MULTIPLE_THREADS (which would wrap accesses in a Mutex) or to use a different RNG. Which build do you see this problem? Chromium? OS? Could you please also attach the repro script?

See http://codereview.chromium.org/6609040/ for where this came up; I was enabling some previously disabled tests (in fact, they've always been disabled). And here's the trybot failure: http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/15118/steps/ui_tests/logs/stdio

(In reply to comment #3) > See http://codereview.chromium.org/6609040/ for where this came up; I was enabling some previously disabled tests (in fact, they've always been disabled). And here's the trybot failure: > > http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/15118/steps/ui_tests/logs/stdio You can also repro this by running chromium ToT build like this: $ path/to/chrome --allow-file-access-from-files files:///path/to/LayoutTests/fast/filesystem/workers/file-from-file-entry.html

Call stack: #0 0x0000000003363ac7 in WTF::(anonymous namespace)::ARC4RandomNumberGenerator: :randomNumber (this=0x7fb96afb7ea0) at third_party/WebKit/Source/JavaScriptCore/wtf/CryptographicallyRandomNumbe r.cpp:146 #1 0x0000000003363c2a in WTF::cryptographicallyRandomNumber () at third_party/WebKit/Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:181 #2 0x0000000003365ad1 in WTF::randomNumber () at third_party/WebKit/Source/JavaScriptCore/wtf/RandomNumber.cpp:57 #3 0x00000000033d2de8 in WebCore::createCanonicalUUIDString () at third_party/WebKit/Source/WebCore/platform/UUID.cpp:99 #4 0x00000000029f29e7 in WebCore::BlobURL::createBlobURL (originString=...) at third_party/WebKit/Source/WebCore/fileapi/BlobURL.cpp:79 #5 0x00000000029f2767 in WebCore::BlobURL::createInternalURL () at third_party/WebKit/Source/WebCore/fileapi/BlobURL.cpp:52 #6 0x00000000029f147a in WebCore::Blob::Blob (this=0x7fb96af5f460, blobData=..., size=-1) at third_party/WebKit/Source/WebCore/fileapi/Blob.cpp:47 #7 0x00000000029fd599 in WebCore::File::File (this=0x7fb96af5f460, path=...) at third_party/WebKit/Source/WebCore/fileapi/File.cpp:52 #8 0x00000000023fb149 in WebCore::File::create (path=...) at third_party/WebKit/Source/WebCore/fileapi/File.h:42 #9 0x00000000029f6ebb in WebCore::DOMFileSystem::createFile ( this=0x7fb95f3633c0, fileEntry=0x7fb95f361db0, successCallback=...) at third_party/WebKit/Source/WebCore/fileapi/DOMFileSystem.cpp:117 #10 0x00000000029fdcd8 in WebCore::FileEntry::file (this=0x7fb95f361db0, successCallback=..., errorCallback=...) at third_party/WebKit/Source/WebCore/fileapi/FileEntry.cpp:56

This assert only occurs in Linux chromium build. When we create a blob, we need to call the WTF random number generation function to create a blob URL. When a blob is created within a worker thread, we will hit an assert in wtf function that complains about the current thread not being main thread. I think the fix is to enable JSC_MULTIPLE_THREADS in chromium build. Currently the web worker is running in a worker process which contains only one WebKit thread. So disabling JSC_MULTIPLE_THREADS is fine. Since we're going to change it to run the worker in a worker thread of the same renderer process (per dimich's V8 work), I think it is the right time for us to enable this flag. Adam and Darin, how do you think?

A quick codesearch finds lots of references to JSC_MULTIPLE_THREADS, so I'd be a little worried about turning it on without doing some profiling. Coming at this from a different direction, how much do we care about UUIDs being cryptographically random? We can't care that much, since we create them using randomNumber() on Mac, which doesn't seem to use OS_RANDOMNESS. Could we just switch the UUID implementation on Chromium/Linux to use something less secure but already threadsafe?

Adding levin, in case he has thoughts about JSC_MULTIPLE_THREADS.

(In reply to comment #8) > Adding levin, in case he has thoughts about JSC_MULTIPLE_THREADS. I agree with Jian that we should enable this. The majority of usage in JavaScriptCore !wtf which is really about making jsc multithreaded but it has propagated outside of that directory to other places that need to be guarded as well. Given that we will do worker and isolates soon, we'll need to flip this switch.

Created attachment 90558 [details] Patch

Comment on attachment 90558 [details] Patch Clearing flags on attachment: 90558 Committed r84548: <http://trac.webkit.org/changeset/84548>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/84548 might have broken Chromium Win Release

Patch was rolled out, need to fix the chromium windows build.

Fixing this is not trivial (though probably not hard for a seasoned Windows coder). The build problem was due to some code that depends on pthread_once: I suspect the easiest fix will be to replace that with some Windows equivalent.

(In reply to comment #15) > Fixing this is not trivial (though probably not hard for a seasoned Windows coder). The build problem was due to some code that depends on pthread_once: I suspect the easiest fix will be to replace that with some Windows equivalent. Can you provide a pointer of the file/class/method that is causing the the problem?

wtf/FastMall(In reply to comment #16) > (In reply to comment #15) > > Fixing this is not trivial (though probably not hard for a seasoned Windows coder). The build problem was due to some code that depends on pthread_once: I suspect the easiest fix will be to replace that with some Windows equivalent. > > Can you provide a pointer of the file/class/method that is causing the the problem? Source/JavaScriptCore/wtf/FastMalloc.cpp:110 #if ENABLE(WTF_MULTIPLE_THREADS) static pthread_key_t isForbiddenKey; static pthread_once_t isForbiddenKeyOnce = PTHREAD_ONCE_INIT; static void initializeIsForbiddenKey() { pthread_key_create(&isForbiddenKey, 0); } ...

(In reply to comment #17) > > Can you provide a pointer of the file/class/method that is causing the the problem? > > Source/JavaScriptCore/wtf/FastMalloc.cpp:110 > > #if ENABLE(WTF_MULTIPLE_THREADS) > static pthread_key_t isForbiddenKey; > static pthread_once_t isForbiddenKeyOnce = PTHREAD_ONCE_INIT; > static void initializeIsForbiddenKey() > { > pthread_key_create(&isForbiddenKey, 0); > } > ... Geez... that's a scary and hairy looking .cpp file. There are a handful of places where pthread_once is used on JavaScriptCore. Most of them have OS(DARWIN) gaurds around them and some of them also have a not-so-thread-safe impl if not OS(DARWIN). JSLock in the absence of PTHREADS is scary... but it looks like the only pthreads callsite that matters for V8 users FastMalloc. The callsite in FastMalloc only matters for DEBUG builds (which is not obvious when looking at the .cpp file). This should be implementable in terms of TlsAlloc(), TlsGetValue(), and TlsSetValue() on windows. #include <windows.h> static DWORD isForibiddenTlsIndex = TLS_OUT_OF_INDEXES; static bool isForbidden() { // By default, fastMalloc is allowed so we don't allocate the // tls index unless we're asked to make it forbidden. return (isForibiddenTlsIndex != TLS_OUT_OF_INDEXES) && (TlsGetValue(isForibiddenTlsIndex) == (LPVOID)1); } void fastMallocForbid() { if (isForibiddenTlsIndex == TLS_OUT_OF_INDEXES) isForibiddenTlsIndex = TlsAlloc(); // a little racey, but close enough for debug only TlsSetValue(isForibiddenTlsIndex, (LPVOID)1); } void fastMallocAllow() { if (isForibiddenTlsIndex == TLS_OUT_OF_INDEXES) return; TlsSetValue(isForibiddenTlsIndex, (LPVOID)0); }

Thanks for taking a look, Michael; would you be up for uploading that patch as a patch? If not, I'll do my best to get to it before the M13 branch date (but Windows dev has a good bit of overhead for me so I try to avoid it when possible...)

(In reply to comment #19) > Thanks for taking a look, Michael; would you be up for uploading that patch as a patch? If not, I'll do my best to get to it before the M13 branch date (but Windows dev has a good bit of overhead for me so I try to avoid it when possible...) Sure... I'll post a patch.

Created attachment 92515 [details] fastMalloc windows fix

Attachment 92515 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/wtf/FastMalloc.cpp:127: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 92526 [details] fastMalloc windows fix

ping i'm just now looking at the prior patch to enable the flag, i could roll that change into the fastMalloc patch too if you like

Comment on attachment 92526 [details] fastMalloc windows fix I wanted to review your patch, but I don't understand this code. Seems reasonable though.

Comment on attachment 92526 [details] fastMalloc windows fix View in context: https://bugs.webkit.org/attachment.cgi?id=92526&action=review > Source/JavaScriptCore/ChangeLog:8 > + https://bugs.webkit.org/show_bug.cgi?id=55728 Probably it is better to create a new bug for this issue. > Source/JavaScriptCore/wtf/FastMalloc.cpp:110 > +#if OS(WINDOWS) It might be better to keep the function defintion unguarded and instead ifdef the implementation. > Source/JavaScriptCore/wtf/FastMalloc.cpp:121 > + && (TlsGetValue(isForibiddenTlsIndex) == (LPVOID)1); No need to separate into 2 lines. Also better to use static_cast<LPVOID>(1).

Comment on attachment 92526 [details] fastMalloc windows fix Thnx for looking jian! View in context: https://bugs.webkit.org/attachment.cgi?id=92526&action=review >> Source/JavaScriptCore/ChangeLog:8 > > Probably it is better to create a new bug for this issue. how about i just merge the two patches into one, define the FLAG (as in adam's patch) and fix the windows bustage (as in my patch) >> Source/JavaScriptCore/wtf/FastMalloc.cpp:110 >> +#if OS(WINDOWS) > > It might be better to keep the function defintion unguarded and instead ifdef the implementation. Did you look at how this was done for pthreads in the #else clause and for the cases where WTF_MULTIPLE_THREADS is not defined. Why should anything be different in the OS(WINDOWS) case? >> Source/JavaScriptCore/wtf/FastMalloc.cpp:121 >> + && (TlsGetValue(isForibiddenTlsIndex) == (LPVOID)1); > > No need to separate into 2 lines. > Also better to use static_cast<LPVOID>(1). ah... of course... the no line is too long rule

Created attachment 92838 [details] merged patches - merged the patches into one - made longer lines - use c++ cast operators instead of c-style

(In reply to comment #28) > Created an attachment (id=92838) [details] > merged patches > > - merged the patches into one > - made longer lines > - use c++ cast operators instead of c-style btw, how did this compile for WebKit Window before? Does this break what was happening there previously?

> btw, how did this compile for WebKit Window before? Does this break what was happening there previously? Oh... I was under the impression that ENABLE_WTF_MULTIPLE_THREADS isn't turned on for the webkit windows build. If that's not the case than idk?

(In reply to comment #30) > > btw, how did this compile for WebKit Window before? Does this break what was happening there previously? > > Oh... I was under the impression that ENABLE_WTF_MULTIPLE_THREADS isn't turned on for the webkit windows build. If that's not the case than idk? ENABLE_WTF_MULTIPLE_THREADS is turned on for Apple's Windows port, as can be seen by looking at Platform.h: <http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/Platform.h?rev=85719#L577>

Comment on attachment 92838 [details] merged patches Attachment 92838 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8645642 New failing tests: http/tests/inspector/console-resource-errors.html fast/forms/input-stepup-stepdown.html http/tests/inspector/console-xhr-logging.html svg/custom/text-repaint-including-stroke.svg fast/forms/input-valueasnumber-range.html fast/css/number-parsing-crash-2.html fast/forms/ValidityState-stepMismatch.html http/tests/inspector/network/download.html http/tests/inspector/network-preflight-options.html fast/forms/input-stepup-stepdown-from-renderer.html http/tests/inspector/network/ping.html fast/css/number-parsing-crash.html fast/forms/ValidityState-rangeOverflow.html fast/css/round-trip-values.html http/tests/inspector/change-iframe-src.html fast/forms/ValidityState-typeMismatch-number.html fast/css/large-value-csstext.html fast/forms/input-valueasnumber-number.html svg/custom/js-update-bounce.svg fast/forms/ValidityState-rangeUnderflow.html

Created attachment 92848 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

> ENABLE_WTF_MULTIPLE_THREADS is turned on for Apple's Windows port, as can be seen by looking at Platform.h: <http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/Platform.h?rev=85719#L577> ah... yes, i see that... so where does the win build pick up pthread.h from or is it that fastmalloc.cpp is not used in that build? i'll try to unravel what the deal is with this file and get back to this afterwards.

Comment on attachment 92838 [details] merged patches I don't think this can be landed as is due to the issue mentioned above about WebKit Windows.

I don't have a webkit win client that builds so it's not so easy to poke at this. I do see this in WTFCommon.vsprops, $(WebKitLibrariesDir)\include\pthreads And there is a pthread.h and matching lib file in WebKitLibraries\win * Pthreads-win32 - POSIX Threads Library for Win32 * Copyright(C) 1998 John E. Bossom So i suppose webkit/win is using that lib for some stuff and FastMalloc.cpp complies and links by virtue of it. Afaik, chrome/win, including chromium's webkit port for windows,does not use a library like that. This may be a good place for judicious use of PLATFORM(CHROMIUM).

(In reply to comment #36) > I don't have a webkit win client that builds so it's not so easy to poke at this. > > I do see this in WTFCommon.vsprops, $(WebKitLibrariesDir)\include\pthreads > And there is a pthread.h and matching lib file in WebKitLibraries\win > * Pthreads-win32 - POSIX Threads Library for Win32 > * Copyright(C) 1998 John E. Bossom > > So i suppose webkit/win is using that lib for some stuff and FastMalloc.cpp complies and links by virtue of it. Afaik, chrome/win, including chromium's webkit port for windows,does not use a library like that. > > This may be a good place for judicious use of PLATFORM(CHROMIUM). Sounds good to me.

> > This may be a good place for judicious use of PLATFORM(CHROMIUM). > > Sounds good to me. Based on comment #32, looks like setting ENABLE_WTF_MULTIPLE_THREADS does have bad consequences for chromium on linux. A more focused change right now to get rid of the particular ASSERTION we're running into would be to not set that flag, and instead poke directly at CryptographicallyRandomNumber.cpp. #if ENABLE(WTF_MULTIPLE_THREADS) || PLATFORM(CHROMIUM) #define THREAD_SAFE_ARC4_RAND 1 #endif class ARC4RandomNumberGenerator { public: ARC4RandomNumberGenerator(); uint32_t randomNumber(); void randomValues(void* buffer, size_t length); private: inline void addRandomData(unsigned char *data, int length); void stir(); void stirIfNeeded(); inline uint8_t getByte(); inline uint32_t getWord(); ARC4Stream m_stream; int m_count; #if THREAD_SAFE_ARC4_RAND Mutex m_mutex; #endif };

(In reply to comment #38) > > > This may be a good place for judicious use of PLATFORM(CHROMIUM). > > > > Sounds good to me. > > Based on comment #32, looks like setting ENABLE_WTF_MULTIPLE_THREADS does have bad consequences for chromium on linux. A more focused change right now to get rid of the particular ASSERTION we're running into would be to not set that flag, and instead poke directly at CryptographicallyRandomNumber.cpp. > > #if ENABLE(WTF_MULTIPLE_THREADS) || PLATFORM(CHROMIUM) > #define THREAD_SAFE_ARC4_RAND 1 > #endif > > class ARC4RandomNumberGenerator { > public: > ARC4RandomNumberGenerator(); > > uint32_t randomNumber(); > void randomValues(void* buffer, size_t length); > > private: > inline void addRandomData(unsigned char *data, int length); > void stir(); > void stirIfNeeded(); > inline uint8_t getByte(); > inline uint32_t getWord(); > > ARC4Stream m_stream; > int m_count; > #if THREAD_SAFE_ARC4_RAND > Mutex m_mutex; > #endif > }; It feels like there may be a deeper issue that is being glossed over with that failure from #32. If you look at what ENABLE_WTF_MULTIPLE_THREADS does, I wonder why it would cause any problems.

> It feels like there may be a deeper issue that is being glossed over with that failure from #32. If you look at what ENABLE_WTF_MULTIPLE_THREADS does, I wonder why it would cause any problems. The failures in #32 indicate that there is something more going on with ENABLE_WTF_MULTIPLE_THREADS, at least in our linux port. I really wouldn't know anything about that. While we definitely would need to come to terms with that to enable v8 isolates, I don't see where that needs prying into in order to enable random number generation on a background thread. There actually aren't all that many references to WTF_MULTIPLE_THREAD... D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\CryptographicallyRandomNumber.cpp(42):#if ENABLE(WTF_MULTIPLE_THREADS) || PLATFORM(CHROMIUM) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\dtoa.cpp(93):#if ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\dtoa.cpp(438):#if ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\dtoa.cpp(453):#if ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\dtoa.cpp(466):#if ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\dtoa.cpp(479):#if ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\FastMalloc.cpp(82):#if ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\FastMalloc.cpp(105):#if ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\FastMalloc.cpp(163):#else // ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\FastMalloc.cpp(180):#endif // ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\Platform.h(582):#define ENABLE_WTF_MULTIPLE_THREADS 1 D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\RandomNumber.cpp(64):#if !ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\RefCountedLeakCounter.cpp(82):#if ENABLE(WTF_MULTIPLE_THREADS) D:\src\chrome\3\src\third_party\WebKit\Source\JavaScriptCore\wtf\RefCountedLeakCounter.cpp(91):#if ENABLE(WTF_MULTIPLE_THREADS) Looks like s_dtoaP5Mutex is NULL because it's not initialized here in JavaScriptCore/runtime/InitializeThreading.cpp. That file is not built into chromium. static void initializeThreadingOnce() { // StringImpl::empty() does not construct its static string in a threadsafe fashion, // so ensure it has been initialized from here. StringImpl::empty(); WTF::initializeThreading(); wtfThreadData(); JSGlobalData::storeVPtrs(); #if ENABLE(JSC_MULTIPLE_THREADS) s_dtoaP5Mutex = new Mutex; initializeDates(); RegisterFile::initializeThreading(); #endif }

(In reply to comment #40) > > Looks like s_dtoaP5Mutex is NULL because it's not initialized here in JavaScriptCore/runtime/InitializeThreading.cpp. > That file is not built into chromium. > > static void initializeThreadingOnce() > { > // StringImpl::empty() does not construct its static string in a threadsafe fashion, > // so ensure it has been initialized from here. > StringImpl::empty(); > > WTF::initializeThreading(); > wtfThreadData(); > JSGlobalData::storeVPtrs(); > #if ENABLE(JSC_MULTIPLE_THREADS) > s_dtoaP5Mutex = new Mutex; > initializeDates(); > RegisterFile::initializeThreading(); > #endif > } Good point. Both s_dtoaP5Mutex = new Mutex; initializeDates(); are in the wrong place. Seems like // StringImpl::empty() does not construct its static string in a threadsafe fashion, // so ensure it has been initialized from here. StringImpl::empty(); wtfThreadData(); Should also be done in wtf. btw, the asserts are about to be removed from this method: https://bugs.webkit.org/show_bug.cgi?id=60846 so this issue won't be noticed. The real issue here (imo) is getting WTF_MULTIPLE_THREADS turned on for chromium but the current fire is won't be there shortly.

> Good point. > > Both > s_dtoaP5Mutex = new Mutex; > initializeDates(); > are in the wrong place. > > Seems like > // StringImpl::empty() does not construct its static string in a threadsafe fashion, > // so ensure it has been initialized from here. > StringImpl::empty(); > wtfThreadData(); > Should also be done in wtf. > > btw, the asserts are about to be removed from this method: https://bugs.webkit.org/show_bug.cgi?id=60846 so this issue won't be noticed. > > The real issue here (imo) is getting WTF_MULTIPLE_THREADS turned on for chromium but the current fire is won't be there shortly. Ah... i like abarth's lean on the delete key solution even more than the narrow #if PLATFORM(CHROMIUM) :)

> Both > s_dtoaP5Mutex = new Mutex; > initializeDates(); > are in the wrong place. > > Seems like > // StringImpl::empty() does not construct its static string in a threadsafe fashion, > // so ensure it has been initialized from here. > StringImpl::empty(); > wtfThreadData(); > Should also be done in wtf. All that looks out of place to me too, needs to be initted for WTF independently of JSC. Is there a WTF::initialize() call anywhere, that would be a good place to put get the TLS slot defined for FastMalloc on windows (the non-pthreads version) too. > btw, the asserts are about to be removed from this method: https://bugs.webkit.org/show_bug.cgi?id=60846 so this issue won't be noticed. > > The real issue here (imo) is getting WTF_MULTIPLE_THREADS turned on for chromium but the current fire is won't be there shortly. That wasn't the issue i came here to fix, but yes it certainly is the bigger fish to fry. Do we have a timeframe for putting v8 isolates to use? Is that actively being worked on at this point?

(In reply to comment #43) > That wasn't the issue i came here to fix, but yes it certainly is the bigger fish to fry. Do we have a timeframe for putting v8 isolates to use? Is that actively being worked on at this point? Yep, bug 61016. I also filed bug 61017 about turning on WTF_MULTIPLE_THREADS for chromium. I don't guess you need to do any more here (once bug 60846 lands).

should be fixed with 60846 being fixed