Bug 90411 – IndexedDB: Consolidate two-phase connection to avoid race conditions

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Consolidate two-phase connection to avoid race conditions

[Joshua Bell]

Created attachment 150493 [details]
Patch

[Joshua Bell]

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.

[Joshua Bell]

Hold off on this until the dust settles from http://webkit.org/b/89505

[Joshua Bell]

Created attachment 161334 [details]
Patch

[Joshua Bell]

Created attachment 162360 [details]
Patch

[Joshua Bell]

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 :(

[David Grogan]

(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 :(

[David Grogan]

Created attachment 162405 [details]
dgrogans

[David Grogan]

Comment on attachment 162405 [details]
dgrogans

interdiff isn't working, just ignore this patch.

[David Grogan]

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.

[Joshua Bell]

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.

[Joshua Bell]

(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)
}

[Joshua Bell]

Created attachment 162541 [details]
Patch

[Joshua Bell]

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

[David Grogan]

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.

[Joshua Bell]

(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

[Joshua Bell]

Created attachment 162802 [details]
Patch

[Joshua Bell]

Created attachment 162901 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Joshua Bell]

Bleah, inspector test failures. Something didn't like being rebased...

[Joshua Bell]

Created attachment 162909 [details]
Patch

[WebKit Review Bot]

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

[David Grogan]

(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.

[Joshua Bell]

Created attachment 163406 [details]
Patch

[Joshua Bell]

> 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.

[Joshua Bell]

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).

[Adam Barth]

Comment on attachment 163406 [details]
Patch

Looks like the API changes are only FIXME comments.  They LGTM.

[Joshua Bell]

Oops, would help to cc tony@ if I ask him for a review. D'oh.

tony@ r?

[Tony Chang]

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.

[WebKit Review Bot]

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

[Joshua Bell]

Created attachment 163921 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 163921 [details]
Patch for landing

Clearing flags on attachment: 163921

Committed r128489: <http://trac.webkit.org/changeset/128489>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

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'

[Joshua Bell]

(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.

[Joshua Bell]

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?

[Joshua Bell]

Created attachment 163994 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 163994 [details]
Patch for landing

Clearing flags on attachment: 163994

Committed r128533: <http://trac.webkit.org/changeset/128533>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.