Closed Bug 672667 Opened 8 years ago Closed 8 years ago

IndexedDB demo causes leaks and never-ending assertions: "Should be impossible!: 'aRecursionDepth > mCreatedRecursionDepth'"

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: luke, Assigned: bent.mozilla)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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.
Still reproduces on tip.
This looks like recursionDepth is 0 when we're processing native events ... that doesn't seem completely insane.  Bent?
I'm getting these assertions too, any started transaction triggers it.
Attached patch Patch, v1 (obsolete) — Splinter Review
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...
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #574165 - Flags: review?(jonas)
Flags: in-litmus?
Comment on attachment 574165 [details] [diff] [review]
Patch, v1

bsmedberg, what do you think about the changes to nsIThread/nsThread?
Attachment #574165 - Flags: superreview?(benjamin)
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 :(
Attachment #574165 - Flags: review?(jonas) → review+
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.
Attachment #574165 - Flags: superreview?(benjamin) → superreview-
Component: DOM → DOM: IndexedDB
Any plans to land?
Actually, I'm working on this today, as chance would have it. Updates soon.
Hot diggidy!
Attached patch Patch, v2Splinter Review
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.
Attachment #574165 - Attachment is obsolete: true
Attachment #616827 - Flags: review?(khuey)
Attachment #616827 - Flags: review?(benjamin)
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.
Attachment #616827 - Flags: review?(smichaud)
Attachment #616827 - Flags: review?(benjamin)
Attachment #616827 - Flags: review+
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.
Attachment #616827 - Flags: review?(smichaud) → review+
Duplicate of this bug: 734770
What's blocking this from landing?
I'm trying to to finish reviews and then I need to figure out how to write a test here.
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
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.
https://hg.mozilla.org/mozilla-central/rev/58d583e7f9b1
https://hg.mozilla.org/mozilla-central/rev/31bc349617c6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.