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
5 years ago
4 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

5 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: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Updated

5 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.