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

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