Last Comment Bug 839193 - IndexedDatabaseManager leaks if CancelGetUsageForURI is called before the runnable for GetUsageForURI has been run
: IndexedDatabaseManager leaks if CancelGetUsageForURI is called before the run...
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: David Keeler [:keeler] (use needinfo?)
:
Mentors:
Depends on: 842384 968047
Blocks: 767896 821892
  Show dependency treegraph
 
Reported: 2013-02-07 11:54 PST by David Keeler [:keeler] (use needinfo?)
Modified: 2014-02-04 22:06 PST (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch to reproduce bug (5.84 KB, patch)
2013-02-07 11:58 PST, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
possible fix (998 bytes, patch)
2013-02-07 11:59 PST, David Keeler [:keeler] (use needinfo?)
khuey: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
test (4.40 KB, patch)
2013-02-07 17:19 PST, David Keeler [:keeler] (use needinfo?)
bent.mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description David Keeler [:keeler] (use needinfo?) 2013-02-07 11:54:16 PST
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.
Comment 1 David Keeler [:keeler] (use needinfo?) 2013-02-07 11:58:49 PST
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
Comment 2 David Keeler [:keeler] (use needinfo?) 2013-02-07 11:59:28 PST
Created attachment 711449 [details] [diff] [review]
possible fix

this should fix it - I'm not sure if it breaks other things, though
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-02-07 15:07:57 PST
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.
Comment 4 David Keeler [:keeler] (use needinfo?) 2013-02-07 17:19:39 PST
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!
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-02-08 01:35:17 PST
(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 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-02-08 01:36:27 PST
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.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-02-08 04:01:32 PST
Comment on attachment 711612 [details] [diff] [review]
test

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

Thanks!
Comment 10 David Keeler [:keeler] (use needinfo?) 2013-02-12 12:02:04 PST
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
Comment 11 David Keeler [:keeler] (use needinfo?) 2013-02-12 12:02:29 PST
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
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-13 12:21:04 PST
taking this for uplift to go along with bug 821892 - which we want on Aurora.

Note You need to log in before you can comment on or make changes to this bug.