Bug 200451 – Tighten WeakPtr threading assertions for GC threads

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

[Chris Dumez]

Tighten WeakPtr threading assertions for GC threads. We're currently overly permissive for GC threads.

[Chris Dumez]

Created attachment 375575 [details]
WIP Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 375712 [details]
WIP Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 375726 [details]
Patch

[Chris Dumez]

Comment on attachment 375726 [details]
Patch

Ready for review.

[youenn fablet]

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.

[Chris Dumez]

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

[WebKit Commit Bot]

Comment on attachment 375726 [details]
Patch

Clearing flags on attachment: 375726

Committed r248386: <https://trac.webkit.org/changeset/248386>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54048244>

[Darin Adler]

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.

[Ryosuke Niwa]

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