Bug 200507 – Add threading assertions to RefCounted

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Add threading assertions to RefCounted to make sure we do not ref / deref them on the wrong thread.

[Chris Dumez]

Created attachment 375704 [details]
WIP Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 375706 [details]
WIP Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Chris Dumez]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 375732 [details]
WIP Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 375735 [details]
WIP Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 375804 [details]
WIP Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 375805 [details]
WIP Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 375828 [details]
WIP Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 375849 [details]
WIP Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 375973 [details]
WIP Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 375990 [details]
WIP Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 376030 [details]
WIP Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 376043 [details]
Patch

[Ryosuke Niwa]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 376051 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376051 [details]
Patch

Clearing flags on attachment: 376051

Committed r248525: <https://trac.webkit.org/changeset/248525>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54191314>

[Truitt Savell]

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

[Truitt Savell]

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

[Chris Dumez]

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>

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 376071 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 376071 [details]
Patch

Clearing flags on attachment: 376071

Committed r248533: <https://trac.webkit.org/changeset/248533>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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
    }

[Darin Adler]

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");

[Chris Dumez]

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

[Chris Dumez]

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

[Fujii Hironori]

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