Last Comment Bug 769356 - Calling transaction.abort() should leave transaction.error as null
: Calling transaction.abort() should leave transaction.error as null
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (9.37 KB, patch)
2012-06-28 22:26 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (9.38 KB, patch)
2012-06-29 05:36 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
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] (khuey@mozilla.com) 2012-06-28 22:26:30 PDT
Created attachment 637783 [details] [diff] [review]
Patch
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] (khuey@mozilla.com) 2012-06-29 05:36:00 PDT
Created attachment 637866 [details] [diff] [review]
Patch

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]
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 [:khuey] (khuey@mozilla.com) 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] (khuey@mozilla.com) 2012-06-29 08:13:07 PDT
And there already is a test in test_transaction_abort.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-29 11:54:52 PDT
https://hg.mozilla.org/mozilla-central/rev/1cf5fe8cdb15

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