769356 - Calling transaction.abort() should leave transaction.error as null

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

How about this?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 637866 [details] [diff] [review]
Patch

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 (Exited; not receiving bugmail, old account, do not use)]

Inlining it leads to include hell with private dom headers in IPC stuff, unfortunately.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And there already is a test in test_transaction_abort.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/1cf5fe8cdb15