479729 - Unable to shutdown using async statements

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P2)

Description

[:Mook]

Attachment: stacks, of main thread + suspicious sqlite thread

sdwilsh told me to file this one, I have no idea what the heck I'm doing ;)

Using his try server build, 2009-02-21_20:53-try-1bb16971859, there's some background thread in an infinite-looking loop (not this bug), and shutdown doesn't work because of that.

Attached stack is of the main thread (which appears to be eating CPU) and what looks like the loopy background sqlite thread.

Comment 1

[Shawn Wilsher :sdwilsh]

Actually, looks like mozStorageTrasnaction keeps sleeping while trying to do the roolback, which means it keeps getting SQLITE_BUSY.  The thread that mozStorageConnection is trying to close won't go away.

Comment 2

[Shawn Wilsher :sdwilsh]

After talking with drh, we need to finalize our statements before we try to rollback.  It thinks there is pending work to do.

Comment 3

[Shawn Wilsher :sdwilsh]

...and returns SQLITE_BUSY as a result.

Comment 4

[Shawn Wilsher :sdwilsh]

Requesting blocking since this could very easily hit anyone who uses this API (multiple statements) and cancels.  I don't think any internal code does this, but add-ons could.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Attachment: candidate unit test, although maybe it's not a good idea

As straight-forward as this problem is (especially after peeking at the sqlite sources to verify the problem), I figured I would try and add a unit test for sanity purposes.  This test hangs (as expected) without the patch, and passes (as expected) with the patch.  In many ways, I feel like this makes it a bad test.  If there's a way to convince xpcshell to quit hard, then this might be an okay test.

(In order to cause the async operation to be long running I populate a table with 256 rows which we then have a SELECT join across 4 times, for an expected result row count of 256**4... it is unlikely/hopeful that we get a chance to cancel() before that completes.  Since it stops doing things when we cancel, this turns out okay-ish.)

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #6)
> result row count of 256**4... it is unlikely/hopeful that we get a chance to
> cancel() before that completes.  Since it stops doing things when we cancel,

Er, I meant to say it is unlikely that we won't get a chance to cancel before it completes / I am hopeful that we get a chance to cancel before it completes.  The cost is that we burn a bunch of CPU cycles for the privilege to actually get to cancel something.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 363790 [details] [diff] [review]
candidate unit test, although maybe it's not a good idea

Yeah, I thought about how to test this but couldn't think of anything that'd be reliable.  r- for the reasons you described.  I don't think this is really testable in a reliable way sadly.

I talked to drh about getting an api addition to track this in the future so in debug builds we can dump out an error at least.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Shawn, I've tested your add-on and I've noticed that when I enter a new url inside the location bar and press enter - nothing happens for about 10-20 seconds. After that the page gets loaded. I had to stop further testing and deactivated the add-on for now.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> Shawn, I've tested your add-on and I've noticed that when I enter a new url
> inside the location bar and press enter - nothing happens for about 10-20
> seconds. After that the page gets loaded. I had to stop further testing and
> deactivated the add-on for now.
not sure this is the bug you meant to comment in...

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh yeah, bug 455555 was my target. Sorry.

Comment 12

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/cb41dd06a36d

Comment 13

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e1f1813889bc

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Any way to verify the fix? Eventually by creating a tryserver build with one of the patches you have used for a trunk tryserver build and which caused this hang? If that would be possible could you prepare such a build?

Comment 15

[Shawn Wilsher :sdwilsh]

I could do that, but the cost seems pretty high when we can just verify that this is fixed with a simple code inspection.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Or is it enough to have your add-on installed and check it this way?

Comment 17

[Shawn Wilsher :sdwilsh]

one of the early versions could trigger this