Bug 211669

Summary:

Add iterator checking to ListHashSet

Product:

WebKit

Reporter:

Darin Adler <darin>

Component:

New Bugs

Assignee:

Darin Adler <darin>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, benjamin, cdumez, cmarcelo, ews-watchlist, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=211733

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Darin Adler

Reported 2020-05-09 12:37:03 PDT

Add iterator checking to ListHashSet

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 attachment proposed patch, testcase, etc.

Darin Adler

Comment 1 2020-05-09 12:39:33 PDT

Created attachment 398934 [details] Patch

Anders Carlsson

Comment 2 2020-05-09 13:07:03 PDT

Comment on attachment 398934 [details] Patch Is WeakPtr thread safe?

Darin Adler

Comment 3 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.

EWS

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

Radar WebKit Bug Importer

Comment 5 2020-05-09 15:30:14 PDT

<rdar://problem/63057348>

Yusuke Suzuki

Comment 6 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.

Ryan Haddad

Comment 7 2020-05-11 12:07:22 PDT

Reverted r261440 for reason: Caused 6 TestWTF.WTF failures Committed r261486: <https://trac.webkit.org/changeset/261486>

Darin Adler

Comment 8 2020-05-11 20:24:45 PDT Comment hidden (obsolete)

Created attachment 399092 [details] Patch

Darin Adler

Comment 9 2020-05-11 20:25:10 PDT

This new patch fixes the TestWTF.WTF failures. Would appreciate review since more has changed now.

Darin Adler

Comment 10 2020-05-11 23:04:33 PDT

Created attachment 399100 [details] Patch

EWS

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

Note You need to log in before you can comment on or make changes to this bug.