Leaving these windows in bfcache will cause script to run if another page calls setVersion.

I think an optimal strategy here would be to when about to fire a versionchange event on a IDBDatabase, see if the window for the database is currently in bfcache (easy to do). If it is in bfcache, don't fire the event, instead force-close the database (also easy), and kick the window out of bfcache (hard to do). The reason kicking something out of the bfcache is hard is that there doesn't seem to be a way to get from a window to the SHEntry which is caching it. The SHEntry already has code for kicking a window out of bfcache, but I can't see a way to get to the SHEntry. Smaug, would it be possible and low-risk to add a path from either the content-viewer or the document or window to the SHEntry in which it is being cached? We probably don't want to make such a reference a strong reference at this point due to the leak risk.

If mOwner is set correctly in IDBDataBase, the event handler won't be called when event is fired if the mOwner isn't the current inner window of the outer window. But if we put the outer window to bfcache (do we ever? Docshell owns the outer window), IDBDataBase could just override GetContextForEventHandlers and check there that the related docshell isn't frozen. If it is frozen, the method could return an error value. I don't know how setVersion works. Could we close DB when page is going to bfcache? Or could we prevent bfcache if there is an open DB?

> I don't know how setVersion works. Could we close DB when page is going to > bfcache? The way that setVersion works is as follows: 1. When setVersion is called, we look at if there are any other IDBDatabase objects open to the same database (i.e. same on-disk database). 2. If there are no other open IDBDatabase objects, a transaction is created and an event is fired to indicate that the original caller can start modifying the database. 3. If there *are* other IDBDatabase objects opened, an even is fired on each such IDBDatabase object indicating that it should close. 4. Only once all other IDBDatabase objects are closed will take the action described in step 2. What we ideally want to do is modify step 3 such that if the opened database is in a bfcached document, instead of firing the event, immediately close the database and kick the document out of the bfcache. We would have to kick the document out of bfcache because otherwise we risk the page malfunctioning since the database version has changed. I don't think this solution would be very hard to implement, all we need is a way to given a document, check if it is in bfcache, as well as kick it out of the bfcache. All that's missing in that is a way to get from a document to the SHEntry that's holding it. The SHEntry already has code for kicking a document out of bfcache. > Or could we prevent bfcache if there is an open DB? That would be an alternative solution. But it's not very nice since staying in bfcache is of benefit to users. > But if we put the outer window to bfcache (do we ever? Docshell owns the > outer window), IDBDataBase could just override GetContextForEventHandlers and > check there that the related docshell isn't frozen. If it is frozen, the > method could return an error value. I don't really understand much of this. But I don't think it matters as we don't just want to prevent an event from firing, we want to kick a document out of bfcache.

(In reply to comment #3) > I don't think this solution would be very hard to implement, all we need is a > way to given a document, check if it is in bfcache, as well as kick it out of > the bfcache. All that's missing in that is a way to get from a document to the > SHEntry that's holding it. Document could get a pointer to SHEntry in the same place where DocumentViewer gets it. Just need to be careful to not cause any leaks. > I don't really understand much of this. But I don't think it matters as we > don't just want to prevent an event from firing, we want to kick a document out > of bfcache. Right. I didn't quite understand what this bug is about, but now I do.

Right, the leak situation is what I'm most concerned about. The easiest solution might be to make nsDocuments have a raw (weak) pointer to the SHEntry, which the SHEntry manually nulls out when it gets destroyed or when it kicks the document out of the bfcache.

I'll take this one

This fixes things. The changes are pretty straight forward. Turns out that the shentry already tells the document when it's being bf-cached and when it's removed from bfcache. So all we need to do is also tell it which shentry it's being bfcached in. The changes to nsDocumentViewer.cpp aren't needed at all obviously (no code changes). I mostly wrote them as I was trying to understand the flow of notifications through the docviewer and the shentry. (Holy hell that code is in need of cleanup). Olli: Can you review the non-indexeddb changes (i.e. things not in /dom/indexeddb) Bent: Can you review the indexeddb changes, including the tests?

Comment on attachment 491708 [details] [diff] [review] Patch to fix >diff --git a/dom/indexedDB/IndexedDatabaseManager.cpp b/dom/indexedDB/IndexedDatabaseManager.cpp > ... >+#include "nsISHEntry.h" Nit: put that with the other interfaces at the top, in alphabetical order :) >diff --git a/dom/indexedDB/test/bfcache_iframe1.html b/dom/indexedDB/test/bfcache_iframe1.html > ... >+moz_indexedDB.open("605019").onsuccess = function(e) { >+ var db = e.result; Hm. If this isn't going to be subject to GC i think you need to declare |db| outside the callback. >+ db.setVersion("1.0").onsuccess = function(e) { >+ trans = e.transaction; >+ if (db.objectStoreNames.contains("mystore")) { >+ db.deleteObjectStore("mystore"); >+ } You don't need to worry about this, all databases are cleared at the end of tests so this won't exist. >diff --git a/dom/indexedDB/test/bfcache_iframe2.html b/dom/indexedDB/test/bfcache_iframe2.html > ... >+ alert("calling setVersion(2.0)"); I bet this doesn't work ;) >diff --git a/dom/indexedDB/test/test_bfcache.html b/dom/indexedDB/test/test_bfcache.html > ... >+ alert("loading second"); And this one too!

(In reply to comment #8) > Comment on attachment 491708 [details] [diff] [review] > Patch to fix > > >diff --git a/dom/indexedDB/IndexedDatabaseManager.cpp b/dom/indexedDB/IndexedDatabaseManager.cpp > > ... > >+#include "nsISHEntry.h" > > Nit: put that with the other interfaces at the top, in alphabetical order :) Is that a case insensitive or case sensitive sort :P > >diff --git a/dom/indexedDB/test/bfcache_iframe1.html b/dom/indexedDB/test/bfcache_iframe1.html > > ... > >+moz_indexedDB.open("605019").onsuccess = function(e) { > >+ var db = e.result; > > Hm. If this isn't going to be subject to GC i think you need to declare |db| > outside the callback. Good catch! Fixed. > >+ db.setVersion("1.0").onsuccess = function(e) { > >+ trans = e.transaction; > >+ if (db.objectStoreNames.contains("mystore")) { > >+ db.deleteObjectStore("mystore"); > >+ } > > You don't need to worry about this, all databases are cleared at the end of > tests so this won't exist. I did this to allow me to rerun the test without shutting down the browser every time.

Checked in: http://hg.mozilla.org/mozilla-central/rev/d25bfdc458cf Thanks for the quick reviews!

Backed out on suspicion of causing the Mac OS X 64 leaks we've been seeing all day. http://hg.mozilla.org/mozilla-central/rev/66d88ee26237

This wasn't responsible.

Re-landed: http://hg.mozilla.org/mozilla-central/rev/f17e290f4e4f