Sebastian Zartner [:sebo] Reporter
	Description • 8 years ago

If an error occurs on deletion of an IndexedDB database, the error is not exposed to the user.

STR:
1. Open the Storage Inspector on https://mdn.mozillademos.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB$samples/Full_IndexedDB_example?revision=997741.
2. Right-click the 'mdn-demo-indexeddb-epublications' database and choose 'Delete "mdn-demo-indexeddb-epublications"' from the context menu.

=> Nothing happens. Looking at the Browser Console, the deletion of the database obviously got blocked.

I see two possible non-exclusive solutions for this:
1. Try to resolve the blocked state and delete the database afterwards.
2. Show an error message to the user that the database could not be deleted.

Sebastian

	Jarda Snajdr [:jsnajdr] Assignee
	Comment 1 • 8 years ago

(In reply to Sebastian Zartner [:sebo] from comment #0)
> I see two possible non-exclusive solutions for this:
> 1. Try to resolve the blocked state and delete the database afterwards.
> 2. Show an error message to the user that the database could not be deleted.

Showing an error message is an easy and obvious solution. However, the blocked state can last for a long time, until the page is closed and the db connection is closed, too. It could be an unwelcome surprise if the database got silently deleted on page unload, a long time after my failed attempt to delete it.

	Sebastian Zartner [:sebo] Reporter
	Comment 2 • 8 years ago

(In reply to Jarda Snajdr [:jsnajdr] from comment #1)
> (In reply to Sebastian Zartner [:sebo] from comment #0)
> > I see two possible non-exclusive solutions for this:
> > 1. Try to resolve the blocked state and delete the database afterwards.
> > 2. Show an error message to the user that the database could not be deleted.
> 
> Showing an error message is an easy and obvious solution. However, the
> blocked state can last for a long time, until the page is closed and the db
> connection is closed, too. It could be an unwelcome surprise if the database
> got silently deleted on page unload, a long time after my failed attempt to
> delete it.

I'm not too much into the IndexedDB spec., but isn't there any way to force unblock a database?
If not, I agree that it would be bad to remove it at a later point.

Sebastian

	Jarda Snajdr [:jsnajdr] Assignee
	Comment 3 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=103a67ba2229

	Jarda Snajdr [:jsnajdr] Assignee
	Comment 4 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f8932ba17c5

	Jarda Snajdr [:jsnajdr] Assignee
	Comment 8 • 8 years ago

When the IndexedDB database is opened during the attempt to delete it, the delete action is scheduled to be performed after all connections are closed (during, e.g., page unload). An "onblocked" event is fired immediately to notify the caller about the delay. The database will be deleted later, and the "onsuccess" event fired. No further action by the caller (like calling the delete again) is needed.

Part 1: the main part
There are now two separate events:
1. "removeDB" in parent is called, it requests the database delete, and returns a promise that resolves almost immediately.
2. The database is actually deleted (possibly many hours later), and an "onDatabaseRemoved" notification is sent back to child.

The "removeDB" call can have 4 possible results:
1. Database is deleted immediately, "onsuccess" handler fires, removeDB returns success
2. Error happens, "onerror" handler fires, removeDB returns failure
3. "onblocked" handler fires, removeDB returns "blocked" state
4. Nothing of the above happens within some timeout (3 seconds). This can happen on repeated attempts to delete when the db is blocked - the "onblocked" event fires only once, and nothing happens on further attempts. In this case, we return "blocked" state.

Because IndexedDB has no API for observing changes, we need to implement its poor man's replacement: the "onDatabaseRemoved" notification. It takes care of removing the database from actor.hostVsStores, and firing the "stores-update" event. After we add more editing capabilites to IndexedDB actor, it will evolve into something like "onCookieChanged" for cookies.

Depending on the actor.removeDatabase result, a warning/error is displayed in the Devtools notification box.

Part 2: use sendAsyncMessage when parent/child parts of the IndexedDB actor talk to each other
I think it's a bug to use sendSyncMessage here - we're not waiting for any return value. There's no need to block the child process.

Part 3: ESLint cleanup

The patch can be tested on the mozillademos.org page mentioned by Sebastian above.

	Sebastian Zartner [:sebo] Reporter
	Comment 9 • 8 years ago

(In reply to Jarda Snajdr [:jsnajdr] from comment #8)
> The patch can be tested on the mozillademos.org page mentioned by Sebastian
> above.

Another test case can be found here:
http://nparashuram.com/trialtool/#example=/IndexedDB/trialtool/index.html

Testing the try build So far this looks really good to me. The database gets properly deleted when there are no open connections and an error message is shown in case there are still connections. What I couldn't test yet is the case when the database is closed at a later point. On the mozillademos.org page I didn't find a way to close the connection.
Jarda, do you have a simple test case for me or do I need to write my own one to test it?

Sebastian

	Jarda Snajdr [:jsnajdr] Assignee
	Comment 10 • 8 years ago

(In reply to Sebastian Zartner [:sebo] from comment #9)
> Jarda, do you have a simple test case for me or do I need to write my own
> one to test it?

I'm attaching my idb.html page that I test on. On load, it opens a database and keeps it open. Then there is a button to "Close" it when you want to.

That's useful to test the blocked delete. I've been unable to make IndexedDB.delete return an actual error. The only way how to test the error state is to pass incorrect host name internally to the actor (see the mochitest).

	Sebastian Zartner [:sebo] Reporter
	Comment 11 • 8 years ago

Thank you for the fast reply and the test case!

What I see in that example is that the info message is not removed once the database is deleted. The database itself is removed as expected, though.

(In reply to Jarda Snajdr [:jsnajdr] from comment #8)
> Because IndexedDB has no API for observing changes, we need to implement its
> poor man's replacement: the "onDatabaseRemoved" notification. It takes care
> of removing the database from actor.hostVsStores, and firing the
> "stores-update" event.

This sounds like there should be chrome APIs created that allow observing changes to update the display accordingly.
Please let me know if I should file a bug for that.

Sebastian

	Jarda Snajdr [:jsnajdr] Assignee
	Comment 12 • 8 years ago

(In reply to Sebastian Zartner [:sebo] from comment #11)
> What I see in that example is that the info message is not removed once the
> database is deleted. The database itself is removed as expected, though.

Yes, the notification has a close button ("x") and is displayed until explicitly closed by the user. This is a standard behavior of the Firefox notification component.

Implementing the autoclose is possible, but it's hard to do in a way that wouldn't look like a hack. I'd rather not do it unless really needed. I think that in most real-world cases, the database will be opened for the whole lifetime of the page and closed only on page unload.

> 
> (In reply to Jarda Snajdr [:jsnajdr] from comment #8)
> > Because IndexedDB has no API for observing changes, we need to implement its
> > poor man's replacement: the "onDatabaseRemoved" notification. It takes care
> > of removing the database from actor.hostVsStores, and firing the
> > "stores-update" event.
> 
> This sounds like there should be chrome APIs created that allow observing
> changes to update the display accordingly.
> Please let me know if I should file a bug for that.

Yes, please file a bug. The IndexedDB standard doesn't specify any observer API at all, but for the devtools purposes, it would be really nice. Maybe there are reasons why it doesn't exist, like being impossible to implement with SQLite, so let's at least hear what the experts think about it.

	Sebastian Zartner [:sebo] Reporter
	Comment 13 • 8 years ago

(In reply to Jarda Snajdr [:jsnajdr] from comment #12)
> (In reply to Sebastian Zartner [:sebo] from comment #11)
> > What I see in that example is that the info message is not removed once the
> > database is deleted. The database itself is removed as expected, though.
> 
> Yes, the notification has a close button ("x") and is displayed until
> explicitly closed by the user. This is a standard behavior of the Firefox
> notification component.
> 
> Implementing the autoclose is possible, but it's hard to do in a way that
> wouldn't look like a hack. I'd rather not do it unless really needed. I
> think that in most real-world cases, the database will be opened for the
> whole lifetime of the page and closed only on page unload.

I think, that's a UX flaw, which should be fixed. Therefore I created bug 1273386.

> > (In reply to Jarda Snajdr [:jsnajdr] from comment #8)
> > > Because IndexedDB has no API for observing changes, we need to implement its
> > > poor man's replacement: the "onDatabaseRemoved" notification. It takes care
> > > of removing the database from actor.hostVsStores, and firing the
> > > "stores-update" event.
> > 
> > This sounds like there should be chrome APIs created that allow observing
> > changes to update the display accordingly.
> > Please let me know if I should file a bug for that.
> 
> Yes, please file a bug. The IndexedDB standard doesn't specify any observer
> API at all, but for the devtools purposes, it would be really nice. Maybe
> there are reasons why it doesn't exist, like being impossible to implement
> with SQLite, so let's at least hear what the experts think about it.

Created bug 1273391 for that.

Is your patch ready to land or still missing something?

Sebastian

	Jarda Snajdr [:jsnajdr] Assignee
	Comment 14 • 8 years ago

(In reply to Sebastian Zartner [:sebo] from comment #13)
> I think, that's a UX flaw, which should be fixed. Therefore I created bug
> 1273386.

OK, I'll be thinking about how to implement it in some elegant way.

> Is your patch ready to land or still missing something?

I'm waiting for a review from Mike. Otherwise, I think it's ready. There's a green try run, you tested it...

	Pulsebot
	Comment 15 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/867ef7c9b653
https://hg.mozilla.org/integration/fx-team/rev/9c4a6ed16fbd
https://hg.mozilla.org/integration/fx-team/rev/f1dacf9e859f

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago
bugherder landing

https://hg.mozilla.org/integration/fx-team/rev/867ef7c9b653
https://hg.mozilla.org/integration/fx-team/rev/9c4a6ed16fbd
https://hg.mozilla.org/integration/fx-team/rev/f1dacf9e859f

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/867ef7c9b653
https://hg.mozilla.org/mozilla-central/rev/9c4a6ed16fbd
https://hg.mozilla.org/mozilla-central/rev/f1dacf9e859f

	Chris Mills [:cmills]
	Comment 18 • 8 years ago

I've documented the update to the storage inspector:

https://developer.mozilla.org/en-US/docs/Tools/Storage_Inspector#IndexedDB

And added a note to the Fx 49 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/49#Developer_Tools

Let me know if this covers all the updates needed currently. Thanks!

	Sebastian Zartner [:sebo] Reporter
	Comment 19 • 8 years ago

The description looks good to me. Thanks Chris!
(I merged the duplicate point about the option to delete the IndexedDB.)

Sebastian

	Md. Rahimul Islam
	Comment 20 • 8 years ago

I have successfully reproduce this bug on firefox nightly 49.0a1 (2016-04-28)
with windows 7 (32 bit)
Mozilla/5.0 (Windows NT 6.1; rv:49.0) Gecko/20100101 Firefox/49.0

I found this fix on latest beta 49.0b5

Mozilla/5.0 (Windows NT 6.1; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID : 20160818050015