Bug 71313 - IndexedDB: Object stores not persisting between sessions
Summary: IndexedDB: Object stores not persisting between sessions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-01 13:15 PDT by Joshua Bell
Modified: 2011-12-14 10:29 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.98 KB, patch)
2011-11-01 13:19 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (2.02 KB, patch)
2011-11-01 14:57 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
persistence layout test (doesn't detect bug) (2.15 KB, text/html)
2011-11-01 15:12 PDT, Joshua Bell
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2011-11-01 13:15:23 PDT
IndexedDB: Object stores not persisting between sessions
Comment 1 Joshua Bell 2011-11-01 13:19:15 PDT
Created attachment 113207 [details]
Patch
Comment 2 jochen 2011-11-01 13:24:47 PDT
LGTM, thanks
Comment 3 Tony Chang 2011-11-01 13:39:27 PDT
Comment on attachment 113207 [details]
Patch

Is it possible to write a layout test for this?
Comment 4 David Grogan 2011-11-01 13:46:38 PDT
Comment on attachment 113207 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113207&action=review

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:112
> +    ASSERT(success == (m_id != InvalidId));

Would the following work?  The only advantage is that it skips the call to loadObjectStores that I just asked you about IRL.

if (success) {
  loadObjectStores();
  return;
}
if (!m_backingStore->createIDB.....)
  ASSERT_NOT_REACHED();
Comment 5 Joshua Bell 2011-11-01 14:57:49 PDT
Created attachment 113227 [details]
Patch
Comment 6 Joshua Bell 2011-11-01 15:12:04 PDT
Created attachment 113235 [details]
persistence layout test (doesn't detect bug)

Logically, the attached layout test should work, but it doesn't detect the bug. The IDBDatabaseBackendImpl is apparently persisting in the IDBFactoryBackendImpl's m_databaseBackendMap across the page load. If there's a way to clear out the script execution context, we can try that. Suggestions welcome.

We should investigate why the impl isn't being torn down as promptly as expected.
Comment 7 jochen 2011-11-01 15:33:10 PDT
(In reply to comment #6)
> Created an attachment (id=113235) [details]
> persistence layout test (doesn't detect bug)
> 
> Logically, the attached layout test should work, but it doesn't detect the bug. The IDBDatabaseBackendImpl is apparently persisting in the IDBFactoryBackendImpl's m_databaseBackendMap across the page load. If there's a way to clear out the script execution context, we can try that. Suggestions welcome.

You could redirect through another security origin, but that would require an http test (localhost -> 127.0.0.01 -> localhost)

> 
> We should investigate why the impl isn't being torn down as promptly as expected.

I also wonder whether it's possible to invoke open() or createObjectStore() after calling close on the IDBDatabase
Comment 8 Joshua Bell 2011-11-03 09:20:51 PDT
(In reply to comment #3)
> (From update of attachment 113207 [details])
> Is it possible to write a layout test for this?

tony@ - I believe the answer is "no" without further work, which we should do, but IMHO without holding up this fix. The issue was caught by a Chromium pyauto test (see http://crbug.com/102537), which is how it was detected (but missed on the webkit side).

r?/cq?
Comment 9 Tony Chang 2011-11-03 10:27:36 PDT
Comment on attachment 113227 [details]
Patch

OK, but some things to investigate.

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=113235) [details] [details]
> > persistence layout test (doesn't detect bug)
> > 
> > Logically, the attached layout test should work, but it doesn't detect the bug. The IDBDatabaseBackendImpl is apparently persisting in the IDBFactoryBackendImpl's m_databaseBackendMap across the page load. If there's a way to clear out the script execution context, we can try that. Suggestions welcome.
> 
> You could redirect through another security origin, but that would require an http test (localhost -> 127.0.0.01 -> localhost)

Does it work to navigate to a different file:/// URL and back?

> > We should investigate why the impl isn't being torn down as promptly as expected.
> 
> I also wonder whether it's possible to invoke open() or createObjectStore() after calling close on the IDBDatabase

You should also feel free to add methods to layoutTestController to enable more testing.
Comment 10 WebKit Review Bot 2011-11-03 10:46:18 PDT
Comment on attachment 113227 [details]
Patch

Clearing flags on attachment: 113227

Committed r99218: <http://trac.webkit.org/changeset/99218>
Comment 11 WebKit Review Bot 2011-11-03 10:46:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Joshua Bell 2011-12-14 10:29:45 PST
(In reply to comment #6)
> Created an attachment (id=113235) [details]
> persistence layout test (doesn't detect bug)
> 
> Logically, the attached layout test should work, but it doesn't detect the bug. The IDBDatabaseBackendImpl is apparently persisting in the IDBFactoryBackendImpl's m_databaseBackendMap across the page load. If there's a way to clear out the script execution context, we can try that. Suggestions welcome.
> 
> We should investigate why the impl isn't being torn down as promptly as expected.

FYI, this specific issue should be fixed with: https://bugs.webkit.org/show_bug.cgi?id=72066