Last Comment Bug 769356 - Calling transaction.abort() should leave transaction.error as null
: Calling transaction.abort() should leave transaction.error as null
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Kyle Huey [:khuey] (
Depends on:
Blocks: 726378
  Show dependency treegraph
Reported: 2012-06-28 10:44 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2012-06-29 11:54 PDT (History)
1 user (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (9.37 KB, patch)
2012-06-28 22:26 PDT, Kyle Huey [:khuey] (
no flags Details | Diff | Splinter Review
Patch (9.38 KB, patch)
2012-06-29 05:36 PDT, Kyle Huey [:khuey] (
jonas: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2012-06-28 10:44:44 PDT
When a transaction is aborted due to an explicit call to transaction.abort(), we should let transaction.error remain null.

Note though that we should not let transaction.error be null if a transaction is aborted due to a "success" or "error" event handler throwing.
Comment 1 Kyle Huey [:khuey] ( 2012-06-28 22:26:30 PDT
Created attachment 637783 [details] [diff] [review]
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-29 03:03:41 PDT
I think this patch is making things more complicated than they need to be. Especially the fact that there are now 4 Abort* functions is unfortunate. And it's confusing that AbortWithCode both acts as an abort function, and as a factory function for creating DOMErrors.

How about instead making AbortWithCode take an nsresult and an optional nsIDOMDOMError*. Then transaction.abort() can call AbortWithCode and pass NS_OK as nsresult. If the nsIDOMDOMError* argument is null, and the nsresult isn't NS_OK, we create the appropriate DOMError.
Comment 3 Kyle Huey [:khuey] ( 2012-06-29 05:36:00 PDT
Created attachment 637866 [details] [diff] [review]

How about this?
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-29 06:08:46 PDT
Comment on attachment 637866 [details] [diff] [review]

Review of attachment 637866 [details] [diff] [review]:

Looks great. Please add a test to ensure that transaction.error is null after a transaction is aborted.

::: dom/indexedDB/IDBTransaction.cpp
@@ +575,2 @@
> +  return AbortInternal(aRequest->GetErrorCode(), error.forget());

Maybe just inline this...

@@ +580,5 @@
> +IDBTransaction::Abort(nsresult aErrorCode)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +  return AbortInternal(aErrorCode, DOMError::CreateForNSResult(aErrorCode));

...and this function. That'll make it more clear what all variants do when looking in IDBTransaction.h
Comment 5 Kyle Huey [:khuey] ( 2012-06-29 08:12:33 PDT
Inlining it leads to include hell with private dom headers in IPC stuff, unfortunately.
Comment 6 Kyle Huey [:khuey] ( 2012-06-29 08:13:07 PDT
And there already is a test in test_transaction_abort.
Comment 7 Kyle Huey [:khuey] ( 2012-06-29 11:54:52 PDT

Note You need to log in before you can comment on or make changes to this bug.