New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 614530 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Unused transactions commit out of sequence

Project Member Reported by jsb...@chromium.org, May 24 2016

Issue description

In the IDB back-end, a Commit() request that comes through for a transaction that has never had any requests is allowed to commit without having ever been started by the transaction coordinator.

While the operations would never run out of order, the commit happens unexpectedly early - before a transaction with overlapping scope has completed.

Raised by Moz at: https://github.com/dmurph/indexed-db-observers/issues/24

Repro attached. Expected:

1 rq1.onsuccess
2 rq2.onsuccess
3 tx1.oncomplete
4 tx2.oncomplete

Actual:

1 rq1.onsuccess
4 tx2.oncomplete
2 rq2.onsuccess
3 tx1.oncomplete

Or sometimes:

1 rq1.onsuccess
2 rq2.onsuccess
4 tx2.oncomplete
3 tx1.oncomplete

 
txorder.html
831 bytes View Download

Comment 1 by jsb...@chromium.org, May 24 2016

Code inspection notes:

* IndexedDBTransaction::Commit() has a special case for !used_ (no requests made), and just commits immediately even if not STARTED. 
* IndexedDBTransaction::Start() bails early if !used_. 
* Watch out for a potential rentrancy problem: IndexedDBTransactionCoordinator::ProcessQueuedTransactions calls IndexedDBTransaction::Start() when iterating over transactions; if that calls Commit() we could end up calling IndexedDBTransactionCoordinator::DidFinishTransaction() and re-enter ProcessQueuedTransactions

One thought: 
* In Commit(), if (!used_ && state_ != STARTED), set commit_pending_ = true then bail
* In Start(), if (!used_ && commit_pending_), PostTask(Commit, this)

This could potentially be optimized by having Start() return a bool and have the coordinator not add the tx to the running transactions, but that's nasty code for a rare case.


Comment 2 by palakj@chromium.org, May 26 2016

Cc: jsb...@chromium.org
Owner: palakj@chromium.org
Status: Assigned (was: Available)
Owner: ----
Status: Available (was: Assigned)

Comment 5 by jsb...@chromium.org, Jun 22 2016

Owner: jsb...@chromium.org
Status: Started (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d546a9d9162ce273c8b789306ae934b5db9ed6d1

commit d546a9d9162ce273c8b789306ae934b5db9ed6d1
Author: jsbell <jsbell@chromium.org>
Date: Wed Jun 22 20:54:15 2016

IndexedDB: Ensure transactions without requests commit in the same order

In the IDB back-end, a Commit() request that came through for a
transaction that never had any requests was allowed to run the commit
steps without having seen a Start() call from the transaction
coordinator. While the operations would never run out of order, the
commit would happens unexpectedly early from the point of view of
script - i.e. before an earlier transaction with overlapping scope has
completed.

Handle this special case, using the existing commit_pending_ flag
and a new test in Start().

R=palakj@chromium.org,cmumford@chromium.org
BUG= 614530 

Review-Url: https://codereview.chromium.org/2083203002
Cr-Commit-Position: refs/heads/master@{#401395}

[modify] https://crrev.com/d546a9d9162ce273c8b789306ae934b5db9ed6d1/content/browser/indexed_db/indexed_db_transaction.cc
[add] https://crrev.com/d546a9d9162ce273c8b789306ae934b5db9ed6d1/third_party/WebKit/LayoutTests/storage/indexeddb/empty-transaction-order.html

Comment 7 by jsb...@chromium.org, Jun 22 2016

Status: Fixed (was: Started)

Sign in to add a comment