Bug 200925

Summary:

WebSQLiteDatabaseTracker does not ensure it is still alive after dispatching to the main thread

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

WebKit2

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, beidson, benjamin, cmarcelo, commit-queue, dbates, ews-watchlist, ggaren, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Chris Dumez

Reported 2019-08-20 10:32:02 PDT

WebSQLiteDatabaseTracker does not ensure it is still alive after dispatching to the main thread, which is not safe.

Attachments
Patch (3.65 KB, patch) 2019-08-20 10:32 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (4.90 KB, patch) 2019-08-20 12:39 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (6.19 KB, patch) 2019-08-20 13:36 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2019-08-20 10:32:58 PDT

Created attachment 376781 [details] Patch

Geoffrey Garen

Comment 2 2019-08-20 10:48:16 PDT

Comment on attachment 376781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376781&action=review r=me > Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp:39 > + : m_weakThis(makeWeakPtr(*this)) This data member is just a copy of the pointer held by CanMakeWeakPtr<WebSQLiteDatabaseTracker>. It would be slightly more efficient to remove this data member, and instead do [weakThis = makeWeakPtr(*this)] in our lambda capture expressions. If there's an issue where it's not safe to call makeWeakPtr(*this) in our lambda capture expressions because they execute on another thread, then I'd recommend just calling makeWeakPtr(*this) here but not storing the result, with a comment explaining that we want to avoid initializing our weak pointer on a secondary thread.

Chris Dumez

Comment 3 2019-08-20 12:39:38 PDT

Created attachment 376791 [details] Patch

Chris Dumez

Comment 4 2019-08-20 12:40:07 PDT

(In reply to Chris Dumez from comment #3) > Created attachment 376791 [details] > Patch Ok, what do you think of this approach?

EWS Watchlist

Comment 5 2019-08-20 12:42:22 PDT

Attachment 376791 [details] did not pass style-queue: ERROR: Source/WTF/wtf/WeakPtr.h:159: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Geoffrey Garen

Comment 6 2019-08-20 12:45:24 PDT

Comment on attachment 376791 [details] Patch cq+

Geoffrey Garen

Comment 7 2019-08-20 12:50:17 PDT

Comment on attachment 376791 [details] Patch cq- because we came up with an even better idea in person

Chris Dumez

Comment 8 2019-08-20 13:36:23 PDT

Created attachment 376797 [details] Patch

WebKit Commit Bot

Comment 9 2019-08-20 15:18:29 PDT

Comment on attachment 376797 [details] Patch Clearing flags on attachment: 376797 Committed r248918: <https://trac.webkit.org/changeset/248918>