Closed Bug 618196 Opened 9 years ago Closed 9 years ago

IndexedDB: Error events and exceptions thrown during success events should abort transactions

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch, v1 (obsolete) — Splinter Review
Per spec, now an error event will abort its transaction by default unless event.preventDefault() is called. Similarly, throwing an exception during a success callback will abort its transaction.
Attachment #496705 - Flags: review?(jonas)
Attachment #496705 - Flags: review?(Olli.Pettay)
Smaug, can you look over the event changes? It seemed pretty straightforward, just want to make sure you agree.
This is part of the last set of IndexedDB bugs we've deemed necessary in order
to ship 2.0, blocking beta9.
blocking2.0: ? → beta9+
Hmm, this changes nsIPrivateDOMEvent interface which is, despite its name, public.
And it is used by extensions.

So, could you just add NS_EVENT_FLAG_EXCEPTION_THROWN and then
access nsEvent->flags in AsyncConnectionHelper::OnSuccess.
A bit ugly but safer for extension.
Attachment #496705 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 496705 [details] [diff] [review]
Patch, v1

>@@ -375,35 +375,58 @@ AsyncConnectionHelper::OnSuccess(nsIDOME
...
>+  if (privateEvent->ExceptionWasThrown() &&
>+      mTransaction &&
>+      mTransaction->TransactionIsOpen()) {
>+    rv = mTransaction->Abort();
>+    NS_ENSURE_SUCCESS(rv, rv);

Can you assert that if the transaction isn't open, it is because it was aborted?


> AsyncConnectionHelper::OnError(nsIDOMEventTarget* aTarget,
...
>+  PRBool doDefault;
>+  nsresult rv = aTarget->DispatchEvent(event, &doDefault);
>+  if (NS_SUCCEEDED(rv)) {
>+    if (doDefault &&
>+        mTransaction &&
>+        mTransaction->TransactionIsOpen() &&
>+        NS_FAILED(mTransaction->Abort())) {
>+      NS_WARNING("Failed to abort transaction!");
>+    }

Same thing here.

r=me with those things fixed, and fix the issue Smaug points out.
Attachment #496705 - Flags: review?(jonas) → review+
Attached patch Patch, v2Splinter Review
Smaug, how's this?
Attachment #496705 - Attachment is obsolete: true
Attachment #497339 - Flags: review?(Olli.Pettay)
Comment on attachment 497339 [details] [diff] [review]
Patch, v2


>-  PRBool dummy;
>-  aTarget->DispatchEvent(event, &dummy);
>+  PRBool doDefault;
>+  nsresult rv = aTarget->DispatchEvent(event, &doDefault);
>+  if (NS_SUCCEEDED(rv)) {
>+    if (doDefault &&
>+        mTransaction &&
>+        mTransaction->TransactionIsOpen() &&
>+        NS_FAILED(mTransaction->Abort())) {
>+      NS_WARNING("Failed to abort transaction!");
>+    }
>+  }
>+  else {
>+    NS_WARNING("DispatchEvent failed!");
>+  }

Nit, usually the coding style is
if () {
...
} else {
...
}

Not sure what style IndexDB code uses.
Attachment #497339 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/b01a871ee77a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
blocking2.0: beta9+ → betaN+
Component: DOM → DOM: IndexedDB
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.