Closed Bug 769356 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: sicking, Assigned: khuey)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
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.
Attached patch PatchSplinter Review
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
Closed: 8 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.