Closed Bug 910226 Opened 11 years ago Closed 11 years ago

Permaorange on debug xpcshell: PROCESS-CRASH | /builds/slave/test/build/xpcshell/tests/chat/components/src/test/test_accounts.js | application crashed [@ sqlite3_finalize]

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 26.0

People

(Reporter: jcranmer, Assigned: Yoric)

References

Details

(Keywords: crash, intermittent-failure)

Attachments

(2 files, 3 obsolete files)

Example:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27113155&tree=Thunderbird-Trunk#error0

This looks like a mozilla-central change that broke comm-central. Bug 874814 looks to be the best candidate in the regression range.
Severity: normal → critical
Keywords: crash
Mmmh...
We crash after 5 warnings saying that some stuff needs to be finalized. I'll investigate.
So, it seems that we have statements created by some JS code that are not finalized properly. Normally, this should have caused assertion failures. I believe that, for some reason, until now, they were caught by the garbage-collector and finalized properly at that moment.

With the new behavior of closing, these statements are now auto-finalized when the connection is closed. Once garbage-collection catches up with these statements, it attempts to finalize them, which is not possible anymore because the connection is closed.
There are two errors involved here.

1/ comm-central code should finalize its statements
- http://mxr.mozilla.org/comm-central/source/chat/components/src/imContacts.js#1213 (statement is never closed);
- http://mxr.mozilla.org/comm-central/source/chat/components/src/imContacts.js#1206 (statement is never closed);
- ...

2/ mozStorage should be more robust about preventing these situations.
Attached patch Quick fix to comm-central (obsolete) — Splinter Review
Here's a quick (and untested) fix. I do not have time to test it today, though.
Attachment #796659 - Flags: feedback?(florian)
Comment on attachment 796659 [details] [diff] [review]
Quick fix to comm-central

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

::: chat/components/src/imContacts.js
@@ +143,5 @@
>      let statement = DBConn.createStatement("SELECT id FROM tags where name = :name");
>      statement.params.name = aName;
>      if (!statement.executeStep())
>        return null;
> +    statement.finalize();

In another place with very similar code, you put the finalize statement in a try/finally block.
Attached patch Quick fix to comm-central, v2 (obsolete) — Splinter Review
Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=bc2bd194b7f7
Attachment #796659 - Attachment is obsolete: true
Attachment #796659 - Flags: feedback?(florian)
Attachment #796813 - Flags: review?(florian)
executeAsyncThenFinalize can call finalize immediately after calling executeAsync.  No need to wait for the execution to complete.

So, it seems like the death problem is definitely that

Statement::internalFinalize called by Statement::~Statement does:

  int srv = ::sqlite3_finalize(mDBStatement);
  mDBStatement = nullptr;

regardless of whether the connection is alive or not.  It needs to check the connection's closed state.  (The connection must still be alive.)

It seems like we can add a unit test to verify this becomes a non-crasher:
- Create sync connection
- Create sync statement, keep it alive
- Run asyncClose on connection, wait for the close to notify as complete.
- Finalize sync statement.  Don't crash.
Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=abeeb120fbd6
Assignee: nobody → dteller
Attachment #796813 - Attachment is obsolete: true
Attachment #796813 - Flags: review?(florian)
Attachment #796918 - Flags: review?(florian)
Comment on attachment 796918 [details] [diff] [review]
Fixing statement management in imContacts.js

I dislike the appearance of the code after this change, but I've nothing better to propose, so let's get this permaorange fixed.

Yoric, thank you very much for looking into this so quickly! :-)
Attachment #796918 - Flags: review?(florian) → review+
With all the oranges on Thunderbird-Trunk, let's hope that I'm not breaking something else without noticing.
Keywords: checkin-needed
Same one, with commit message.
Attachment #796918 - Attachment is obsolete: true
Attachment #797145 - Flags: review+
https://hg.mozilla.org/comm-central/rev/520495970114
Status: NEW → RESOLVED
Closed: 11 years ago
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
Version: unspecified → Trunk
Comment on attachment 797145 [details] [diff] [review]
Fixing statement management in imContacts.js, v2

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

::: chat/components/src/imContacts.js
@@ +149,1 @@
>      return this.getTagById(statement.row.id);

This line no longer works as the statement has already been finalized when we reach it.

@@ +1322,1 @@
>      return BuddiesById[statement.row.id];

Same here.

@@ +1344,2 @@
>        buddy =
>          new Buddy(DBConn.lastInsertRowID, normalizedName, name, srvAlias, 0);

This line no longer works because the 'name' and 'srvAlias' variables are now scoped to the try block.
Attachment #797519 - Flags: review? → review+
We fixed another regression caused by this in https://bugzilla.instantbird.org/show_bug.cgi?id=2179
You need to log in before you can comment on or make changes to this bug.