Bug 15976 – Throwing an exception within Database.transaction ASSERTs then crashes

• Component: DOM
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

Throwing an exception within Database.transaction ASSERTs then crashes. This can happen if there was an explicit throw statement, or if there was a type mismatch error or something similar.

[Adam Roben (:aroben)]

Created attachment 17251 [details]
testcase

[Brady Eidson]

I made an assumption that the transaction error callback would only be scheduled, therefore invoked, if it was actually set.

The problem is when we make the transaction callback and it immediately invokes the transaction error callback, the assumption I made is no longer valid.

Basically, it's just a bogus assert the logic should change to involve a null check.  The spec already accounts for this

[Brady Eidson]

Created attachment 17254 [details]
Proposed fix

[Adam Roben (:aroben)]

Comment on attachment 17254 [details]
Proposed fix

+        m_transactionError = new SQLError(0, "the SQLTransactionCallback was null or threw an exception");

Do we need to localize this string? Should "the" be capitalized? Where is this text used?

It doesn't look like m_transactionError gets set everywhere deliverTransactionErrorCallback() is called -- do we need to do it elsewhere? What's the rule?

r- until there's a regression test (you should be able to turn the attached testcase into a test pretty easily).

[Brady Eidson]

> Do we need to localize this string? 
no (not required, and we don't for any other DOM error messages)

> Should "the" be capitalized? 
no (we don't for any other DOM error messages)

> Where is this text used?
Wherever the web developer chooses - it's an attribute on the SQLError they get, in addition to the error code (0, in this case)

> It doesn't look like m_transactionError gets set everywhere
deliverTransactionErrorCallback() is called -- do we need to do it elsewhere?
What's the rule?

Actually it was set everywhere else - The only other entry point to deliverTransactonErrorCallback() is through handleTransactionError(), and a quick audit of each site that calls that shows the error being set first.

This was a unique case, jumping from one callback straight to the error callback.  What the assertion was designed to catch!

I overlooked the fact that this was easily layout-testable since I still haven't gotten to DRT infrastructure for a real database test suite, whereas this one is just a "crash or doesn't" test.  I will add that and put a new patch up

[Brady Eidson]

Created attachment 17260 [details]
Proposed fix w/ layout test

[Adam Roben (:aroben)]

Comment on attachment 17260 [details]
Proposed fix w/ layout test

r=me

We should probably add a way for DRT to clear databases created by tests.

[Brady Eidson]

Thanks for the review.

I plan on augmenting DRT thouroughly when adding a database layout test suite, and that is one enhancement I had in mind!

[Mark Rowe (bdash)]

This was landed in r27784.