Closed Bug 618590 Opened 12 years ago Closed 12 years ago

IndexedDB: Don't put windows with active transactions into the bfcache

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: bent.mozilla, Assigned: sicking)

References

(Blocks 1 open bug)

Details

(Whiteboard: [indexeddb][softblocker])

Attachments

(3 files)

Jonas agreed to take this one. Basically we have to abort any active transactions and refuse to add the window to the bfcache if transactions are in progress when the user leaves the page.
blocking2.0: ? → beta9+
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
blocking2.0: beta9+ → betaN+
Whiteboard: [indexeddb]
Whiteboard: [indexeddb] → [indexeddb] softblocker
Whiteboard: [indexeddb] softblocker → [indexeddb][softblocker]
Change transaction.abort() so that it's possible to abort even when not in a callback.
Attachment #502925 - Flags: review?(bent.mozilla)
Don't put a window in the bfcache if there are running transactions in it.
Attachment #502926 - Attachment is patch: true
Attachment #502926 - Attachment mime type: application/octet-stream → text/plain
Attachment #502926 - Flags: review?(bent.mozilla)
When a page is destroyed, if there are running transactions in it, attempt to abort those transactions.
Attachment #502932 - Flags: review?(bent.mozilla)
Comment on attachment 502925 [details] [diff] [review]
Part 1: Fix abort

>-  if (!IsOpen()) {
>+  if (mReadyState != IDBTransaction::INITIAL &&
>+      mReadyState != IDBTransaction::LOADING) {

r=me, just add a comment saying why we can't use IsOpen() here.
Attachment #502925 - Flags: review?(bent.mozilla) → review+
Comment on attachment 502926 [details] [diff] [review]
Part 2: Don't put in bfcache

>+  // Check if we have runnint IndexedDB transactions

No Swedish here! ;)

>+  for (PRUint32 index = 0; index < liveDatabases.Length(); index++) {
>+    IDBDatabase*& database = liveDatabases[index];
>+    if (database->Owner() == aWindow &&
>+        pool->HasTransactionsForDatabase(database)) {
>+      return true;

How about:

  if (database->Owner() == aWindow) {
    return pool->HasTransactionsForDatabase(database);
  }

>+  // Are there running or pending transactions for a given database?

Make this a statement, "Returns true if there are running..."
Attachment #502926 - Flags: review?(bent.mozilla) → review+
Comment on attachment 502932 [details] [diff] [review]
Part 3: Abort when leaving page

>+IndexedDatabaseManager::AbortCloseDatabasesForWindow(nsPIDOMWindow* aWindow)

AbortTransactionsAndCloseDatabasesForWindow?

>+  // Get list of transactions for this database id

Nit: Closing periods on comments?

>+  // Collect any pending transactions. Do this before the running transactions
>+  // so we don't waste time scheduling them while aborting the running
>+  // transactions.

I don't understand this comment...

>+  // Abort transactions. Do this after collecting the transactions in case
>+  // calling Abort() modifies the data structures we're iterating above.
>+  for (PRUint32 index = 0; index < transactions.Length(); index++) {
>+    // See if any transaction belongs to this IDBDatabase instance
>+    IDBTransaction* transaction = mDelayedDispatchQueue[index].transaction;

Copy/paste bug.

>+    db.setVersion("1.0").onsuccess = function(e) {
>+      var trans = e.transaction;

That should be e.target.transaction


>+++ b/dom/indexedDB/test/test_leaving_page.html
> ...
>+<body onload="runTest();"></body>

Eek.
Attachment #502932 - Flags: review?(bent.mozilla) → review+
> >+  for (PRUint32 index = 0; index < liveDatabases.Length(); index++) {
> >+    IDBDatabase*& database = liveDatabases[index];
> >+    if (database->Owner() == aWindow &&
> >+        pool->HasTransactionsForDatabase(database)) {
> >+      return true;
> 
> How about:
> 
>   if (database->Owner() == aWindow) {
>     return pool->HasTransactionsForDatabase(database);
>   }

That wouldn't work since if the first database we find doesn't have transactions, we need to keep looping to see if there are other databases in the same window.
(In reply to comment #8)
> That wouldn't work since if the first database we find doesn't have
> transactions, we need to keep looping to see if there are other databases in
> the same window.

Right. Duh. Never mind!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Component: DOM → DOM: IndexedDB
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.