IndexedDB: Consolidate two-phase connection to avoid race conditions
Created attachment 150493 [details] Patch
Work in progress - the attached patch builds with DumpRenderTree (with some Chromium webkit support library changes) and all Layout Tests pass. I'm still working my way through the Chromium side changes, then it will be a matter of figuring out a landing sequence for this. The basic pattern is that instead of: IDBFactoy::open(...) { r = IDBRequest::create(); IDBFactoryBackend->open(..., r, ...); } IDBRequest::onSuccess(IDBDatabaseBackend b) { db = IDBDatabase::create(); b->registerFrontend(db->callbacks); } (which leaves nothing registered with the backend in the mean time, so it must be artificially kept alive) We do: IDBFactoy::open(...) { r = IDBRequest::create(); c = IDBDatabaseCallbacks::create(); r.setDBCallbacks(c); IDBFactoryBackend->open(..., r, c, ...); } IDBRequest::onSuccess(IDBDatabaseBackend b) { db = IDBDatabase::create(this->callbacks); } This seems easy in theory, but the multiple layers of indirection mean that each layer ends up with wrappers around the db_callbacks which need to be kept in sync. So... it might make more sense to have the db_callbacks minted during the callback path leading to the IDBRequest::onSuccess call instead.
Hold off on this until the dust settles from http://webkit.org/b/89505
Created attachment 161334 [details] Patch
Created attachment 162360 [details] Patch
Getting close - with the Chromium side http://codereview.chromium.org/10917099/ in place, all tests pass (various chromium tests plus layout tests). Need to dig into the specifics of: * Why this affects layout tests behavior (although the tests are more correct now, that isn't the intent of the patch!) * The "FIXME: Verify that the following is still needed..." note * If the onSuccess -> onError logic in IDBOpenDBRequest::onSuccess is really the best approach * How to split this up for landing :(
(In reply to comment #6) > Getting close - with the Chromium side http://codereview.chromium.org/10917099/ in place, all tests pass (various chromium tests plus layout tests). > > Need to dig into the specifics of: > * Why this affects layout tests behavior (although the tests are more correct now, that isn't the intent of the patch!) Regardless of why, I think we can (and should) split that fix into a separate patch. Let me know if you want me to do so. > * The "FIXME: Verify that the following is still needed..." note Well, we don't HAVE to figure that out now. > * If the onSuccess -> onError logic in IDBOpenDBRequest::onSuccess is really the best approach After seeing it I think I prefer it over sending an IPC to the backend. > * How to split this up for landing :(
Created attachment 162405 [details] dgrogans
Comment on attachment 162405 [details] dgrogans interdiff isn't working, just ignore this patch.
Comment on attachment 162360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162360&action=review > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:472 > + if (m_runningVersionChangeTransaction || connectionCount() > 1) { Reverting this to > 0 makes the layout tests unaffected for me. >0 seems correct as we don't want to fire blocked (above) and immediately schedule the setIntVersionInternal task (below). That this was hidden by correcting the layout tests' other faults makes me think we don't have enough test coverage. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:562 > + for (Deque<RefPtr<PendingOpenWithVersionCall> >::iterator it = m_pendingSecondHalfOpenWithVersionCalls.begin(); it != m_pendingSecondHalfOpenWithVersionCalls.end(); ++it) { I think you can move this loop to a later patch. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:570 > + if (connectionCount() > 1 || m_pendingSecondHalfOpenWithVersionCalls.size()) This second clause looks to be a no-op as the previous loop emptied m_pendingSecondHalfOpenWithVersionCalls. If you remove that loop, which you should be able to do, then I'm not sure what the ramifications of that second clause are. Removing it didn't change the result of any of our existing tests but I'm working on writing one that would bring out a difference. What's your intuition/understanding? > Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:112 > + if (idbDatabase->isClosePending()) { This can be moved to a later patch.
Comment on attachment 162360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162360&action=review >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:472 >> + if (m_runningVersionChangeTransaction || connectionCount() > 1) { > > Reverting this to > 0 makes the layout tests unaffected for me. >0 seems correct as we don't want to fire blocked (above) and immediately schedule the setIntVersionInternal task (below). > > That this was hidden by correcting the layout tests' other faults makes me think we don't have enough test coverage. Good catch. Yeah, makes sense for the tests to be the same. This is probably residue from an earlier iteration where m_databaseCallbacksSet.add(databaseCallbacks) ran earlier. It makes sense for the test to be > 0 with set addition being at the end of this method. >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:562 >> + for (Deque<RefPtr<PendingOpenWithVersionCall> >::iterator it = m_pendingSecondHalfOpenWithVersionCalls.begin(); it != m_pendingSecondHalfOpenWithVersionCalls.end(); ++it) { > > I think you can move this loop to a later patch. The loop doesn't necessarily empty the m_pendingSecondHalfWithVersionCalls, just removes the connection being closed if it was in the halfway state. (A "find" would make more sense than an iteration, but it will stop being a list soon anyway.) So, this is to handle the case where the connection being closed is in the halfway state, i.e. upgradeneeded has fired and the front end has called close() before success fires. close() needs to ensure that onsuccess won't fire, so it clears it from the ...SecondHalf... queue. Without this, it looks like we'll fall into processPendingCalls() which will dispatch an onSuccess() then drop the connection on the floor, which I guess matches the current behavior so we can defer? >> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:570 >> + if (connectionCount() > 1 || m_pendingSecondHalfOpenWithVersionCalls.size()) > > This second clause looks to be a no-op as the previous loop emptied m_pendingSecondHalfOpenWithVersionCalls. If you remove that loop, which you should be able to do, then I'm not sure what the ramifications of that second clause are. Removing it didn't change the result of any of our existing tests but I'm working on writing one that would bring out a difference. What's your intuition/understanding? The if() is asserting that either there are multiple connections (so delete, openWithVersion, setVersion should remain blocked) or that there's still an pending second half unrelated to the close() (which should not be unblocked by the close). That said, after thinking further, I think we claim: ASSERT((connectionCount() == 1) == (m_pendingSecondHalfOpenWithVersionCalls.size() == 1)) ... as there should never be pending second half if there are multiple connections. If that's true, then the second half of the if() clause can indeed go away. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:576 > + // FIXME: Verify that the following is still needed even if there are no connections or pending activity: This may be from the forceClose() case, where the back end is told to close even though the front end may not have tidied up yet. >> Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:112 >> + if (idbDatabase->isClosePending()) { > > This can be moved to a later patch. Yeah... not worse than what we're currently doing.
(In reply to comment #11) > > That said, after thinking further, I think we claim: > > ASSERT((connectionCount() == 1) == (m_pendingSecondHalfOpenWithVersionCalls.size() == 1)) > .. er, not quite: if (m_pendingSecondHalfOpenWithVersionCalls.size() == 1) { ASSERT(connectionCount() == 1) }
Created attachment 162541 [details] Patch
Landing sequence is: (1) Update Chromium to handle old/new WK API: https://chromiumcodereview.appspot.com/10917099 (2) Land WK change (this): https://bugs.webkit.org/show_bug.cgi?id=90411 (3) Remove back-compat shims from Chromium (4) Remove back-compat shims from WebKit/chromium/public
Comment on attachment 162541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162541&action=review Substantively I'd say this and the cr patch LGTM. Just the question of breaking up for landing remains. > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:570 > + if (!connectionCount() && !m_pendingOpenCalls.size() && !m_pendingOpenWithVersionCalls.size() && !m_pendingDeleteCalls.size()) { Could you add a FIXME saying to add a test for !m_pendingOpenCalls.size() && !m_pendingOpenWithVersionCalls.size() ? Removing those doesn't make any of our tests fail.
(In reply to comment #14) (0) Land new WK API method: http://webkit.org/b/96122 > (1) Update Chromium to handle old/new WK API: https://chromiumcodereview.appspot.com/10917099 > (2) Land WK change (this): https://bugs.webkit.org/show_bug.cgi?id=90411 > (3) Remove back-compat shims from Chromium > (4) Remove back-compat shims from WebKit/chromium/public
Created attachment 162802 [details] Patch
Created attachment 162901 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Bleah, inspector test failures. Something didn't like being rebased...
Created attachment 162909 [details] Patch
Comment on attachment 162909 [details] Patch Attachment 162909 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13794559 New failing tests: platform/chromium/compositing/layout-width-change.html platform/chromium/compositing/render-surface-alpha-blending.html platform/chromium/compositing/tiny-layer-rotated.html platform/chromium/compositing/huge-layer-rotated.html http/tests/inspector/indexeddb/database-data.html platform/chromium/compositing/3d-corners.html platform/chromium/compositing/video-frame-size-change.html platform/chromium/compositing/perpendicular-layer-sorting.html fast/loader/unload-form-post-about-blank.html platform/chromium/compositing/child-layer-3d-sorting.html platform/chromium/compositing/accelerated-drawing/svg-filters.html platform/chromium/compositing/lost-compositor-context-permanently.html platform/chromium/compositing/filters/background-filter-blur-outsets.html platform/chromium/compositing/lost-compositor-context-with-video.html platform/chromium/compositing/accelerated-drawing/alpha.html http/tests/inspector/indexeddb/database-structure.html fast/repaint/block-selection-gap-in-composited-layer.html http/tests/inspector/indexeddb/resources-panel.html fast/canvas/webgl/shader-precision-format.html platform/chromium/compositing/webgl-loses-compositor-context.html platform/chromium/compositing/backface-visibility-transformed.html platform/chromium/compositing/lost-compositor-context-with-rendersurface.html http/tests/xmlhttprequest/web-apps/001.html http/tests/inspector/indexeddb/database-names.html platform/chromium/compositing/lost-compositor-context.html platform/chromium/compositing/filters/background-filter-blur-off-axis.html http/tests/media/media-source/seek-to-end-after-duration-change.html fast/loader/local-CSS-from-local.html platform/chromium/compositing/img-layer-grow.html platform/chromium/compositing/lost-compositor-context-twice.html
(In reply to comment #20) > Bleah, inspector test failures. Something didn't like being rebased... The inspector tests pass for me locally after applying that latest patch, fwiw.
Created attachment 163406 [details] Patch
> The inspector tests pass for me locally after applying that latest patch, fwiw. Yeah, I'm not sure what that later failure was about - possibly it was results of a previous run mixed in? The other failures were obviously unrelated. Anyway, passed locally for me as well. I just re-uploaded the same patch, let's see what happens.
abarth@ - can you look at the chromum public webkit API part of this? tony@ - can you take a general look? Sorry about the size of this. It's big, but fairly mechanical (thanks to dgrogan's suggestions for minimizing test impact).
Comment on attachment 163406 [details] Patch Looks like the API changes are only FIXME comments. They LGTM.
Oops, would help to cc tony@ if I ask him for a review. D'oh. tony@ r?
Comment on attachment 163406 [details] Patch There's some flakiness with the compositor tests on the ews bots. We're still trying to track down the cause. This looks much simpler. LGTM.
Comment on attachment 163406 [details] Patch Rejecting attachment 163406 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: seImpl.cpp patching file Source/WebKit/chromium/src/WebIDBDatabaseImpl.h patching file Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp patching file Source/WebKit/chromium/src/WebIDBFactoryImpl.h patching file Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.cpp patching file Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Chang']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/13855088
Created attachment 163921 [details] Patch for landing
Comment on attachment 163921 [details] Patch for landing Clearing flags on attachment: 163921 Committed r128489: <http://trac.webkit.org/changeset/128489>
All reviewed patches have been landed. Closing bug.
This doesn't compile on windows unfortunately: 1>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webcore\modules\indexeddb\IDBFactoryBackendInterface.h(57):error C2923: 'WTF::PassRefPtr' : 'WebCore::IDBDatabaseCallbacks' is not a valid template type argument for parameter 'T' 1> C:\b\build\slave\webkit-win-latest-rel\build\src\build\Release\obj\global_intermediate\webcore\bindings/V8IDBDatabase.cpp(407) : see declaration of 'WebCore::IDBDatabaseCallbacks'
(In reply to comment #34) > This doesn't compile on windows unfortunately: > > > 1>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webcore\modules\indexeddb\IDBFactoryBackendInterface.h(57):error C2923: 'WTF::PassRefPtr' : 'WebCore::IDBDatabaseCallbacks' is not a valid template type argument for parameter 'T' > 1> C:\b\build\slave\webkit-win-latest-rel\build\src\build\Release\obj\global_intermediate\webcore\bindings/V8IDBDatabase.cpp(407) : see declaration of 'WebCore::IDBDatabaseCallbacks' Bleah. Thanks jamesr@ I think it just needs #include "IDBDatabaseCallbacks.h" in IDBFactoryBackendInterface.h - submitting a try job to see.
Oh, bleah, it's confusing the IDBDatabaseCallbacks auto-generated inside V8IDBDatabase.cpp with the "real" IDBDatabaseCallbacks type. The former is a static array of V8DOMConfiguration::BatchedCallback entries - it should probably be in the IDBDatabaseV8Internal namespace, which means a code generator change?
Created attachment 163994 [details] Patch for landing
Comment on attachment 163994 [details] Patch for landing Clearing flags on attachment: 163994 Committed r128533: <http://trac.webkit.org/changeset/128533>