Bug 154922

Summary: Modern IDB: Close UniqueIDBDatabases once they become unused
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, commit-queue, jsbell
Priority: P2    
Version: Safari 9   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149117    
Attachments:
Description Flags
Patch v1
none
Patch v1 with more style
none
Patch for landing none

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>