226885 – crash when openDBRequest is null

RESOLVED FIXED 226885

crash when openDBRequest is null

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

Summary crash when openDBRequest is null

Venky Dass

Reported 2021-06-10 10:57:21 PDT

rdar://78703857

Attachments
Patch (4.35 KB, patch) 2021-06-10 12:25 PDT, Venky Dass	no flags	Details Formatted Diff Diff
Patch (5.96 KB, patch) 2021-06-15 22:33 PDT, Venky Dass	no flags	Details Formatted Diff Diff
Patch (5.65 KB, patch) 2021-06-17 10:45 PDT, Venky Dass	no flags	Details Formatted Diff Diff
Patch (5.73 KB, patch) 2021-06-17 13:18 PDT, Venky Dass	no flags	Details Formatted Diff Diff
Patch (5.38 KB, patch) 2021-06-22 11:21 PDT, Venky Dass	no flags	Details Formatted Diff Diff
Patch (5.13 KB, patch) 2021-06-24 09:26 PDT, Venky Dass	no flags	Details Formatted Diff Diff
Patch (5.13 KB, patch) 2021-06-24 14:06 PDT, Venky Dass	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2021-06-10 10:57:34 PDT

<rdar://problem/79149041>

Venky Dass

Comment 2 2021-06-10 12:25:56 PDT

Created attachment 431108 [details] Patch

Sam Weinig

Comment 3 2021-06-14 09:12:45 PDT

Comment on attachment 431108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431108&action=review > LayoutTests/editing/inserting/insert-horizontal-rule-in-empty-document-crash.html:22 > +https://slack-imgs.com/?c=1&o1=gu&url=https%3A%2F%2Fa.slack-edge.com%2Fproduction-standard-emoji-assets%2F13.0%2Fapple-small%2F1f3e0%402x.png This seems unrelated.

Sihui Liu

Comment 4 2021-06-14 09:52:54 PDT

Comment on attachment 431108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431108&action=review > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:604 > + if (isVersionChange() && m_openDBRequest) { In IDBTransaction::dispatchEvent, the assumption is the event is either complete or abort: ASSERT(event.type() == eventNames().completeEvent || event.type() == eventNames().abortEvent); and the event is created by IDBTransaction itself (using IDBTransaction::enqueueEvent), so the block here is to finish version change transaction on receiving events. Based on the test, the assumption is incorrect: you can dispatch any event to IDBTransaction, e.g. transaction.dispatchEvent(new Event('select')). The complete fix should probably be invoking the block only when the event is type complete or abort, and it is dispatched by IDBTransaction because of state change. Like storing events in IDBTransaction::enqueueEvent, and checking if it is the same event in IDBTransaction::dispatchEvent after EventDispatcher::dispatchEvent({ this, m_database.ptr() }, event);. If it's not, return without invoking the block or setting m_didDispatchAbortOrCommit. > LayoutTests/editing/inserting/request_with_null_open_db_request.html:1 > +<!DOCTYPE html> The test should probably be put in LayoutTests/storage/indexeddb.

Venky Dass

Comment 5 2021-06-15 22:33:02 PDT

Created attachment 431518 [details] Patch

Ryosuke Niwa

Comment 6 2021-06-15 22:45:32 PDT

Comment on attachment 431518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431518&action=review > LayoutTests/ChangeLog:9 > + * storage/indexeddb/request_with_null_open_db_request.html: Added. Please use - instead of _ for the delimiter. > LayoutTests/platform/mac/editing/inserting/request_with_null_open_db_request-expected.txt:1 > +layer at (0,0) size 800x600 Please call dumpAsText in the test to avoid platform specific test results. > LayoutTests/storage/indexeddb/request_with_null_open_db_request.html:4 > + globalThis.testRunner?.waitUntilDone(); Neat!

Sihui Liu

Comment 7 2021-06-16 10:17:36 PDT

Comment on attachment 431518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431518&action=review > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:316 > + m_queuedEvents.clear(); Do we need to clear the events here? > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:588 > + m_queuedEvents.append(event.copyRef()); WeakPtr should be enough. > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:-602 > - m_didDispatchAbortOrCommit = true; I think this one should be kept outside of the block, so it should be something like (if we use WeakPtr): // Clear first null events in m_queuedEvents // Compare first non-null event in m_queuedEvents with event; if different, return // Remove first non-null event m_didDispatchAbortOrCommit = true; if (isVersionChange() { ... } > Source/WebCore/Modules/indexeddb/IDBTransaction.h:253 > + Deque<RefPtr<Event>> m_queuedEvents; WeakPtr<> should be enough

Ryosuke Niwa

Comment 8 2021-06-16 14:17:16 PDT

(In reply to Sihui Liu from comment #7) > Comment on attachment 431518 [details] > Patch > > > Source/WebCore/Modules/indexeddb/IDBTransaction.h:253 > > + Deque<RefPtr<Event>> m_queuedEvents; > > WeakPtr<> should be enough We can't create WeakPtr of an Event.

Venky Dass

Comment 9 2021-06-17 10:45:22 PDT

Created attachment 431689 [details] Patch

Ryosuke Niwa

Comment 10 2021-06-17 12:57:20 PDT

Comment on attachment 431689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431689&action=review > LayoutTests/storage/indexeddb/request_with_null_open_db_request.html:4 > + globalThis.testRunner?.waitUntilDone(); Looks like you modified the expected result but not the test?

Venky Dass

Comment 11 2021-06-17 13:18:35 PDT

Created attachment 431713 [details] Patch

Ryosuke Niwa

Comment 12 2021-06-17 13:22:49 PDT

Comment on attachment 431713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431713&action=review > LayoutTests/storage/indexeddb/request_with_null_open_db_request.html:7 > + if (window.testRunner) > + testRunner.dumpAsText(); > + > + globalThis.testRunner?.waitUntilDone(); You can do this instead: globalThis.testRunner?.dumpAsText(); globalThis.testRunner?.waitUntilDone(); Or: if (window.testRunner) { testRunner.dumpAsText(); testRunner.waitUntilDone(); }

Sihui Liu

Comment 13 2021-06-21 09:50:49 PDT

Comment on attachment 431713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431713&action=review > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:316 > + m_queuedEvents.clear(); Thinking about this again, IDBTransaction should not generate two abort or commit events, so we only need to store one event, instead of a queue: like Event* m_abortOrCommitEvent. And you don't need to clear it. > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:588 > + m_queuedEvents.append(event.copyRef()); And we can change this to m_abortOrCommitEvent = event.ptr(); > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:607 > - m_didDispatchAbortOrCommit = true; > + > > - if (isVersionChange()) { > - ASSERT(m_openDBRequest); > + if (isVersionChange() && (event.type() == eventNames().completeEvent || event.type() == eventNames().abortEvent) && (&event == m_queuedEvents.takeFirst())) { > + m_didDispatchAbortOrCommit = true; This is not right as I mentioned in last review; m_didDispatchAbortOrCommit should be set no matter it's versionchange or not. So it should be like: if (m_abortOrCommitEvent != &event) return; m_abortOrCommitEvent = nullptr; ASSERT(event.type() == eventNames().completeEvent || event.type() == eventNames().abortEvent); m_didDispatchAbortOrCommit = true; if (isVersionChange()) { // unchanged }

Venky Dass

Comment 14 2021-06-22 11:21:28 PDT

Created attachment 431980 [details] Patch

Sihui Liu

Comment 15 2021-06-23 14:12:24 PDT

Comment on attachment 431980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431980&action=review Looks good to me with two nits > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:611 > + m_didDispatchAbortOrCommit = true; This is redundant: you've set m_didDispatchAbortOrCommit above. > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:623 > + Extra line?

Venky Dass

Comment 16 2021-06-24 09:26:13 PDT

Created attachment 432173 [details] Patch

Ryosuke Niwa

Comment 17 2021-06-24 13:43:22 PDT

Comment on attachment 432173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432173&action=review > LayoutTests/ChangeLog:9 > + * storage/indexeddb/request_with_null_open_db_request-expected.txt: Added. > + * storage/indexeddb/request_with_null_open_db_request.html: Added. Please use - instead of _ as a delimiter.

Venky Dass

Comment 18 2021-06-24 14:06:47 PDT

Created attachment 432214 [details] Patch

EWS

Comment 19 2021-06-24 17:06:13 PDT

Committed r279255 (239139@main): <https://commits.webkit.org/239139@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432214 [details].

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Local Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Nobody

Reported

2021-06-10 10:57 PDT

Modified

2021-06-24 17:06 PDT History

CC List

9 users Show

URL

Keywords InRadar

Depends on

Blocks