Tighten WeakPtr threading assertions for GC threads. We're currently overly permissive for GC threads.
Created attachment 375575 [details] WIP Patch
Attachment 375575 [details] did not pass style-queue: ERROR: Source/WTF/wtf/WeakPtr.h:71: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:73: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:78: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:80: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:86: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:88: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 6 in 1 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375575 [details] WIP Patch Attachment 375575 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12866226 New failing tests: imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/007.html imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/history_go_plus.html imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/history_forward.html imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/history_go_minus.html imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/combination_history_004.html
Created attachment 375587 [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 375575 [details] WIP Patch Attachment 375575 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12866608 New failing tests: imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/007.html
Created attachment 375595 [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 375712 [details] WIP Patch
Attachment 375712 [details] did not pass style-queue: ERROR: Source/WTF/wtf/WeakPtr.h:71: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:73: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:78: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:80: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:86: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WTF/wtf/WeakPtr.h:88: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 6 in 1 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 375726 [details] Patch
Comment on attachment 375726 [details] Patch Ready for review.
Comment on attachment 375726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375726&action=review > Source/WTF/wtf/WeakPtr.h:102 > + // FIXME: Our GC threads currently need to get opaque pointers from WeakPtrs and have to be special-cased. Do we have a plan to further tighten things? It seems fine to get access to a value stored in a DOM object from GC thread, be it an integer, a raw pointer or a weak pointer.
(In reply to youenn fablet from comment #11) > Comment on attachment 375726 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375726&action=review > > > Source/WTF/wtf/WeakPtr.h:102 > > + // FIXME: Our GC threads currently need to get opaque pointers from WeakPtrs and have to be special-cased. > > Do we have a plan to further tighten things? > It seems fine to get access to a value stored in a DOM object from GC > thread, be it an integer, a raw pointer or a weak pointer. The thing is that somebody may dereference the pointer returned by get() later on, which would not be safe. I do not know if we have plans to tighten things further but Geoff said we might and suggested I put a comment when I discussed it with him.
Comment on attachment 375726 [details] Patch Clearing flags on attachment: 375726 Committed r248386: <https://trac.webkit.org/changeset/248386>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54048244>
Idea for long term: We could consider creating a special type (or struct with just a pointer data member) for "pointer that can only be used to make GC decisions and shouldn't be used in any other way" and we could return that from a function like get, but with a different name. That special type would also help us express "this is the right pointer for GC liveness checking, not a pointer further into an object" and also "don't remember exactly what type of object this is, since you can't make good use of that". Some people don't like using types for something that's just a pointer, but I think there ways this could make code more foolproof if the idea works.
(In reply to Darin Adler from comment #16) > Idea for long term: We could consider creating a special type (or struct > with just a pointer data member) for "pointer that can only be used to make > GC decisions and shouldn't be used in any other way" and we could return > that from a function like get, but with a different name. That special type > would also help us express "this is the right pointer for GC liveness > checking, not a pointer further into an object" and also "don't remember > exactly what type of object this is, since you can't make good use of that". > Some people don't like using types for something that's just a pointer, but > I think there ways this could make code more foolproof if the idea works. Yeah, that would be a great change in long term. I think for short term, we can add a special version of WeakPtr::get which asserts that it's either called in the GC thread or during a pause-the-main-thread phase of GC, and name it accordingly.