Closed
Bug 618590
Opened 14 years ago
Closed 14 years ago
IndexedDB: Don't put windows with active transactions into the bfcache
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
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)
|
10.50 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
|
6.54 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
|
11.51 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
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.
|
Assignee | |
Updated•14 years ago
|
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+
|
||
Updated•14 years ago
|
Whiteboard: [indexeddb]
|
||
Updated•14 years ago
|
Whiteboard: [indexeddb] → [indexeddb] softblocker
|
|
||
Updated•14 years ago
|
Whiteboard: [indexeddb] softblocker → [indexeddb][softblocker]
|
|
Assignee | |
Comment 2•14 years ago
|
||
Change transaction.abort() so that it's possible to abort even when not in a callback.
Attachment #502925 -
Flags: review?(bent.mozilla)
|
|
Assignee | |
Comment 3•14 years ago
|
||
Don't put a window in the bfcache if there are running transactions in it.
|
|
Assignee | |
Updated•14 years ago
|
Attachment #502926 -
Attachment is patch: true
Attachment #502926 -
Attachment mime type: application/octet-stream → text/plain
Attachment #502926 -
Flags: review?(bent.mozilla)
|
|
Assignee | |
Comment 4•14 years ago
|
||
When a page is destroyed, if there are running transactions in it, attempt to abort those transactions.
Attachment #502932 -
Flags: review?(bent.mozilla)
|
Reporter | |
Comment 5•14 years ago
|
||
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+
|
|
Reporter | |
Comment 6•14 years ago
|
||
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+
|
|
Reporter | |
Comment 7•14 years ago
|
||
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+
|
|
Assignee | |
Comment 8•14 years ago
|
||
> >+ 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.
|
|
Reporter | |
Comment 9•14 years ago
|
||
(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!
|
|
Assignee | |
Comment 10•14 years ago
|
||
|
|
Assignee | |
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 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.
Description
•