Bug 75751 - IndexedDB: Make WebIDBDatabase::close() idempotent
Summary: IndexedDB: Make WebIDBDatabase::close() idempotent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-06 15:19 PST by Joshua Bell
Modified: 2012-01-09 13:08 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.51 KB, patch)
2012-01-06 15:22 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (1.51 KB, patch)
2012-01-06 17:53 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2012-01-09 11:25 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-01-06 15:19:25 PST
IndexedDB: Make WebIDBDatabase::close() idempotent
Comment 1 Joshua Bell 2012-01-06 15:22:12 PST
Created attachment 121508 [details]
Patch
Comment 2 Joshua Bell 2012-01-06 15:23:59 PST
The simplest/cleanest fix for http://code.google.com/p/chromium/issues/detail?id=80111 requires that chromium's implementation of WebIDBDatabase::close() be idempotent. This is one way to achieve that, which keeps the assertion behavior when called directly (i.e. non-Chromium builds)

That said, I'm not positive that this is the best approach.
Comment 3 David Grogan 2012-01-06 17:12:49 PST
Comment on attachment 121508 [details]
Patch

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

LGTM

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:106
>      ASSERT(m_databaseCallbacks);

You can delete this line now.
Comment 4 Joshua Bell 2012-01-06 17:53:03 PST
Created attachment 121527 [details]
Patch
Comment 5 Joshua Bell 2012-01-09 10:30:29 PST
tony@ - r? cq?
Comment 6 Tony Chang 2012-01-09 10:40:12 PST
Comment on attachment 121527 [details]
Patch

Nit: Should we have a layout test that calls close() twice?  The assert should be hit in a debug build without this change.
Comment 7 Joshua Bell 2012-01-09 11:13:45 PST
(In reply to comment #6)
> (From update of attachment 121527 [details])
> Nit: Should we have a layout test that calls close() twice?  The assert should be hit in a debug build without this change.

The IDBDatabase object has an internal m_closePending flag which makes a script's call to db.close() exit early if called twice. If that wasn't present, then in the Chromium port the call would eventually make it into WebIDBDatabase::close() and we'd hit this assert. 

All that said, we didn't appear to have a layout test that called close() twice, so I've added that to database-basics, but it passes both with and without this patch.
Comment 8 Joshua Bell 2012-01-09 11:25:11 PST
Created attachment 121700 [details]
Patch
Comment 9 WebKit Review Bot 2012-01-09 13:08:46 PST
Comment on attachment 121700 [details]
Patch

Clearing flags on attachment: 121700

Committed r104481: <http://trac.webkit.org/changeset/104481>
Comment 10 WebKit Review Bot 2012-01-09 13:08:50 PST
All reviewed patches have been landed.  Closing bug.