Bug 211669 - Add iterator checking to ListHashSet
Summary: Add iterator checking to ListHashSet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-09 12:37 PDT by Darin Adler
Modified: 2020-05-18 10:16 PDT (History)
7 users (show)

See Also:

Attachments
Patch (4.22 KB, patch)
2020-05-09 12:39 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (22.63 KB, patch)
2020-05-11 20:24 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (23.98 KB, patch)
2020-05-11 23:04 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-05-09 12:37:03 PDT 
Add iterator checking to ListHashSet
Comment 1 Darin Adler 2020-05-09 12:39:33 PDT 
Created attachment 398934 [details]
Patch
Comment 2 Anders Carlsson 2020-05-09 13:07:03 PDT 
Comment on attachment 398934 [details]
Patch

Is WeakPtr thread safe?
Comment 3 Darin Adler 2020-05-09 13:48:26 PDT 
(In reply to Anders Carlsson from comment #2)
> Is WeakPtr thread safe?

It uses ThreadSafeRefCounted, so I think yes.
Comment 4 EWS 2020-05-09 15:29:55 PDT 
Committed r261440: <https://trac.webkit.org/changeset/261440>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398934 [details].
Comment 5 Radar WebKit Bug Importer 2020-05-09 15:30:14 PDT 
<rdar://problem/63057348>
Comment 6 Yusuke Suzuki 2020-05-09 15:47:11 PDT 
(In reply to Darin Adler from comment #3)
> (In reply to Anders Carlsson from comment #2)
> > Is WeakPtr thread safe?
> 
> It uses ThreadSafeRefCounted, so I think yes.

WeakPtr<T>::get returns weird results in multi-threaded environment. This is slightly different from `std::weak_ptr<T>::lock()`.
So, in general, it is not encouraged that we use WeakPtr<T> in multi-threading use because it does not work as expected.
However, in this use case, it is OK.

1. T inherits CanMakeWeakPtr<T> and ThreadSafeRefCounted<T>.
2. In thread A, we create Ref<T>.
3. In thread A, we create WeakPtr<T> from Ref<T>
4. We pass WeakPtr<T> to another thread B.
5. B thread is attempting to call WeakPtr<T>::get().
6. At the same time, in thread A, we start destroying Ref<T>.
7. Before (6) calls WeakPtrImpl::clear(), (5) gets a non nullptr T*.
8. Just after (5) gets non nullptr T*, (7) destroys Ref<T> completely.

WeakPtr<T>::get can return a non-nullptr even if the obtained T* is half-destroyed right now.
So, it is possible that we could miss some cases that T* is already destroyed while WeakPtr<T>::get returns non-nullptr.
But since this is for assertion, false-negative is OK.
Comment 7 Ryan Haddad 2020-05-11 12:07:22 PDT 
Reverted r261440 for reason:

Caused 6 TestWTF.WTF failures

Committed r261486: <https://trac.webkit.org/changeset/261486>
Comment 8 Darin Adler 2020-05-11 20:24:45 PDT Comment hidden (obsolete)
Comment 9 Darin Adler 2020-05-11 20:25:10 PDT 
This new patch fixes the TestWTF.WTF failures. Would appreciate review since more has changed now.
Comment 10 Darin Adler 2020-05-11 23:04:33 PDT 
Created attachment 399100 [details]
Patch
Comment 11 EWS 2020-05-18 10:16:20 PDT 
Committed r261818: <https://trac.webkit.org/changeset/261818>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399100 [details].