Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sicking, Assigned: khuey)

Tracking

Trunk
mozilla16
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 637783 [details] [diff] [review]
Patch
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #637783 - Flags: review?(jonas)
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.
Created attachment 637866 [details] [diff] [review]
Patch

How about this?
Attachment #637783 - Attachment is obsolete: true
Attachment #637783 - Flags: review?(jonas)
Attachment #637866 - Flags: review?(jonas)
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
Attachment #637866 - Flags: review?(jonas) → review+
Inlining it leads to include hell with private dom headers in IPC stuff, unfortunately.
And there already is a test in test_transaction_abort.
https://hg.mozilla.org/mozilla-central/rev/1cf5fe8cdb15
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.