Add an assertion to ensure AtomicString from two different threads are not compared
Created attachment 193959 [details] Patch
A tiny step toward more safety debugging WebCore code with threads. I will have to modify WebCore accordingly (some HashTable use StringImpl* directly).
YES I'm glad you're adding this. Sad that it's only turned on when that option is turned on. Maybe this should always be on for Debug?
Also, be aware that abarth is in the process of making HTMLNames and friends static Atomic strings, which would allow them to be used in multiple AtomicStringTables, and potentially safe for x-thread compares.
(In reply to comment #3) > YES I'm glad you're adding this. Sad that it's only turned on when that option is turned on. Maybe this should always be on for Debug? ASSERT_ATOMIC_STRINGIMPL_THREAD_CONSISTENCY is enabled on !ASSERT_DISABLED It will be enabled on the vast majority of debug builds. (In reply to comment #4) > Also, be aware that abarth is in the process of making HTMLNames and friends static Atomic strings, which would allow them to be used in multiple AtomicStringTables, and potentially safe for x-thread compares. I am really excited about that work actually :) When it lands, we will need to change the assertion accordingly (probably just add that a && b must be static).
This looks interesting. Would be willing to wait on landing this patch until after https://bugs.webkit.org/show_bug.cgi?id=112769 lands?
(In reply to comment #6) > This looks interesting. Would be willing to wait on landing this patch until after https://bugs.webkit.org/show_bug.cgi?id=112769 lands? Not a problem. But I don't think the 2 patches conflicts. It will just be 1 more condition in the assertion + 1 more initialization in the constructor.
This patch assumes that a string impl is only ever in one atomic string table. That assumption might or might not remain true after my patch.
Created attachment 205639 [details] Patch
Comment on attachment 205639 [details] Patch Attachment 205639 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/998356 New failing tests: fast/forms/select/popup-closes-on-blur.html
Created attachment 205757 [details] Archive of layout-test-results from APPLE-EWS-5 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: APPLE-EWS-5 Port: win-future Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
Comment on attachment 205639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205639&action=review > Source/WTF/wtf/text/AtomicString.cpp:431 > + r->setAtomicStringTable(0); Perhaps not part of this patch, but we really need to assert that we are not removing from the wrong table here. > Source/WTF/wtf/text/AtomicStringImpl.h:48 > +ALWAYS_INLINE bool safeAtomicEqual(const AtomicStringImpl* a, const AtomicStringImpl* b) I'm not a fan of "safe" names, they don't really explain much. Perhaps "equalWithThreadingAssertion"? Or just split the assertion into a separate function, and have callers invoke it explicitly before comparing pointers? The latter seems preferable to me.