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

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: luke, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

unspecified
mozilla15
Points:
---
Bug Flags:
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Still reproduces on tip.
This looks like recursionDepth is 0 when we're processing native events ... that doesn't seem completely insane.  Bent?

Comment 3

6 years ago
I'm getting these assertions too, any started transaction triggers it.
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...
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #574165 - Flags: review?(jonas)

Updated

6 years ago
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
(Reporter)

Comment 8

5 years ago
Any plans to land?
Actually, I'm working on this today, as chance would have it. Updates soon.
(Reporter)

Comment 10

5 years ago
Hot diggidy!
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.
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+
Attachment #616827 - Flags: review?(khuey) → review+

Updated

5 years ago
Duplicate of this bug: 734770
Duplicate of this bug: 752171
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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
You need to log in before you can comment on or make changes to this bug.