Last Comment Bug 672667 - IndexedDB demo causes leaks and never-ending assertions: "Should be impossible!: 'aRecursionDepth > mCreatedRecursionDepth'"
: IndexedDB demo causes leaks and never-ending assertions: "Should be impossibl...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
: Hsin-Yi Tsai (OOO until Dec. 19) [:hsinyi]
Mentors:
http://www.html5rocks.com/en/tutorial...
: 734770 752171 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-19 15:47 PDT by Luke Wagner [:luke]
Modified: 2012-05-08 06:12 PDT (History)
12 users (show)
Ms2ger: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, v1 (12.29 KB, patch)
2011-11-13 10:26 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
benjamin: superreview-
Details | Diff | Splinter Review
Patch, v2 (22.04 KB, patch)
2012-04-19 17:58 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
benjamin: review+
smichaud: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-07-19 15:47:52 PDT
This is reproducible from a clean profile on a fresh debug build off mozilla-inbound:
1. http://www.html5rocks.com/en/tutorials/indexeddb/todo
2. click "Allow"
3. scroll down and click "Add TODO Item"

After that, I get endless spew alternating asserts at two lines:
###!!! ASSERTION: Should be impossible!: 'aRecursionDepth > mCreatedRecursionDepth', file dom/indexedDB/IDBTransaction.cpp, line 873
###!!! ASSERTION: Should be impossible!: 'aRecursionDepth >= mCreatedRecursionDepth', file dom/indexedDB/IDBTransaction.cpp, line 885

When the browser is closed, a bunch of urls are printed as leaked, many including html5rocks.com.
Comment 1 Luke Wagner [:luke] 2011-08-22 10:59:30 PDT
Still reproduces on tip.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-22 16:10:28 PDT
This looks like recursionDepth is 0 when we're processing native events ... that doesn't seem completely insane.  Bent?
Comment 3 Jan Varga [:janv] 2011-09-21 01:27:53 PDT
I'm getting these assertions too, any started transaction triggers it.
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-13 10:26:58 PST
Created attachment 574165 [details] [diff] [review]
Patch, v1

Ok, finally figured out how to reproduce this. Transaction creation has to come from a native event (like a mouse click handler) in order to get this problem. Not sure how we can test this since any simulated native events will not really be native events...
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-14 11:22:59 PST
Comment on attachment 574165 [details] [diff] [review]
Patch, v1

bsmedberg, what do you think about the changes to nsIThread/nsThread?
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-16 11:42:33 PST
Comment on attachment 574165 [details] [diff] [review]
Patch, v1

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

r=me, but we need a litmus test for this. And bsmedberg's sr.

Also, would be lovely to make native events not be so different from runnables :(
Comment 7 Benjamin Smedberg [:bsmedberg] 2011-11-21 09:22:15 PST
Comment on attachment 574165 [details] [diff] [review]
Patch, v1

As I mentioned on IRC, I think this API is too hacky and bakes in details about how our thread observers work which are likely to change. I think instead there should be an API like nsIAppShell.runInStableState except it runs in the current event loop nesting depth. This may require teaching appshell and nsThread to cooperate in their understanding of nesting depth.
Comment 8 Luke Wagner [:luke] 2012-04-05 23:10:36 PDT
Any plans to land?
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-06 09:17:48 PDT
Actually, I'm working on this today, as chance would have it. Updates soon.
Comment 10 Luke Wagner [:luke] 2012-04-06 10:03:47 PDT
Hot diggidy!
Comment 11 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-04-19 17:58:51 PDT
Created attachment 616827 [details] [diff] [review]
Patch, v2

Ok, this patch seems to pass try and everything. khuey, can you check the indexeddb changes, and bsmedberg the appshell changes?

As requested, this is similar to the RunInStableState, and it even reuses that machinery. The main difference is that it ensures we run before the next native event, and I made that comment explicitly in the idl.
Comment 12 Benjamin Smedberg [:bsmedberg] 2012-04-20 10:48:19 PDT
Comment on attachment 616827 [details] [diff] [review]
Patch, v2

ok, this looks reasonable, although I'd like somebody else to also look it over. In addition as discussed on IRC, this really needs tests, because it's something that I suspect we could break pretty easily.

I'm a little worried that because the mac appshell is significantly different (it doesn't run the XPCOM event loop, it runs a native event loop and does XPCOM event processing internal to that loop) that this might not behave the same way. smichaud should understand what is being done here and confirm that it will behave correctly on mac.
Comment 13 Steven Michaud [:smichaud] (Retired) 2012-04-20 13:20:33 PDT
Comment on attachment 616827 [details] [diff] [review]
Patch, v2

I only really looked at this patch's changes to nsBaseAppShell, and didn't make any attempt to test it.  Nor (I must admit) do I fully understand why those changes help.

But I don't see anything in the patch that's likely to cause trouble specifically on OS X.

Yes, the Mac appshell doesn't ever call nsBaseAppShell::Run().  But it has it's own way to process Gecko events in a timely and efficient manner, and I don't see this patch interfering with it.

Native events might get processed while an event that's being run "before the next event" is being handled.  But this is (I think) both expected and desirable.
Comment 14 Blake Kaplan (:mrbkap) (PTO until Jan. 2, 2017) 2012-05-04 06:40:18 PDT
*** Bug 734770 has been marked as a duplicate of this bug. ***
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-05 07:14:27 PDT
*** Bug 752171 has been marked as a duplicate of this bug. ***
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-05 07:14:51 PDT
What's blocking this from landing?
Comment 17 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-05 09:57:02 PDT
I'm trying to to finish reviews and then I need to figure out how to write a test here.
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-07 16:44:25 PDT
Ok, so I got a cpp test file that works on windows. It will need some mac/linux love (i don't know how to fake native events on those platforms). I think this is good enough to land though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/58d583e7f9b1
Comment 19 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-07 16:47:45 PDT
Oh, to explain a little better, nsIWidget has SynthesizeNativeKeyEvent but it is synchronous. Calling it from a gecko event defeats the purpose of this test (which is to ensure correct behavior when there are no gecko events on the stack). To get around this on Windows I used a native Timer event. We will need something similar to that for mac/linux.
Comment 21 Ted Mielczarek [:ted.mielczarek] 2012-05-08 06:12:32 PDT
You might be able to use the technique I use in the event tracer code. Here's the mac and gtk2 implementations:
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/WidgetTraceEvent.mm#93
http://mxr.mozilla.org/mozilla-central/source/widget/gtk2/WidgetTraceEvent.cpp#90

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