rdar://78703857
<rdar://problem/79149041>
Created attachment 431108 [details] Patch
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.
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.
Created attachment 431518 [details] Patch
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!
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
(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.
Created attachment 431689 [details] Patch
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?
Created attachment 431713 [details] Patch
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(); }
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 }
Created attachment 431980 [details] Patch
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?
Created attachment 432173 [details] Patch
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.
Created attachment 432214 [details] Patch
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].