In the IDB methods that create IDBRequests follow this pattern: PassRefPtr<IDBRequest> IDBFoo::frobWidget() { if (!m_transaction->isActive()) { ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR; return 0; } RefPtr<IDBRequest> request = IDBRequest::create(context, IDBAny::create(this), m_transaction.get()); m_backend->frobWidget(request, m_transaction->backend(), ec); if (ec) { request->markEarlyDeath(); return 0; } return request.release(); } IDBFooBackendImpl::frobWidget(PassRefPtr<IDBCallbacks> callbacks, IDBTransactionBackendInterface* transaction, ExceptionCode& ec) { if (!IDBTransactionBackendImpl::from(transaction)->scheduleTask( createCallbackTask(&IDBFooBackendImpl::frobWidgetInternal, this, callbacks))) ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR; } Note the two places where inactive transactions are tested for - this is because the backend may abort at the same time (different thread/process) as the script is issuing the request. This has the side effect that this user code could fail: var request1 = foo.frobWidget(); // no exception - but will get abort event // transaction backend aborts asynchronously here var request2 = foo.frobWidget(); // throws exception Ideally, TransactionInactiveError would not be issued until the script was at least plausibly aware. To simplify the implementation and make for a more consistent developer experience, maybe we can: * Remove the markEarlyDeath() logic and state, which simplifies IDBRequest. * If the have either the front end or back end fire an AbortError event at the request if the transaction aborted asynchronously
Josh and I were talking about this and one thing I pointed out is that the frontend can't really get notified until after the current script task is done running - even though the backend calls asynchronously into the frontend, the frontend can't process that call until the current script finishes executing. So theoretically m_transaction->isActive() can't change state during script execution, at least due to anything happening in the backend. So that may mean this is simpler than we thought. checking for it is tricky though.
*** Bug 97732 has been marked as a duplicate of this bug. ***
Created attachment 165884 [details] Patch
dgrogan@, alecflett@ - please take a look.
Comment on attachment 165884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165884&action=review > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:235 > + if (!m_gotValue || isKeyCursor()) { is this just an edge case that got pushed to the frontend? > Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:66 > +void IDBCursorBackendImpl::continueFunction(PassRefPtr<IDBKey> prpKey, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode&) Is it worth stubbing on non-ec versions of these methods now, so that at some point we can remove them after the chromium code is removed? > Source/WebCore/Modules/indexeddb/IDBIndexBackendImpl.cpp:127 > + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::IDB_ABORT_ERR, "")); Seems unfortunately not to have an error message, even if it's a cryptic one (so that we could at least trace it back to the individual failure) like "InternalError: openKeyCursor failed to run." > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:255 > + if (m_readyState == DONE && error->code() == IDBDatabaseException::IDB_ABORT_ERR) oh I see now.. so no error event should fire from the request?
Comment on attachment 165884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165884&action=review >> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:235 >> + if (!m_gotValue || isKeyCursor()) { > > is this just an edge case that got pushed to the frontend? Yes. The back-end had this test, the front end was missing it. It was caught by storage/indexeddb/exceptions.html (w00t!) >> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:66 >> +void IDBCursorBackendImpl::continueFunction(PassRefPtr<IDBKey> prpKey, PassRefPtr<IDBCallbacks> prpCallbacks, ExceptionCode&) > > Is it worth stubbing on non-ec versions of these methods now, so that at some point we can remove them after the chromium code is removed? Since that will touch the chromium webkit API I wanted to do that separately as a pure cleanup patch. (It'll be multi-part too.) > Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:-115 > - if (!m_cursor || m_cursorType == IndexKeyCursor) { Here's where the removed test came from. The new code (front and back end) looks just like update() now. >> Source/WebCore/Modules/indexeddb/IDBIndexBackendImpl.cpp:127 >> + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::IDB_ABORT_ERR, "")); > > Seems unfortunately not to have an error message, even if it's a cryptic one (so that we could at least trace it back to the individual failure) like "InternalError: openKeyCursor failed to run." Good idea - I'll add it. >> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:255 >> + if (m_readyState == DONE && error->code() == IDBDatabaseException::IDB_ABORT_ERR) > > oh I see now.. so no error event should fire from the request? Correct - this is just for cleanup in Chromium IPC. * front-end issues request, back end receives request, then aborts: front end is sent abort event, transaction aborts the requests * front-end issues request, back end aborts, then receives request: front end is sent abort event, front end is sent error event, transaction aborts the requests, error event ignored * back end aborts, first: front-end never issues request (We may already "leak" requests in the Chromium IPC layer if a transaction aborts since the callbacks are never fired. I'll investigate.)
Created attachment 165899 [details] Patch
I'm confused how this doesn't show up in our layout tests. It seems that we have tests that test for the presence of exceptions in these exceptional situations. Now that we're not throwing them anymore shouldn't our tests fail?
(In reply to comment #8) > I'm confused how this doesn't show up in our layout tests. It seems that we have tests that test for the presence of exceptions in these exceptional situations. Now that we're not throwing them anymore shouldn't our tests fail? We were generating TransactionInactiveError in two places: (1) Front end - if the front end has already received the "transaction finished" notification from the back end, the front end can throw the exception directly and skip creating the request. (2) Back end - if the "transaction finished" notification had not yet reached the front end, the back end would return an exception code, and the front end will "mark early death" the request it created and throw the exception. #2 would only occur in multiprocess ports where the async "transaction finished" notification from the back end could be sent while script was executing, and therefore the front end is too busy to have seen it yet. This patch just eliminates special handling for #2; if the front end is unaware that the transaction has finished then simply let the request be created and returned to script, and when the "transaction finished" notification is processed on the front end the request will be aborted. Existing layout tests necessarily only verify #1.
(In reply to comment #0) I just saw this explanation up top. Thanks for bearing with me as I get my head around this. > var request1 = foo.frobWidget(); // no exception - but will get abort event > // transaction backend aborts asynchronously here > var request2 = foo.frobWidget(); // throws exception It does seem that writing even a flaky layout test for that situation would be hard, let alone a solid one. Can a unit test be constructed? > Ideally, TransactionInactiveError would not be issued until the script was at least plausibly aware. I don't understand this sentence. Aware of what?
(In reply to comment #10) > It does seem that writing even a flaky layout test for that situation would be hard, let alone a solid one. Yeah - it won't happen in single-process e.g. DumpRenderTree. > Can a unit test be constructed? I'll look into how difficult. (We need to get to the point where we can have the entire back end mocked to drive the front end through a series of states. And vice versa.) > > Ideally, TransactionInactiveError would not be issued until the script was at least plausibly aware. > > I don't understand this sentence. Aware of what? Erm, yeah, not sure I finished that sentence - I tossed this bug in as it occurred to me as I was rushing out the door. What I meant was: scripts can detect that the transaction is inactive by doing a dummy get, e.g. function blah() { var transactionInactive = false; try { store.get(0); } catch (e) { transactionInactive = true; } ... if (!transactionInactive) { ... should be able to do something here... } } Once the transaction is active, then unless abort() is called by the script itself then IMHO the active state of the transaction shouldn't change until control returns to the event loop. It would only change due to asynchronous processing of tasks, which seems like something we should conceal from script. ... which is basically me justifying the change in behavior I was suggesting.
Created attachment 166507 [details] Patch
tony@ - r? (the ChangeLog may be too verbose at this point)
Comment on attachment 166507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166507&action=review > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:90 > + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::IDB_ABORT_ERR, "The transaction was aborted, so the request cannot be fulfilled.")); Should we share this string? Maybe a static const char[] IDBObjectStoreBackendImpl::abortedTransactionErrorMessage? > Source/WebKit/chromium/tests/IDBRequestTest.cpp:72 > + // Now simulate the back end having fired an abort error at the request to clear up any intermediaries. > + request->onError(IDBDatabaseError::create(IDBDatabaseException::IDB_ABORT_ERR, "Description goes here.")); You're just checking that an ASSERT isn't triggered?
(In reply to comment #14) > (From update of attachment 166507 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166507&action=review > > > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:90 > > + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::IDB_ABORT_ERR, "The transaction was aborted, so the request cannot be fulfilled.")); > > Should we share this string? Maybe a static const char[] IDBObjectStoreBackendImpl::abortedTransactionErrorMessage? Will do. > > Source/WebKit/chromium/tests/IDBRequestTest.cpp:72 > > + // Now simulate the back end having fired an abort error at the request to clear up any intermediaries. > > + request->onError(IDBDatabaseError::create(IDBDatabaseException::IDB_ABORT_ERR, "Description goes here.")); > > You're just checking that an ASSERT isn't triggered? Yes. I'll comment at least, see if there's a pattern for indicating this.
Created attachment 166557 [details] Patch for landing
Comment on attachment 166557 [details] Patch for landing Clearing flags on attachment: 166557 Committed r130095: <http://trac.webkit.org/changeset/130095>
All reviewed patches have been landed. Closing bug.