BlinkPlatformImpl::userAgent() is not thread safe |
||||
Issue description
WebString BlinkPlatformImpl::userAgent() {
CR_DEFINE_STATIC_LOCAL(
blink::WebString, user_agent,
(blink::WebString::fromUTF8(GetContentClient()->GetUserAgent())));
DCHECK(user_agent ==
blink::WebString::fromUTF8(GetContentClient()->GetUserAgent()));
return user_agent;
}
but navigator.userAgent is available in both workers and the main thread. I don't think making this ThreadSpecific makes sense though that just means we're copying the UA string for every thread. A static string should be safe to use across threads, we just need to mark the isStatic() bit inside it. I think that needs some work to play nice with AtomicStrings today.
,
Mar 29 2016
To fix this I think we need to remove the CR_STATIC_LOCAL in BlinkPlatformImpl::userAgent and instead cache the agent in FrameLoaderClientImpl. hajimehoshi@ can you look into this? It's okay if you're too busy.
,
Mar 29 2016
,
Mar 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40696fd9bf6a11db52416ccf98f129e72df47f73 commit 40696fd9bf6a11db52416ccf98f129e72df47f73 Author: hajimehoshi <hajimehoshi@chromium.org> Date: Wed Mar 30 05:40:00 2016 Cache user agent string in FrameLoaderClientImpl instead of BlinkPlatformImpl This CL makes FrameLoaderClientImpl cache user agent string instead of BlinkPlatformImpl to avoid threading issue. BUG= 598479 Review URL: https://codereview.chromium.org/1839023002 Cr-Commit-Position: refs/heads/master@{#383915} [modify] https://crrev.com/40696fd9bf6a11db52416ccf98f129e72df47f73/content/child/blink_platform_impl.cc [modify] https://crrev.com/40696fd9bf6a11db52416ccf98f129e72df47f73/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp [modify] https://crrev.com/40696fd9bf6a11db52416ccf98f129e72df47f73/third_party/WebKit/Source/web/FrameLoaderClientImpl.h
,
Mar 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40696fd9bf6a11db52416ccf98f129e72df47f73 commit 40696fd9bf6a11db52416ccf98f129e72df47f73 Author: hajimehoshi <hajimehoshi@chromium.org> Date: Wed Mar 30 05:40:00 2016 Cache user agent string in FrameLoaderClientImpl instead of BlinkPlatformImpl This CL makes FrameLoaderClientImpl cache user agent string instead of BlinkPlatformImpl to avoid threading issue. BUG= 598479 Review URL: https://codereview.chromium.org/1839023002 Cr-Commit-Position: refs/heads/master@{#383915} [modify] https://crrev.com/40696fd9bf6a11db52416ccf98f129e72df47f73/content/child/blink_platform_impl.cc [modify] https://crrev.com/40696fd9bf6a11db52416ccf98f129e72df47f73/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp [modify] https://crrev.com/40696fd9bf6a11db52416ccf98f129e72df47f73/third_party/WebKit/Source/web/FrameLoaderClientImpl.h
,
Mar 30 2016
,
Jun 23 2016
Renaming Blink>Architecture to Blink>Internals |
||||
►
Sign in to add a comment |
||||
Comment 1 by esprehn@chromium.org
, Mar 29 2016Hmm, I see now why it's hard to make AtomicString work. Today all static strings are inserted into the atomic string table for every thread. That way the isAtomic() bit is true on every thread. The shared static navigator.userAgent should cause spurious failures where isAtomic() is true because of one thread, but false because of another. Ugh, that's really bad. ex. worker.js var string = navigator.userAgent; while (true) { callNativeFunctionThatUsesAtomicString(string); } main.js var worker = new Worker("worker.js"); worker.onload = function() { callNativeFunctionThatUsesAtomicString(navigator.userAgent); }; if the worker.js runs first, it'll add the userAgent string to the Worker's AtomicStringTable and set isAtomic == true. Then when the main.js runs the isAtomic() bit is true, so we don't add it to the table, but then another AtomicString with the same value on the thread will fail == because they have different StringImpl values.