Closed Bug 596776 Opened 11 years ago Closed 10 years ago

IndexedDB: Prevent quota prompt from hanging on shutdown

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch, v1 (obsolete) — Splinter Review
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.
Attachment #475674 - Flags: review?(jonas)
This needs fixed before we ship IMO.
blocking2.0: --- → final+
Attached patch Patch, v1.1Splinter Review
Carrying sicking's r+ forward, needs a slightly new patch based on changes in bug 595253.
Attachment #475674 - Attachment is obsolete: true
Attachment #477329 - Flags: review+
Comment on attachment 477329 [details] [diff] [review]
Patch, v1.1

Gavin, can you look over the js changes?
Attachment #477329 - Flags: review?(gavin.sharp)
Gavin, review ping!
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 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.
Attachment #477329 - Flags: review?(gavin.sharp)
(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 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?
Attachment #477329 - Flags: review?(gavin.sharp)
Comment on attachment 477329 [details] [diff] [review]
Patch, v1.1

Reviewed this in bug 595253.
Attachment #477329 - Flags: review?(gavin.sharp)
Let's call this fixed then.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: IndexedDB
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.