IndexedDB: deleteDatabase is broken

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

({regression})

unspecified
x86_64
Windows 7
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10 unaffected, firefox11 fixed, firefox12 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

Created attachment 585512 [details] [diff] [review]
Patch

deleteDatabase is broken because we don't remove the subdir and then we can never open the db again.
Attachment #585512 - Flags: review?(Jan.Varga)
Comment on attachment 585512 [details] [diff] [review]
Patch

Review of attachment 585512 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you address these:

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +2371,5 @@
>        NS_WARNING("Failed to delete db file!");
>        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>      }
> +
> +    dbFile->Remove(true);

error check

@@ +2401,5 @@
>        NS_WARNING("Failed to delete file directory!");
>        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
>      }
> +
> +    fileManagerDirectory->Remove(true);

here too.
Attachment #585512 - Flags: review?(Jan.Varga) → review+
Could you also add a test for this. Specifically the following two things should be tested:

* Delete a database which doesn't exist and then open it and make sure that opening succeeds.
* Create a new database with an objectstore. Close it. Delete it. Open database and make sure that opening succeeds.
Duplicate of this bug: 714816
(In reply to Jonas Sicking (:sicking) from comment #2)
> Could you also add a test for this. Specifically the following two things
> should be tested:
> 
> * Delete a database which doesn't exist and then open it and make sure that
> opening succeeds.

Done.

> * Create a new database with an objectstore. Close it. Delete it. Open
> database and make sure that opening succeeds.

This is tested by the test in this patch.
https://hg.mozilla.org/mozilla-central/rev/e6fb800eb24a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Comment on attachment 585512 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): Bug 661877
User impact if declined: deleteDatabase will be broken, web apps may break
Testing completed (on m-c, etc.): Automated testing
Risk to taking this patch (and alternatives if risky): Limited.  Backing out Bug 661877 is far more risky.
Attachment #585512 - Flags: approval-mozilla-aurora?
status-firefox10: --- → unaffected
status-firefox11: --- → affected
status-firefox12: --- → fixed

Comment 7

6 years ago
Comment on attachment 585512 [details] [diff] [review]
Patch

[Triage Comment]
Limited risk, 11 regression, it's had a day on m-c, and unbreaks the web. Sounds righteous for Aurora.
Attachment #585512 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/673da4a818a0
status-firefox11: affected → fixed
Is there a way QA can manually verify this? I assume some form of comment 2?
Whiteboard: [qa?]
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla12 → ---
Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.