The default bug view has changed. See this FAQ.

IndexedDatabaseManager leaks if CancelGetUsageForURI is called before the runnable for GetUsageForURI has been run

RESOLVED FIXED in Firefox 20

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox20 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments, 1 obsolete attachment)

I could only reproduce this by repeatedly opening and closing a window that makes a call to IndexedDatabaseManager::GetUsageForURI followed by CancelGetUsageForURI. I think the issue is that in IndexedDatabaseManager::AsyncUsageRunnable::RunInternal, if the runnable has been cancelled, it exits early, meaning that it hangs on mURI and mCallback and never gets removed from mUsageRunnables.
Created attachment 711448 [details] [diff] [review]
patch to reproduce bug

after applying this patch and building, running `./mach browser/base/content/test/browser_idbbug.js' should reproduce the bug
Created attachment 711449 [details] [diff] [review]
possible fix

this should fix it - I'm not sure if it breaks other things, though
(Assignee)

Updated

4 years ago
Blocks: 821892
Whiteboard: [MemShrink]
Comment on attachment 711449 [details] [diff] [review]
possible fix

I believe this patch is correct.

There's an even worse bug in here.  Not only does the test case cause a leak, it also prevents opening any more databases from that origin until the browser is restarted.
Attachment #711449 - Flags: review+
Created attachment 711612 [details] [diff] [review]
test

Just to be clear, does the patch also fix the worse bug you mentioned?
Anyway, I munged the repro patch into something that I think works as a mochitest for indexedDB. If you would let me know if this looks alright, that would be great. Thanks for your responsiveness on this!
Assignee: nobody → dkeeler
Attachment #711448 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #711612 - Flags: review?(khuey)
(In reply to David Keeler (:keeler) from comment #4)
> Created attachment 711612 [details] [diff] [review]
> test
> 
> Just to be clear, does the patch also fix the worse bug you mentioned?

Yes.
Comment on attachment 711612 [details] [diff] [review]
test

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

I'm not really competent to review browser-chrome tests.
Attachment #711612 - Flags: review?(khuey) → review?(bent.mozilla)
Comment on attachment 711612 [details] [diff] [review]
test

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

Thanks!
Attachment #711612 - Flags: review?(bent.mozilla) → review+
The try run looked good:
https://tbpl.mozilla.org/?tree=Try&rev=c0ed21c1ac11

Checkin:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1fa0280708
https://hg.mozilla.org/integration/mozilla-inbound/rev/783d5b18ee7

Thanks, all!
https://hg.mozilla.org/mozilla-central/rev/cf1fa0280708
https://hg.mozilla.org/mozilla-central/rev/783d5b18ee73
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Updated

4 years ago
Blocks: 767896
Comment on attachment 711449 [details] [diff] [review]
possible fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): indexed db manager
User impact if declined: the test for bug 821892 will leak - if that's going to be uplifted to aurora, we need this too
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #711449 - Flags: approval-mozilla-aurora?
Comment on attachment 711612 [details] [diff] [review]
test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): indexed db manager
User impact if declined: the test for bug 821892 will leak - if that's going to be uplifted to aurora, we need this too
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none
Attachment #711612 - Flags: approval-mozilla-aurora?
Attachment #711449 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #711612 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
taking this for uplift to go along with bug 821892 - which we want on Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/9b6f9b97e938
https://hg.mozilla.org/releases/mozilla-aurora/rev/e841c66bde34
status-firefox20: --- → fixed
Depends on: 842384
Depends on: 968047
You need to log in before you can comment on or make changes to this bug.