Bug 103536 – IndexedDB: Simplify IDBTransactionBackendImpl::scheduleTask usage

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

Currently it returns true/false but we eventually drop the return type on the floor.

[Alec Flett]

which function?

[Joshua Bell]

Whoops - scheduleTask. Fixed title.

[Joshua Bell]

Created attachment 176787 [details]
Patch

[Joshua Bell]

So hrm. Two reasons this can't be all glorious cleanup:

* The schema change methods (create/delete ObjectStore/Index) are still synchronous and return exception codes.

* The other (async) calls rely on callback->onError(ABORT_ERR) to clean up internal callback tracking inside Chromium, even though the AbortError actually gets generated on the front end.

[Joshua Bell]

Created attachment 177036 [details]
Patch

[Joshua Bell]

So this patch doesn't actually simplify anything; I'm going to punt on it for now.

An alternate approach might be to optionally pass the IDBCallback into scheduleTask, and have scheduleTask() responsible for calling callbacks->onError(), which would take care of most of the cases.

[Joshua Bell]

Or have the WebIDBCallbacksProxy destructor call onError if it never fired onSuccess/onError - that's probably the best approach.

[Joshua Bell]

Created attachment 177368 [details]
Patch

[Joshua Bell]

I took another stab at this, making chromium clean up itself. Still not sure this is worth it, but I'm liking it more.

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

[David Grogan]

Comment on attachment 177368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177368&action=review

> Source/WebCore/ChangeLog:9
> +        back-end after an asynchronous transaction abort out of WebCore, since the

Asynchronous transaction abort == one that was initiated by code in the browser process, not by script?

> Source/WebKit/chromium/src/WebIDBCallbacksImpl.cpp:62
> +        m_callbacks->onError(WebIDBDatabaseError(WebIDBDatabaseExceptionAbortError, WebString()));

This just calls IDBRequest::onError without going through chrome code, doesn't it? Was this supposed to go in IDBCallbacksProxy?

[Joshua Bell]

(In reply to comment #11)
> (From update of attachment 177368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177368&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        back-end after an asynchronous transaction abort out of WebCore, since the
> 
> Asynchronous transaction abort == one that was initiated by code in the browser process, not by script?

Correct. The scenario is that there are requests in-flight from the front end via async IPC when the back-end decides to abort.

> > Source/WebKit/chromium/src/WebIDBCallbacksImpl.cpp:62
> > +        m_callbacks->onError(WebIDBDatabaseError(WebIDBDatabaseExceptionAbortError, WebString()));
> 
> This just calls IDBRequest::onError without going through chrome code, doesn't it? Was this supposed to go in IDBCallbacksProxy?
q
*sigh* Yes, thanks.

[Joshua Bell]

Created attachment 177497 [details]
Patch

[David Grogan]

Comment on attachment 177497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177497&action=review

LGTM

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:260
>          ec = IDBDatabaseException::TransactionInactiveError;

Could you add a FIXME to add a test for this clause? It seems like it would be hard to test without flakiness.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:514
>          ec = IDBDatabaseException::TransactionInactiveError;

Could you add a FIXME to add a test for this clause?

> Source/WebKit/chromium/src/IDBCallbacksProxy.cpp:69
> +    // This cleans up the request's IPC id.

To test this, I suppose you'd need a browser test that loaded up a page that tries to trigger this scenario (backend aborts while there was an in-flight request from the frontend) many times. Then the browser test code would retrieve the IndexedDBDispatcherHost and check its maps for residual callbacks entries. Seems like you wouldn't be able to guarantee that the page triggered this scenario.

Or we could let the valgrind bots run our layout tests in content_shell and see if we get any reports. I don't know if stray map entries is something valgrind would catch though.

Any other options you've thought of? If you haven't come up with something better I'd say just add a FIXME noting that this has no test.

[Joshua Bell]

Created attachment 178054 [details]
Patch

[Joshua Bell]

(In reply to comment #15)
> Created an attachment (id=178054) [details]
> Patch

(Just a rebase, I haven't addressed the above comments yet)

[Joshua Bell]

Created attachment 181082 [details]
Patch

[Joshua Bell]

Revised the patch to remove the need to explicitly test transaction->isFinished() before calling scheduleTask(), and have the latter still return a success flag. This streamlines the code and is less likely to lead to flaky bugs in the future where a dev adds a scheduleTask() call without checking.

dgrogan@, alecflett@ - one more look?

[David Grogan]

lgtm except that the bug title is now inaccurate.

[Joshua Bell]

(In reply to comment #19)
> lgtm except that the bug title is now inaccurate.

Hah, will fix. :)

[Joshua Bell]

Created attachment 181116 [details]
Patch

[Joshua Bell]

abarth@ - r? for the trivial Chromium public API change?
tony@ - r? overall?

[Adam Barth]

Comment on attachment 181116 [details]
Patch

API change LGTM

[Alec Flett]

Comment on attachment 181116 [details]
Patch

any reason scheduleTask can't now return void as you originally tried?

[Alec Flett]

oh wait comment 4..

[Joshua Bell]

(In reply to comment #25)
> oh wait comment 4..

Yep. :) Once {create,delete}{ObjectStore,Index} are async we can probably simplify further.

[WebKit Review Bot]

Comment on attachment 181116 [details]
Patch

Clearing flags on attachment: 181116

Committed r138716: <http://trac.webkit.org/changeset/138716>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.