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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

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.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Smaug, can you look over the event changes? It seemed pretty straightforward, just want to make sure you agree.

Comment 2

[Johnny Stenback (:jst)]

This is part of the last set of IndexedDB bugs we've deemed necessary in order
to ship 2.0, blocking beta9.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

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

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.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Smaug, how's this?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/b01a871ee77a

Comment 8

[LegNeato]

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)