596776 - IndexedDB: Prevent quota prompt from hanging on shutdown

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

If the window that is prompting is closed we don't currently get any callback from the prompt. That leaves our threads in a nasty state. Attached patch fixes this by watching dom window destruction. Also bulletproofs by allowing the shutdown timer to force a response.

This patch needs bug 595253 applied.

Comment 1

[Johnny Stenback (:jst)]

This needs fixed before we ship IMO.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

Carrying sicking's r+ forward, needs a slightly new patch based on changes in bug 595253.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 477329 [details] [diff] [review]
Patch, v1.1

Gavin, can you look over the js changes?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Gavin, review ping!

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I landed this by accident here:

http://hg.mozilla.org/mozilla-central/rev/1e456c2f2c8f

And then removed the unreviewed JS changes here:

http://hg.mozilla.org/mozilla-central/rev/23a389589086

Sorry about that.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 477329 [details] [diff] [review]
Patch, v1.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+    else if (topic == this._quotaCancel) {
>+      responseTopic = this._quotaResponse;
>+    }

This doesn't look like it serves a purpose, since you don't notify for quotaCancel.

Seems like this might be less confusing if you handled quotaCancel separately rather than trying to share the logic with the prompting in observe(). Not closing over the timeout ID means you couldn't cancel it, but it should be able to deal with running after the notification's already been closed.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #6)
> This doesn't look like it serves a purpose, since you don't notify for
> quotaCancel.

Yes, I do. timeoutNotification() does the call to the observer, with the responseTopic that I have set.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 477329 [details] [diff] [review]
Patch, v1.1

Gavin, can you look over this again? I think it works as it should. Maybe I just need more comments?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 477329 [details] [diff] [review]
Patch, v1.1

Reviewed this in bug 595253.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Let's call this fixed then.