Bug 154922 - Modern IDB: Close UniqueIDBDatabases once they become unused
Summary: Modern IDB: Close UniqueIDBDatabases once they become unused
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117
  Show dependency treegraph
 
Reported: 2016-03-02 10:54 PST by Brady Eidson
Modified: 2016-03-02 22:45 PST (History)
4 users (show)

See Also:


Attachments
Patch v1 (25.41 KB, patch)
2016-03-02 12:21 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v1 with more style (25.44 KB, patch)
2016-03-02 12:26 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch for landing (28.58 KB, patch)
2016-03-02 14:53 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-03-02 10:54:49 PST
Modern IDB: Close UniqueIDBDatabases once they become unused

Besides just being a good idea, this was actually causing problems in testing on OS X as once a DatabaseProcess (or DumpRenderTree) accumulated ~256 open UniqueIDBDatabases it run up against the launchd enforced "256 open file handles" limit.
Comment 1 Brady Eidson 2016-03-02 12:21:01 PST
Created attachment 272674 [details]
Patch v1
Comment 2 WebKit Commit Bot 2016-03-02 12:22:55 PST
Attachment 272674 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:601:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brady Eidson 2016-03-02 12:26:04 PST
Created attachment 272675 [details]
Patch v1 with more style
Comment 4 Alex Christensen 2016-03-02 14:14:53 PST
Comment on attachment 272675 [details]
Patch v1 with more style

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

Please add a test that opens too many databases and hits the expected error.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1137
> +    // Assuming it is not ephemeral, the server should now close it to free up resources.

Why do we only close non-ephemeral databases?  Shouldn't a memory backing store just not do anything when it closes?  I think this is unnecessary complexity.
Comment 5 Brady Eidson 2016-03-02 14:24:09 PST
(In reply to comment #4)
> Comment on attachment 272675 [details]
> Patch v1 with more style
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272675&action=review
> 
> Please add a test that opens too many databases and hits the expected error.
> 
> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1137
> > +    // Assuming it is not ephemeral, the server should now close it to free up resources.
> 
> Why do we only close non-ephemeral databases?  Shouldn't a memory backing
> store just not do anything when it closes?  I think this is unnecessary
> complexity.

As discussed in person, opening, then closing, then reopening a database in private browsing needs to work - you need to get back to the same in-memory data.

The only reason we can close *persistent* databases is because we can get them back from disk.
Comment 6 Brady Eidson 2016-03-02 14:53:21 PST
Created attachment 272689 [details]
Patch for landing
Comment 7 Brady Eidson 2016-03-02 14:53:44 PST
(Alex said r+ in person, didn't twiddle the review bit here in the bug)
Comment 8 Brady Eidson 2016-03-02 15:27:53 PST
https://bugs.webkit.org/show_bug.cgi?id=154706 was caused by this bug.
Comment 9 WebKit Commit Bot 2016-03-02 16:23:40 PST
Comment on attachment 272689 [details]
Patch for landing

Clearing flags on attachment: 272689

Committed r197474: <http://trac.webkit.org/changeset/197474>