IDB: storage/indexeddb/mozilla/object-store-remove-values.html fails

• Component: WebKit2
• Status: ASSIGNED ---

[Brady Eidson]

IDB: storage/indexeddb/mozilla/object-store-remove-values.html fails

The test does a deleteDatabase and then immediately reopens a connection to that same database name.  We don't handle the deleteDatabase cleanup properly in this case.

[Radar WebKit Bug Importer]

<rdar://problem/16029319>

[Brady Eidson]

Created attachment 223740 [details]
Patch v1

[WebKit Commit Bot]

Attachment 223740 [details] did not pass style-queue:


ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:165:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 223740 [details]
Patch v1

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

> Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp:62
> +    if (m_uniqueIDBDatabase)

early return instead?

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:166
> +    while (performNextMainThreadTaskWithoutAdoptRef())
> +    {
> +    }

Indentation/brace location looks weird to me.

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:79
> +        JSLockHolder lockHolder(JSLockHolder(m_vm.get()));

wut

[Mark Hahnenberg]

Comment on attachment 223740 [details]
Patch v1

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

>> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:79
>> +        JSLockHolder lockHolder(JSLockHolder(m_vm.get()));
> 
> wut

I think you only want one level of JSLockHolder here. It probably works because C++ is auto generating a copy constructor that does something at least somewhat reasonable.

JSLockHolder holder(m_vm.get());

should work.

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 223740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223740&action=review
> 
> >> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:79
> >> +        JSLockHolder lockHolder(JSLockHolder(m_vm.get()));
> > 
> > wut
> 
> I think you only want one level of JSLockHolder here. It probably works because C++ is auto generating a copy constructor that does something at least somewhat reasonable.
> 
> JSLockHolder holder(m_vm.get());
> 
> should work.

You are, of course, very correct.  *sigh*

Just landed in http://trac.webkit.org/changeset/163817 but will followup.

[Brady Eidson]

http://trac.webkit.org/changeset/163817
and followup http://trac.webkit.org/changeset/163818

[Alexey Proskuryakov]

Has this been finished? See bug 145623 though.