605019 - IndexedDB: Figure out what when to evict windows with live databases from the bfcache

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll take this one

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

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 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in: http://hg.mozilla.org/mozilla-central/rev/d25bfdc458cf

Thanks for the quick reviews!

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This wasn't responsible.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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