Bug 200507

Created attachment 375704 [details] WIP Patch

Attachment 375704 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:76: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:78: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:129: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:131: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 1 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 375706 [details] WIP Patch

Attachment 375706 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:76: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:78: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:129: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:131: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 375706 [details] WIP Patch Attachment 375706 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12874080 Number of test failures exceeded the failure limit.

Created attachment 375714 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6

The assertions would have to be a bit smarter since it is not uncommon for us to transfer ownership of a RefCounted object from one thread to another, in a thread-safe way. I think there must be a way to write such assertions though.

Comment on attachment 375706 [details] WIP Patch Attachment 375706 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12874295 Number of test failures exceeded the failure limit.

Created attachment 375730 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 375732 [details] WIP Patch

Attachment 375732 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:79: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:81: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:135: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:137: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 375735 [details] WIP Patch

Attachment 375735 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:79: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:81: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:135: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:137: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 375735 [details] WIP Patch Attachment 375735 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12875137 Number of test failures exceeded the failure limit.

Created attachment 375751 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 375735 [details] WIP Patch Attachment 375735 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12875372 Number of test failures exceeded the failure limit.

Created attachment 375755 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 375735 [details] WIP Patch Attachment 375735 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12875801 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases

Comment on attachment 375735 [details] WIP Patch Attachment 375735 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12876136 New failing tests: storage/indexeddb/pending-version-change-on-exit-private.html fast/images/clear-animation-decoder.html storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/modern/worker-getall.html storage/indexeddb/modern/blob-simple-workers.html storage/indexeddb/pending-version-change-stuck-private.html storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange-workers.html storage/indexeddb/pending-version-change-on-exit.html storage/indexeddb/open-twice-workers.html storage/indexeddb/objectstore-basics-workers.html storage/indexeddb/pending-version-change-stuck.html storage/indexeddb/dont-commit-on-blocked-private.html storage/indexeddb/basics-workers.html storage/indexeddb/dont-commit-on-blocked.html storage/indexeddb/transaction-complete-workers.html storage/indexeddb/pending-version-change-stuck-works-with-terminate.html storage/indexeddb/transaction-complete-workers-private.html storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html storage/indexeddb/cursor-advance-workers.html storage/indexeddb/index-basics-workers.html storage/indexeddb/pending-activity-workers.html

Created attachment 375773 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 375735 [details] WIP Patch Attachment 375735 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12877098 New failing tests: storage/indexeddb/pending-version-change-on-exit-private.html storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/modern/worker-getall.html storage/indexeddb/modern/blob-simple-workers.html storage/indexeddb/pending-version-change-stuck-private.html storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange-workers.html storage/indexeddb/dont-commit-on-blocked-private.html storage/indexeddb/objectstore-basics-workers.html storage/indexeddb/pending-version-change-stuck.html storage/indexeddb/pending-version-change-on-exit.html storage/indexeddb/basics-workers.html storage/indexeddb/dont-commit-on-blocked.html storage/indexeddb/transaction-complete-workers.html storage/indexeddb/pending-version-change-stuck-works-with-terminate.html storage/indexeddb/transaction-complete-workers-private.html storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html storage/indexeddb/pending-activity-workers.html storage/indexeddb/cursor-advance-workers.html storage/indexeddb/index-basics-workers.html storage/indexeddb/open-twice-workers.html

Created attachment 375785 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 375804 [details] WIP Patch

Attachment 375804 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:84: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:86: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:140: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:143: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 375805 [details] WIP Patch

Attachment 375805 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:84: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:86: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:140: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:143: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 375805 [details] WIP Patch Attachment 375805 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12879480 New failing tests: storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/pending-version-change-stuck-private.html webaudio/silence-after-playback.html http/tests/security/webaudio-render-remote-audio-blocked-no-crossorigin.html storage/indexeddb/pending-activity-workers.html storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange-workers.html webaudio/silent-audio-interrupted-in-background.html storage/indexeddb/open-twice-workers.html storage/indexeddb/modern/worker-getall.html http/tests/security/webaudio-render-remote-audio-allowed-crossorigin.html storage/indexeddb/pending-version-change-stuck.html imported/w3c/IndexedDB-private-browsing/idb_webworkers.html storage/indexeddb/objectstore-basics-workers.html storage/indexeddb/pending-version-change-on-exit.html storage/indexeddb/basics-workers.html storage/indexeddb/dont-commit-on-blocked.html storage/indexeddb/transaction-complete-workers-private.html storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html storage/indexeddb/cursor-advance-workers.html storage/indexeddb/pending-version-change-on-exit-private.html storage/indexeddb/modern/blob-simple-workers.html imported/w3c/web-platform-tests/webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/mediaElementAudioSourceToScriptProcessorTest.html storage/indexeddb/transaction-complete-workers.html storage/indexeddb/pending-version-change-stuck-works-with-terminate.html imported/w3c/web-platform-tests/IndexedDB/idb_webworkers.htm storage/indexeddb/index-basics-workers.html

Created attachment 375812 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 375805 [details] WIP Patch Attachment 375805 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12879471 New failing tests: apiTests

Comment on attachment 375805 [details] WIP Patch Attachment 375805 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12879509 New failing tests: storage/indexeddb/pending-activity-workers.html storage/indexeddb/pending-version-change-stuck-private.html webaudio/silence-after-playback.html http/tests/security/webaudio-render-remote-audio-blocked-no-crossorigin.html storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange-workers.html webaudio/silent-audio-interrupted-in-background.html storage/indexeddb/open-twice-workers.html storage/indexeddb/modern/worker-getall.html http/tests/security/webaudio-render-remote-audio-allowed-crossorigin.html storage/indexeddb/pending-version-change-stuck.html imported/w3c/IndexedDB-private-browsing/idb_webworkers.html storage/indexeddb/objectstore-basics-workers.html storage/indexeddb/pending-version-change-on-exit.html storage/indexeddb/basics-workers.html storage/indexeddb/dont-commit-on-blocked.html storage/indexeddb/transaction-complete-workers-private.html storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html storage/indexeddb/cursor-advance-workers.html storage/indexeddb/pending-version-change-on-exit-private.html storage/indexeddb/modern/blob-simple-workers.html storage/indexeddb/transaction-complete-workers.html storage/indexeddb/pending-version-change-stuck-works-with-terminate.html imported/w3c/web-platform-tests/IndexedDB/idb_webworkers.htm storage/indexeddb/index-basics-workers.html

Created attachment 375822 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6

Created attachment 375828 [details] WIP Patch

Attachment 375828 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:84: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:86: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:140: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:143: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 375828 [details] WIP Patch Attachment 375828 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12880433 New failing tests: apiTests

Created attachment 375849 [details] WIP Patch

Attachment 375849 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:84: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:86: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:140: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:143: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 375849 [details] WIP Patch Attachment 375849 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12881687 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases apiTests

Created attachment 375973 [details] WIP Patch

Attachment 375973 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:89: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:91: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:155: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:158: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 375990 [details] WIP Patch

Attachment 375990 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/RefCounted.h:89: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:91: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:155: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/RefCounted.h:158: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 5 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 375990 [details] WIP Patch Attachment 375990 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12888809 New failing tests: apiTests

(In reply to Build Bot from comment #42) > Comment on attachment 375990 [details] > WIP Patch > > Attachment 375990 [details] did not pass jsc-ews (mac): > Output: https://webkit-queues.webkit.org/results/12888809 > > New failing tests: > apiTests Ahhh, so close. I was supposed to have disabled the assertion for jsc executable. Not sure why it did not work.

Created attachment 376030 [details] WIP Patch

Attachment 376030 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RefCounted.h:26: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 376043 [details] Patch

Comment on attachment 376043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376043&action=review > Source/JavaScriptCore/ChangeLog:9 > + * dfg/DFGPlan.cpp: > + (JSC::DFG::Plan::Plan): Could you explain why we want to disable the assertion here? > Source/WTF/ChangeLog:9 > + Add threading assertions to RefCounted to try and catch non-thread safe using of > + RefCounted objects. These assertions also found several thread safety bugs in our Instead of saying non-thread safe use, can we just say that cross-thread / concurrent use of RefCounted objects and note that ThreadSafeRefCounted should be used instead? As in, explain why using RefCounted in different threads is unsafe so that people reading this commit message can easily figure out what needs to be done when they encounter this assertion failure. > Source/WTF/wtf/RefCounted.h:46 > + ASSERT(!areThreadingCheckedEnabled() || m_isOwnedByMainThread == isMainThread()); Please add a comment saying that we should be using ThreadSafeRefCounted instead if objects need to be shared / used across threads. > Source/WTF/wtf/RefCounted.h:106 > + bool areThreadingCheckedEnabled() const Could you add a comment clarifying when this function can be used? I'm afraid people would start calling this in a bunch of other places when they hit this assertion.

(In reply to Ryosuke Niwa from comment #47) > Comment on attachment 376043 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376043&action=review > > > Source/JavaScriptCore/ChangeLog:9 > > + * dfg/DFGPlan.cpp: > > + (JSC::DFG::Plan::Plan): > > Could you explain why we want to disable the assertion here? I will put in a comment in the change log. I reported this to the JSC team and they said they would investigate, also Phil suspects the code is actually safe. > > Source/WTF/ChangeLog:9 > > + Add threading assertions to RefCounted to try and catch non-thread safe using of > > + RefCounted objects. These assertions also found several thread safety bugs in our > > Instead of saying non-thread safe use, can we just say that cross-thread / > concurrent use of RefCounted objects > and note that ThreadSafeRefCounted should be used instead? > As in, explain why using RefCounted in different threads is unsafe so that > people reading this commit message > can easily figure out what needs to be done when they encounter this > assertion failure. > > > Source/WTF/wtf/RefCounted.h:46 > > + ASSERT(!areThreadingCheckedEnabled() || m_isOwnedByMainThread == isMainThread()); > > Please add a comment saying that we should be using ThreadSafeRefCounted > instead > if objects need to be shared / used across threads. > > > Source/WTF/wtf/RefCounted.h:106 > > + bool areThreadingCheckedEnabled() const > > Could you add a comment clarifying when this function can be used? > I'm afraid people would start calling this in a bunch of other places when > they hit this assertion. I guess you meant this comment for the disableThreadingChecks() method, ok.

Created attachment 376051 [details] Patch

Comment on attachment 376051 [details] Patch Clearing flags on attachment: 376051 Committed r248525: <https://trac.webkit.org/changeset/248525>

All reviewed patches have been landed. Closing bug.

<rdar://problem/54191314>

It looks like the changes in https://trac.webkit.org/changeset/248525/webkit made tests exit early with crashes on Mojave Debug WK2. Build: https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/builds/4020 results: https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r248525%20(4020)/results.html

It looks like this may have also caused some JSC failures: https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/3424 stdio: https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/3424/steps/jscore-test/logs/stdio

Reverted r248525 for reason: Revert new threading assertions while I work on fixing the issues they exposed Committed r248529: <https://trac.webkit.org/changeset/248529>

(In reply to Truitt Savell from comment #53) > It looks like the changes in https://trac.webkit.org/changeset/248525/webkit > made tests exit early with crashes on Mojave Debug WK2. > > Build: > https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20%28Tests%29/ > builds/4020 > > results: > https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/ > r248525%20(4020)/results.html Those will be addressed via Bug 200629.

(In reply to Truitt Savell from comment #54) > It looks like this may have also caused some JSC failures: > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/3424 > > stdio: > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/3424/steps/jscore- > test/logs/stdio Looks like we are simply missing a couple of WTF::initializeMainThread(); in JSC.

Created attachment 376071 [details] Patch

Comment on attachment 376071 [details] Patch Clearing flags on attachment: 376071 Committed r248533: <https://trac.webkit.org/changeset/248533>

All reviewed patches have been landed. Closing bug.

Comment on attachment 376071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376071&action=review A few post-landing comments. Sorry I didn’t see this earlier. > Source/WTF/wtf/RefCounted.h:49 > + // If you hit this assertion, it means that the RefCounted object was ref'd or deref'd > + // concurrent from several threads, which is not safe. You should either subclass > + // ThreadSafeRefCounted instead, or make sure to always ref / deref from the same thread. Grammar error. Should be "concurrently from several threads" not "concurrent from several threads". But does the assertion really mean that it was ref/deref'd concurrently? To be pedantic, it means it was ref/deref'd on different threads, but not that this ref/deref'ing was actually concurrent. The concept of the assertion is that it detects that this object was ref/deref'd on multiple threads, and therefore we think it's *highly likely* that it can be ref'd/deref'd concurrently, which will cause really serious problems. I think it would be even better to have the comment be more precise about this. I took a cut at that below, when I saw that this code is repeated twice and suggested a new version. > Source/WTF/wtf/RefCounted.h:112 > + bool areThreadingCheckedEnabled() const Grammar error. Should be "are threading checks enabled", not "are threading checked enabled". But also, I suggest getting rid of the function since it only has to be called once. > Source/WTF/wtf/RefCounted.h:137 > +#if !ASSERT_DISABLED > + if (m_isOwnedByMainThread != isMainThread() && hasOneRef()) > + m_isOwnedByMainThread = isMainThread(); // Likely ownership transfer. > + > + // If you hit this assertion, it means that the RefCounted object was ref'd or deref'd > + // concurrent from several threads, which is not safe. You should either subclass > + // ThreadSafeRefCounted instead, or make sure to always ref / deref from the same thread. > + ASSERT_WITH_MESSAGE(!areThreadingCheckedEnabled() || m_isOwnedByMainThread == isMainThread(), "Should not be ref'd / deref'd concurrently from several threads"); > +#endif Since this is the same for ref and deref, I suggest we share this code instead of writing it out twice. And since it's inside ASSERT_DISABLED we can write it out in a way that's clearer. Here’s my cut: void applyRefDerefThreadingCheck() const { #if !ASSERT_DISABLED if (hasOneRef()) { // Likely an ownership transfer across threads that may be safe. m_isOwnedByMainThread = isMainThread(); } else if (areThreadingChecksEnabledGlobally && m_areThreadingChecksEnabled) { // If you hit this assertion, it means that the RefCounted object was ref/deref'd // from more than one thread in a way that is likely concurrent and not safe. Either derive // from ThreadSafeRefCounted instead of RefCounted or always ref/deref from the same thread. ASSERT_WITH_MESSAGE(m_isOwnedByMainThread == isMainThread(), "Should not ref/deref concurrently from several threads"); } #endif }

Comment on attachment 376071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376071&action=review >> Source/WTF/wtf/RefCounted.h:137 >> +#endif > > Since this is the same for ref and deref, I suggest we share this code instead of writing it out twice. And since it's inside ASSERT_DISABLED we can write it out in a way that's clearer. Here’s my cut: > > void applyRefDerefThreadingCheck() const > { > #if !ASSERT_DISABLED > if (hasOneRef()) { > // Likely an ownership transfer across threads that may be safe. > m_isOwnedByMainThread = isMainThread(); > } else if (areThreadingChecksEnabledGlobally && m_areThreadingChecksEnabled) { > // If you hit this assertion, it means that the RefCounted object was ref/deref'd > // from more than one thread in a way that is likely concurrent and not safe. Either derive > // from ThreadSafeRefCounted instead of RefCounted or always ref/deref from the same thread. > ASSERT_WITH_MESSAGE(m_isOwnedByMainThread == isMainThread(), "Should not ref/deref concurrently from several threads"); > } > #endif > } The comment could be even more accurate without diminishing its value. Here’s yet another cut: // If you hit this assertion, it means that the RefCounted object was ref/deref'd // from both the main thread and another in a way that is likely concurrent and unsafe. // Derive from ThreadSafeRefCounted and make sure the destructor is safe on threads // that call deref, or ref/deref from a single thread. ASSERT_WITH_MESSAGE(m_isOwnedByMainThread == isMainThread(), "Unsafe to ref/deref from different threads");

Applied Darin's post landing review comments in <https://trac.webkit.org/changeset/248544>.

(In reply to Chris Dumez from comment #63) > Applied Darin's post landing review comments in > <https://trac.webkit.org/changeset/248544>. And thanks, it does look better.

*** Bug 195656 has been marked as a duplicate of this bug. ***