RESOLVED FIXED 188728
Various IndexDB tests abandon documents
https://bugs.webkit.org/show_bug.cgi?id=188728
Summary Various IndexDB tests abandon documents
Simon Fraser (smfr)
Reported 2018-08-18 17:06:11 PDT
Tests like imported/w3c/IndexedDB-private-browsing/cursor-overloads.html, imported/w3c/IndexedDB-private-browsing/idbcursor_advance_index.html, and others abandon the main document (bug 186214). I don't see any suspicious-looking retain stacks in C++, so I suspect this might be a GC-world issue.
Attachments
Patch (16.92 KB, patch)
2018-08-24 09:52 PDT, youenn fablet
no flags
Patch (15.00 KB, patch)
2018-08-24 10:57 PDT, youenn fablet
no flags
Patch (15.82 KB, patch)
2018-08-24 11:51 PDT, youenn fablet
no flags
Rebased (14.58 KB, patch)
2018-08-27 12:36 PDT, youenn fablet
no flags
Patch for landing (14.67 KB, patch)
2018-08-27 15:59 PDT, youenn fablet
youennf: commit-queue+
Simon Fraser (smfr)
Comment 1 2018-08-22 15:17:49 PDT
A bit confused by this one. The Document is released in MiniBrowser (by an EventHandler being GC'd), but never in WebKitTestRunner even when followed by other tests and GCing.
Simon Fraser (smfr)
Comment 2 2018-08-22 18:51:03 PDT
GC heap inspector to the rescue: HTMLDocument cell 0x42775c060 wrapped 0x42c202400 “http://localhost:8800/IndexedDB/value.htm”Show all paths—3 steps from IDBOpenDBRequest cell 0x42775c3f0 wrapped 0x42d97c3d0 (GC root—Weak sets, ActiveDOMObject with pending activity) IDBOpenDBRequest cell 0x42775c3f0 wrapped 0x42d97c3d0 (GC root—Weak sets, ActiveDOMObject with pending activity)Internal 0 Structure cell 0x42d7a0500 “IDBOpenDBRequest”Internal 0 Window cell 0x4277d8fb0 Variable document HTMLDocument cell 0x42775c060 wrapped 0x42c202400 “http://localhost:8800/IndexedDB/value.htm
Simon Fraser (smfr)
Comment 3 2018-08-22 19:08:19 PDT
The IDBRequest that is always active is a "blocked" event.
Simon Fraser (smfr)
Comment 4 2018-08-22 19:39:07 PDT
The test triggers the call to IDBConnectionToServer::deleteDatabase(), but we never get the IDBConnectionToServer::didDeleteDatabase() message back (unlike in MiniBrowser).
Simon Fraser (smfr)
Comment 5 2018-08-22 20:54:29 PDT
I think what's happening here is this: 1. The test runs, and says it's done, but there are still a bunch of DB operations pending. 2. In WebKtTestRunner, we immediately load about:blank. This load causes us to call IDBRequest::stop() on several IDB requests: 1 0x1f1621f75 WebCore::IDBRequest::stop() 2 0x1f219fbd5 auto WebCore::ScriptExecutionContext::stopActiveDOMObjects()::$_5::operator()<WebCore::ActiveDOMObject>(WebCore::ActiveDOMObject&) const 3 0x1f219fb84 WTF::Function<WebCore::ScriptExecutionContext::ShouldContinue (WebCore::ActiveDOMObject&)>::CallableWrapper<WebCore::ScriptExecutionContext::stopActiveDOMObjects()::$_5>::call(WebCore::ActiveDOMObject&) 4 0x1f2187060 WTF::Function<WebCore::ScriptExecutionContext::ShouldContinue (WebCore::ActiveDOMObject&)>::operator()(WebCore::ActiveDOMObject&) const 5 0x1f2186cb3 WebCore::ScriptExecutionContext::forEachActiveDOMObject(WTF::Function<WebCore::ScriptExecutionContext::ShouldContinue (WebCore::ActiveDOMObject&)> const&) const 6 0x1f2187294 WebCore::ScriptExecutionContext::stopActiveDOMObjects() 7 0x1f1fea655 WebCore::Document::stopActiveDOMObjects() 8 0x1f1fe9e8c WebCore::Document::prepareForDestruction() 9 0x1f2a0d620 WebCore::Frame::setView(WTF::RefPtr<WebCore::FrameView, WTF::DumbPtrTraits<WebCore::FrameView> >&&) 10 0x1f2a1194b WebCore::Frame::createView(WebCore::IntSize const&, WebCore::Color const&, bool, WebCore::IntSize const&, WebCore::IntRect const&, bool, WebCore::ScrollbarMode, bool, WebCore::ScrollbarMode, bool) 11 0x10b9039da WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage() 12 0x1f2846955 WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) 13 0x1f2845990 WebCore::FrameLoader::commitProvisionalLoad() 14 0x1f27ea8bc WebCore::DocumentLoader::commitIfReady() 3. We get to IDBConnectionToServer::didDeleteDatabase() with IDBResultType::DeleteDatabaseSuccess, but m_contextStopped is true, so we never enqueue a "success" event. This leaves us in a state where a "blocked" event is never cleared, and we have an ActiveDOMObject which remains active, which keeps the Document alive. Maybe IDBRequest::stop() should set m_hasPendingActivity to false?
Radar WebKit Bug Importer
Comment 6 2018-08-23 11:30:06 PDT
Simon Fraser (smfr)
Comment 7 2018-08-23 11:57:15 PDT
How to reproduce in MiniBrowser: 1. Apply this patch to the test: diff --git a/LayoutTests/imported/w3c/web-platform-tests/IndexedDB/value.htm b/LayoutTests/imported/w3c/web-platform-tests/IndexedDB/value.htm index 36e8339524852590d52ce2275a9f0c0f372f6d31..397ca733f74360321db7dd4ae042ad718615d923 100644 --- a/LayoutTests/imported/w3c/web-platform-tests/IndexedDB/value.htm +++ b/LayoutTests/imported/w3c/web-platform-tests/IndexedDB/value.htm @@ -27,6 +27,7 @@ function value(value, _instanceof) { { assert_true(e.target.result instanceof _instanceof, "instanceof") t.done(); + window.location.href = 'about:blank'; }); }); }; 2. Start the http server: ./Tools/Scripts/run-webkit-httpd 3. Load the test in MiniBrowser: http://localhost:8800/IndexedDB/value.htm 4. Fire a memory warning (in Terminal: "notifyutil -p org.WebKit.lowMemory") 5. Dump the list of live documents (in Terminal: "notifyutil -p com.apple.WebKit.showAllDocuments") http://localhost:8800/IndexedDB/value.htm will be listed there.
youenn fablet
Comment 8 2018-08-23 15:24:04 PDT
Several IDB objects implement hasPendingActivity. IDBRequest and IDBCursor at least may still returning true to hasPendingActivity even when stopped. This should probably not be the case.
youenn fablet
Comment 9 2018-08-24 09:52:36 PDT
Simon Fraser (smfr)
Comment 10 2018-08-24 10:11:10 PDT
Comment on attachment 348018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348018&action=review > Source/WebCore/testing/Internals.idl:668 > + sequence<DOMString> documentURLs(); I don't think you need to add this. Instead, use: unsigned long long documentIdentifier(Document document); boolean isDocumentAlive(unsigned long long documentIdentifier);
youenn fablet
Comment 11 2018-08-24 10:36:33 PDT
Doing some more code inspection, I can see several reasons why IDBRequest::m_hasPendingActivity would remain true: 1. IDBOpenDBRequest::requestCompleted is called after the request is stopped. We then never set back m_hasPendingActivity to false since we do not want to call enqueueEvent. This could be fixed by adding something like a IDBRequest::requestCompletedAfterContextStopped to do the cleanup. 2. IDBRequest::dispatchEvent has a code path where it will reset m_hasPendingActivity to true (in case of upgrade needed or blocked event). I do not see how in that case we can ensure that a new transaction will be created so that IDBRequest::dispatchEvent would be called again to set m_hasPendingActivity to false again. The fact that m_hasPendingActivity is true by default is also a potential source of issue but from what I see, we are always creating a request and then doing some transaction, so this does not seem to be a possible cause now. There is the context of a TransactionOperation that has a ref to a IDBRequest. Maybe IDBRequest::m_hasPendingActivity should be true as long as a TransactionOperation holds a ref to it. That would require to better understand case 2.
youenn fablet
Comment 12 2018-08-24 10:36:55 PDT
(In reply to Simon Fraser (smfr) from comment #10) > Comment on attachment 348018 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348018&action=review > > > Source/WebCore/testing/Internals.idl:668 > > + sequence<DOMString> documentURLs(); > > I don't think you need to add this. Instead, use: > > unsigned long long documentIdentifier(Document document); > boolean isDocumentAlive(unsigned long long documentIdentifier); ok, will do.
youenn fablet
Comment 13 2018-08-24 10:42:20 PDT
To make code more robust, we could envision to make all ActiveDOMObjects be collectable once their context is stopped/preparing for destruction. That could be done in the binding generator, JSXXOwner::isReachableFromOpaqueRoots. Looking at all ActiveDOMObject reimplementing hasPendingActivity, they seem to follow this strategy. For the other ActiveDOMObjects, they usually rely on setPendingActivity/unsetPendingActivity so need to make sure they mirror setPendingActivity calls with un setPendingActivity calls.
youenn fablet
Comment 14 2018-08-24 10:57:11 PDT
youenn fablet
Comment 15 2018-08-24 11:51:49 PDT
Simon Fraser (smfr)
Comment 16 2018-08-24 12:54:14 PDT
Comment on attachment 348031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348031&action=review > Source/WebCore/ChangeLog:9 > + Some IDB objects implement hasPendingActivity but there are some possibilities that they continue returning true after being stopped. I think you should explain these scenarios in more detail here. > Source/WebCore/ChangeLog:11 > + This ensures that they can be garbage collected once their document is preparing for destruction. The dependency is the other way around: they have to be GC'd before the Document can be destroyed. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:372 > + return !m_contextStopped && m_outstandingRequestCount; Don't we need to wait for outstanding requests to complete (either after finishing, or being canceled)? I'm worried that allowing IDBCursor's to be GC'd while things are still happening could corrupt databases. > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:269 > + return !m_contextStopped && m_hasPendingActivity; Ditto.
youenn fablet
Comment 17 2018-08-24 13:51:25 PDT
> > Source/WebCore/ChangeLog:9 > > + Some IDB objects implement hasPendingActivity but there are some possibilities that they continue returning true after being stopped. > > I think you should explain these scenarios in more detail here. OK > > Source/WebCore/ChangeLog:11 > > + This ensures that they can be garbage collected once their document is preparing for destruction. > > The dependency is the other way around: they have to be GC'd before the > Document can be destroyed. I was meaning Document::prepareForDestruction, not Document destructor. > > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:372 > > + return !m_contextStopped && m_outstandingRequestCount; > > Don't we need to wait for outstanding requests to complete (either after > finishing, or being canceled)? I'm worried that allowing IDBCursor's to be > GC'd while things are still happening could corrupt databases. AFAIUI, the point of hasPendingActivity implementation here is that the objects will not be GCed so that we can dispatch their events properly. Since there is no point in dispatching events after they are stopped, the change seems fine to me. IDBCursor and IDBRequest hasPendingActivity implementation is not to ensure objects get not deallocated. For that purpose, other objects are taking Ref to these objects. For instance, nobody is storing a raw reference/pointer to IDBCursor. IDBCursor does take a raw pointer to IDBRequest but this gets cleared in IDBRequest destructor. In general, I do not think we should count on GC to keep the objects alive when we are waiting for some async task. Instead, the async task should take a Ref until being completed. In the case of IDBCursor, we could try to stick with m_outstandingRequestCount only although I do not see a huge benefit there. We would probably just need to clear IDBRequest::m_cursorRequestNotifier in IDBRequest::stop. We could add an ASSERT to make sure that m_outstandingRequestCount is 0 once stopped. > > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:269 > > + return !m_contextStopped && m_hasPendingActivity; > > Ditto. For this one, I do not have a better plan right now. I do not understand IDB enough yet to see why m_hasPendingActivity can be set back to true in IDBRequest::dispatchEvent. Again, my interpretation is that it only has to do with event dispatching, not keeping the C++ object alive.
Alex Christensen
Comment 18 2018-08-27 08:47:19 PDT
Comment on attachment 348031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348031&action=review >> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:372 >> + return !m_contextStopped && m_outstandingRequestCount; > > Don't we need to wait for outstanding requests to complete (either after finishing, or being canceled)? I'm worried that allowing IDBCursor's to be GC'd while things are still happening could corrupt databases. This needs a new patch after making IDBCursor no longer an ActiveDOMObject
youenn fablet
Comment 19 2018-08-27 12:36:42 PDT
Wenson Hsieh
Comment 20 2018-08-27 13:35:40 PDT
Comment on attachment 348188 [details] Rebased View in context: https://bugs.webkit.org/attachment.cgi?id=348188&action=review > Source/WebCore/ChangeLog:11 > + This is also the case for requests that emits upgradenneeded or blocked events. Nit - upgradenneeded!
youenn fablet
Comment 21 2018-08-27 15:55:27 PDT
Doing some more tests, IDBDatabase also has a similar risk, let's handle it in a separate patch.
youenn fablet
Comment 22 2018-08-27 15:59:45 PDT
Created attachment 348226 [details] Patch for landing
WebKit Commit Bot
Comment 23 2018-08-27 16:41:19 PDT
Comment on attachment 348226 [details] Patch for landing Clearing flags on attachment: 348226 Committed r235410: <https://trac.webkit.org/changeset/235410>
WebKit Commit Bot
Comment 24 2018-08-27 16:41:21 PDT
All reviewed patches have been landed. Closing bug.
Truitt Savell
Comment 25 2018-09-04 17:03:49 PDT
Looks like the test http/tests/IndexedDB/collect-IDB-objects.https.html introduced in https://trac.webkit.org/changeset/235410/webkit has been flakey from the start Test History: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FIndexedDB%2Fcollect-IDB-objects.https.html Diff: --- /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/http/tests/IndexedDB/collect-IDB-objects.https-expected.txt +++ /Volumes/Data/slave/highsierra-release-tests-wk1/build/layout-test-results/http/tests/IndexedDB/collect-IDB-objects.https-actual.txt @@ -1,4 +1,4 @@ -PASS Ensuring frame document gets collected after being stopped in the middle of IDB operations +FAIL Ensuring frame document gets collected after being stopped in the middle of IDB operations promise_test: Unhandled rejection with value: "Test failed"
Note You need to log in before you can comment on or make changes to this bug.