Bug 200507 - Add threading assertions to RefCounted
Summary: Add threading assertions to RefCounted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 200540 200542 200545 200547 200599 200629
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-07 09:36 PDT by Chris Dumez
Modified: 2019-08-12 13:16 PDT (History)
18 users (show)

See Also:


Attachments
WIP Patch (1.47 KB, patch)
2019-08-07 09:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (1.83 KB, patch)
2019-08-07 09:51 PDT, Chris Dumez
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (2.55 MB, application/zip)
2019-08-07 11:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews214 for win-future (13.00 MB, application/zip)
2019-08-07 12:15 PDT, Build Bot
no flags Details
WIP Patch (2.09 KB, patch)
2019-08-07 12:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (2.73 KB, patch)
2019-08-07 12:53 PDT, Chris Dumez
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (541.28 KB, application/zip)
2019-08-07 14:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.73 MB, application/zip)
2019-08-07 14:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews214 for win-future (13.88 MB, application/zip)
2019-08-07 18:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews210 for win-future (14.02 MB, application/zip)
2019-08-07 21:38 PDT, Build Bot
no flags Details
WIP Patch (3.45 KB, patch)
2019-08-08 08:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (3.45 KB, patch)
2019-08-08 08:30 PDT, Chris Dumez
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (4.36 MB, application/zip)
2019-08-08 10:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (3.52 MB, application/zip)
2019-08-08 10:59 PDT, Build Bot
no flags Details
WIP Patch (3.45 KB, patch)
2019-08-08 11:43 PDT, Chris Dumez
ews: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (3.45 KB, patch)
2019-08-08 15:57 PDT, Chris Dumez
ews: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (4.78 KB, patch)
2019-08-09 15:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (4.78 KB, patch)
2019-08-09 18:16 PDT, Chris Dumez
ews: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (6.24 KB, patch)
2019-08-10 20:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (15.05 KB, patch)
2019-08-11 09:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (16.47 KB, patch)
2019-08-11 17:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (17.86 KB, patch)
2019-08-12 09:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-08-07 09:36:28 PDT
Add threading assertions to RefCounted to make sure we do not ref / deref them on the wrong thread.
Comment 1 Chris Dumez 2019-08-07 09:39:20 PDT
Created attachment 375704 [details]
WIP Patch
Comment 2 Build Bot 2019-08-07 09:40:54 PDT
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.
Comment 3 Chris Dumez 2019-08-07 09:51:56 PDT
Created attachment 375706 [details]
WIP Patch
Comment 4 Build Bot 2019-08-07 09:52:58 PDT
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 5 Build Bot 2019-08-07 11:00:17 PDT
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.
Comment 6 Build Bot 2019-08-07 11:00:19 PDT
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
Comment 7 Chris Dumez 2019-08-07 12:02:58 PDT
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 8 Build Bot 2019-08-07 12:15:01 PDT
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.
Comment 9 Build Bot 2019-08-07 12:15:03 PDT
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
Comment 10 Chris Dumez 2019-08-07 12:38:03 PDT
Created attachment 375732 [details]
WIP Patch
Comment 11 Build Bot 2019-08-07 12:40:27 PDT
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.
Comment 12 Chris Dumez 2019-08-07 12:53:03 PDT
Created attachment 375735 [details]
WIP Patch
Comment 13 Build Bot 2019-08-07 12:54:22 PDT
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 14 Build Bot 2019-08-07 14:04:07 PDT
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.
Comment 15 Build Bot 2019-08-07 14:04:09 PDT
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 16 Build Bot 2019-08-07 14:51:13 PDT
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.
Comment 17 Build Bot 2019-08-07 14:51:15 PDT
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 18 Build Bot 2019-08-07 16:33:07 PDT
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 19 Build Bot 2019-08-07 18:16:07 PDT
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
Comment 20 Build Bot 2019-08-07 18:16:10 PDT
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 21 Build Bot 2019-08-07 21:38:17 PDT
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
Comment 22 Build Bot 2019-08-07 21:38:19 PDT
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
Comment 23 Chris Dumez 2019-08-08 08:28:39 PDT
Created attachment 375804 [details]
WIP Patch
Comment 24 Build Bot 2019-08-08 08:30:03 PDT
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.
Comment 25 Chris Dumez 2019-08-08 08:30:30 PDT
Created attachment 375805 [details]
WIP Patch
Comment 26 Build Bot 2019-08-08 08:31:55 PDT
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 27 Build Bot 2019-08-08 10:12:23 PDT
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
Comment 28 Build Bot 2019-08-08 10:12:25 PDT
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 29 Build Bot 2019-08-08 10:32:06 PDT
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 30 Build Bot 2019-08-08 10:59:48 PDT
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
Comment 31 Build Bot 2019-08-08 10:59:50 PDT
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
Comment 32 Chris Dumez 2019-08-08 11:43:30 PDT
Created attachment 375828 [details]
WIP Patch
Comment 33 Build Bot 2019-08-08 11:46:47 PDT
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 34 Build Bot 2019-08-08 13:50:09 PDT
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
Comment 35 Chris Dumez 2019-08-08 15:57:57 PDT
Created attachment 375849 [details]
WIP Patch
Comment 36 Build Bot 2019-08-08 16:01:18 PDT
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 37 Build Bot 2019-08-08 18:07:14 PDT
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
Comment 38 Chris Dumez 2019-08-09 15:55:48 PDT
Created attachment 375973 [details]
WIP Patch
Comment 39 Build Bot 2019-08-09 15:58:07 PDT
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.
Comment 40 Chris Dumez 2019-08-09 18:16:17 PDT
Created attachment 375990 [details]
WIP Patch
Comment 41 Build Bot 2019-08-09 18:18:09 PDT
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 42 Build Bot 2019-08-09 21:02:21 PDT
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
Comment 43 Chris Dumez 2019-08-10 19:08:12 PDT
(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.
Comment 44 Chris Dumez 2019-08-10 20:49:05 PDT
Created attachment 376030 [details]
WIP Patch
Comment 45 Build Bot 2019-08-10 20:51:20 PDT
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.
Comment 46 Chris Dumez 2019-08-11 09:04:59 PDT
Created attachment 376043 [details]
Patch
Comment 47 Ryosuke Niwa 2019-08-11 16:59:36 PDT
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.
Comment 48 Chris Dumez 2019-08-11 17:07:31 PDT
(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.
Comment 49 Chris Dumez 2019-08-11 17:20:33 PDT
Created attachment 376051 [details]
Patch
Comment 50 WebKit Commit Bot 2019-08-11 19:00:36 PDT
Comment on attachment 376051 [details]
Patch

Clearing flags on attachment: 376051

Committed r248525: <https://trac.webkit.org/changeset/248525>
Comment 51 WebKit Commit Bot 2019-08-11 19:00:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 Radar WebKit Bug Importer 2019-08-11 19:01:44 PDT
<rdar://problem/54191314>
Comment 55 Chris Dumez 2019-08-12 09:06:09 PDT
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>
Comment 56 Chris Dumez 2019-08-12 09:46:25 PDT
(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.
Comment 57 Chris Dumez 2019-08-12 09:47:59 PDT
(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.
Comment 58 Chris Dumez 2019-08-12 09:52:10 PDT
Created attachment 376071 [details]
Patch
Comment 59 WebKit Commit Bot 2019-08-12 11:10:33 PDT
Comment on attachment 376071 [details]
Patch

Clearing flags on attachment: 376071

Committed r248533: <https://trac.webkit.org/changeset/248533>
Comment 60 WebKit Commit Bot 2019-08-12 11:10:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 61 Darin Adler 2019-08-12 12:45:36 PDT
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 62 Darin Adler 2019-08-12 12:52:42 PDT
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");
Comment 63 Chris Dumez 2019-08-12 13:15:58 PDT
Applied Darin's post landing review comments in <https://trac.webkit.org/changeset/248544>.
Comment 64 Chris Dumez 2019-08-12 13:16:22 PDT
(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.