Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IDBTransaction commit() method #242

Merged
merged 2 commits into from Jan 23, 2019

Conversation

@inexorabletash
Copy link
Member

commented Jun 18, 2018

Per discussion in #234 - add an explicit commit() method to IDBTransaction.

  • Reify an internal "state" for transaction (active, inactive, committing, finished) replacing the active flag.
  • Make transaction life-cycle somewhat more rigorous.
  • Add "processed" state to request, distinct from "done" (when the result has been delivered).
  • Firm up the notion of a transaction's request list w/r/t processing state of requests.
  • Firm up commit/abort steps w/r/t processing state of requests.
  • Add commit() method itself.

Preview | Diff

@inexorabletash inexorabletash requested a review from aliams Jun 18, 2018

@inexorabletash

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2018

@aliams, @pwnall - can you take an initial look?

This needs further discussion, tests, and an implementation before merging, but I wanted draft text to guide those efforts.

@inexorabletash

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

cc: @spanicker and @fmeawad - your input on this CL would be welcome. Note that it's mostly refactor to support the addition.

@brettz9

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

Just a nit observation--some of the new text creates lines going past the usual 80 character limit...

index.bs Outdated
@@ -3142,7 +3150,7 @@ must run these steps:
1. If |store| has been deleted, [=throw=] an
"{{InvalidStateError}}" {{DOMException}}.

1. If |transaction| is not [=transaction/active=], [=throw=] a
4. If |transaction| is not [=transaction/active=], [=throw=] a

This comment has been minimized.

Copy link
@asutherland

asutherland Jun 27, 2018

The 1 changed to a 4 for some reason here but the "transaction/active" => "transaction/state" transform didn't happen. (There are other cases where the pre-patch phrasing continues to exist with the state reference implicit, but in algorithms you seem to have consistently changed transaction/active to be explicitly referenced in terms of transaction/state.)

This comment has been minimized.

Copy link
@inexorabletash

inexorabletash Jun 27, 2018

Author Member

Thanks, fixed in 0b9c57b

Re: state reference implicit - any thoughts on changing the non-algorithm transaction/active references? (There appear to be 11 or so of them) It should probably have a separate term and definition. Maybe "a transaction is said to be accepting requests if its state is active" or something?

@asutherland

This comment has been minimized.

Copy link

commented Jun 27, 2018

I think there's a hole or two in the spec around the "committing" state and aborting, at least as I understood the intent of the commit() changes to be.

Previously, the spec abstracted away most of the realities of the IDB logic running on some other event loop that's asynchronously accessed. A transaction being "finished" was accordingly posed as whether "commit a transaction" or "abort a transaction" had run. "Commit a transaction" now surfaces this reality by introducing the "committing" state so that new requests can't be issued and to more accurately express that the already-enqueued requests could still fail and cause the transaction to abort.

The issues I see with the patch as stands are:

  • abort() post-patch only checks for a state of transaction/finished before throwing. This creates an awkward semantic state where commit(); abort(); could be invoked and neither method would throw because abort() is okay with being invoked with the transaction in the "committing" state, but either one of the two results could happen depending on the implementation.
  • Similarly, it seems like "fire a success event" can still trigger an (ambiguous) abort even after commit() is called. It directly invokes the "abort a transaction" algorithm without any guards, but I think a transaction state check of [committing, finished] is appropriate. ("Fire an error event" does this too, but we would expect the abort to have been internally triggered in that case already?).
  • commit() should probably mention the impact it will have on "success" events if we are going with what I understood. So we'd explicitly say that they will still fire but if they throw, the transaction will not be aborted. Also we'd possibly say that the transaction will still commit even if the events are unable to be fired. For example transaction.commit(); self.close(); in a worker should still cause the transaction to commit.
  • transaction/lifetime should also mention the "committing". Alternately, the "finished" state could perhaps instead be "completed" and the original semantics of "finished" meaning "abort a transaction" or "commit a transaction" having been invoked (at least once) could be restored.
@inexorabletash

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

Thank you very much @asutherland for your detailed feedback. This is extremely helpful, and I'm very happy to have someone else thinking this through. More please!

abort() post-patch only checks for a state of transaction/finished before throwing. This creates an awkward semantic state where commit(); abort(); could be invoked and neither method would throw because abort() is okay with being invoked with the transaction in the "committing" state, but either one of the two results could happen depending on the implementation.

True. For reference, in the Chrome implementation it would be racy - the transaction would commit if there was no pending work to be done so it actually committed before the abort showed up. Work might be pending due to implementation details, e.g. multi-stage commit (e.g. blob copies) or if the transaction hadn't processed requests yet (e.g. the transaction hadn't even started, etc).

I'd be okay with making abort() throw if the state was_ committing_. Thoughts?

Similarly, it seems like "fire a success event" can still trigger an (ambiguous) abort even after commit() is called. It directly invokes the "abort a transaction" algorithm without any guards, but I think a transaction state check of [committing, finished] is appropriate.

Agreed, I just missed that. Refined in 9ba00b5

commit() should probably mention the impact it will have on "success" events

Great point. Added in ef28100 - note that this appears in the "domintro" (non-normative) block, but suggestions for a better location are welcome. (I may have moved too much text out of the normative method descriptions.)

transaction/lifetime should also mention the "committing".

Thanks! Done in ca0f393

Alternately, the "finished" state could perhaps instead be "completed" and the original semantics of "finished" meaning "abort a transaction" or "commit a transaction" having been invoked (at least once) could be restored.

I was thinking about that too. But since "complete" is used as the event for "successfully committed" (vs. '"abort" for "aborted") I didn't want to overload that already confusing term. Alternate ideas?

@asutherland

This comment has been minimized.

Copy link

commented Jul 2, 2018

Great cleanups! They all look good to me.

I'd be okay with making abort() throw if the state was_ committing_. Thoughts?

This sounds like the right choice to me.

I was thinking about that too. But since "complete" is used as the event for "successfully committed" (vs. "abort" for "aborted") I didn't want to overload that already confusing term. Alternate ideas?

Given the cleanups you've made, I think my "Alternately..." sentence is nicely mooted.

@inexorabletash

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2018

I'd be okay with making abort() throw if the state was committing. Thoughts?
This sounds like the right choice to me.

Done in a40328b ... but in a way that may not be web-compatible, at least not without further rework of the PR. Today you can call abort() at any point, even when the transaction is inactive, and thus e.g. when the auto-commit has already begun. Since the PR flips the transaction into the new committing state, that would now throw. Requiring abort() to be called only when the transaction is active would have been a better design from the beginning to avoid a race, but it's probably too late for that.

I don't want to introduce yet another state, so we'll see how this goes.

@asutherland

This comment has been minimized.

Copy link

commented Jul 2, 2018

I agree that additional constraint on abort() would have been nice.

Spec-wise, I think we're in the clear because, modulo some phrasing ambiguity, I think one can interpret the auto-commit step 7 in IDB 2.0 2.7.1 as saying "run commit a transaction, that could still fail over to run abort a transaction". And I at least interpreted running either algorithm as marking the transaction finished given how the spec abstracted away the actual backend database work.

Web-wise, it's at least the case that Firefox's auto-commit logic already sets the transaction's internal state to COMMITTING which will cause content calls to abort() to throw. If Chrome has a race at the current time as it relates to abort(), then perhaps you can think of the change as aligning with Firefox's non-racing implementation ;).

@pwnall
pwnall approved these changes Sep 13, 2018
Copy link

left a comment

The text LGTM, from the perspective of an implementer who's not super-versed in spec subtleties.

index.bs Outdated
database.
When a transaction is created its [=transaction/state=] is initially [=transaction/active=].

1. When an implementation is able to enforce the constraints defined for the transaction [=transaction/scope=] and [=transaction/mode=], defined [below](#transaction-scheduling], the implementation must [=queue a task=] to <dfn lt="start|started">start</dfn> the transaction asynchronously.

This comment has been minimized.

Copy link
@pwnall

pwnall Sep 13, 2018

nit: This line is really long and thus hard to review on GitHub.

This comment has been minimized.

Copy link
@inexorabletash

inexorabletash Sep 13, 2018

Author Member

Tab has been advocating for line breaking at logical (sentence, clause) boundaries rather than to just wrap at 80 columns and reflowing, since that makes future diffs easier to parse. That said, there are some places this could break.

@inexorabletash inexorabletash force-pushed the commit branch 2 times, most recently from 59b2ac8 to 86e7337 Sep 17, 2018

@andreas-butler

This comment has been minimized.

Copy link

commented Oct 5, 2018

Hi all, chrome is getting ready to move forward with implementation of IDBTransaction.comit(). As such any new or evolved thoughts/input regarding the proposed spec changes would be much appreciated at the time, otherwise our intention is for our implementation to comply with the details of this PR.

@asutherland @beidson @aliams @nolanlawson

@asutherland

This comment has been minimized.

Copy link

commented Oct 6, 2018

Can we mark the new commit() method as [SecureContext]? I was just reviewing our (Mozilla's) exposure guidelines and I was reminded of our Secure Contexts Everywhere policy.

@aliams

This comment has been minimized.

Copy link

commented Oct 9, 2018

@asutherland are you concerned about potential abuse of this method?

@asutherland

This comment has been minimized.

Copy link

commented Oct 9, 2018

@aliams No, Mozilla just has a bias towards requiring SecureContext on new stuff. I think the blog post and linked TAG github issue covers it. And in the case of something like 'commit' which currently would stand in isolation and need to be feature-detected, in theory Firefox could ship it SecureContext even if no one else did.

@aliams

This comment has been minimized.

Copy link

commented Oct 9, 2018

Based on my reading of the revisions, it sounds like calling commit would actually flush changes to disk. I think that there's a concern with allowing malicious websites to flush to disk as they please because they'd be able to cause hardware wear among other things.

@pwnall

This comment has been minimized.

Copy link

commented Oct 9, 2018

@aliams My understanding of this feature is that it lets sites hint to the implementation that they're done with the transaction. It's intended to enable optimizations, not to be a new capability.

A malicious site can still cause a lot of disk writes by opening up many transactions and letting them auto-commit. Committing is still asynchronous, so the implementation has the opportunity to rate-limit and/or batch transactions.

Does this help reason about abuse potential for the new API?

@aliams

This comment has been minimized.

Copy link

commented Oct 9, 2018

The spec currently states (under 2.7.1):

That is, either all of the changes must be written, or if an error occurs, such as a disk write error, the implementation must not write any of the changes to the database.

This would seem to indicate that we'd need to actually flush to disk?

@aliams

This comment has been minimized.

Copy link

commented Oct 9, 2018

Reading more into this, it seems that @pwnall's statements make sense. Here's the relevant part:

An explicit call to commit() will initiate a commit

(emphasis on initiate)

@pwnall

This comment has been minimized.

Copy link

commented Oct 9, 2018

@aliams Thank you for explaining!

I read that paragraph as stating that the transaction must be committed in an atomic write. I understood the sentence you quoted as a brief explanation what "atomic" means. I think the intent is only to state that the IDB API user should not be able to observe any partial writes.

@inexorabletash inexorabletash force-pushed the commit branch 4 times, most recently from 0a269fa to dd5d067 Nov 2, 2018

@inexorabletash

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

I went ahead and merged this because we now have tests and an experimental implementation. There are outstanding issues (e.g. @asutherland's query about [SecureContext] above) and I'm still not completely thrilled with the spec's Transaction section (which IMHO is an improvement over what was there), but I think we can iterate on the spec and additional issues.

I'll file one for [SecureContext] on commit().

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 23, 2019
[IndexedDB]: Explicit commit error timing handling.
When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
GitHub Discussion: w3c/IndexedDB#242

@inexorabletash inexorabletash deleted the commit branch Jan 23, 2019

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 23, 2019
[IndexedDB]: Explicit commit error timing handling.
When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
GitHub Discussion: w3c/IndexedDB#242
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 24, 2019
[IndexedDB]: Explicit commit error timing handling.
When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 24, 2019
[IndexedDB]: Explicit commit error timing handling.
When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}
chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Jan 24, 2019
[IndexedDB]: Explicit commit error timing handling.
When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 5, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 5, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Feb 6, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Feb 6, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 7, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 7, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Feb 8, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Feb 8, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761

1. Unset the [=/transaction=]'s [=transaction/active flag=] and run the
1. Set the [=/transaction=]'s [=transaction/state=] to [=transaction/active=] and run the

This comment has been minimized.

Copy link
@brettz9

brettz9 Mar 21, 2019

Contributor

Shouldn't this say to set the transaction state to inactive?

This comment has been minimized.

Copy link
@inexorabletash

inexorabletash Mar 21, 2019

Author Member

Yes, nice catch.

inexorabletash added a commit that referenced this pull request Mar 21, 2019
`abort()` to set transaction state to inactive (#258)
This was introduced by #242 as part of a refactor which changed the wording from "Unset the transaction's active flag" to "Set the transaction's state to active". Oops! This corrects the refactor.
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 11, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709

UltraBlame original commit: 0af13c7830739101a3df4966589731de34f3145a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 11, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761

UltraBlame original commit: e16198c2b2ad50d3dd5a027245381842c493176e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 11, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709

UltraBlame original commit: f1961afe6051c5b0249ab2facb5bd3182bb4c222
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Sep 11, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761

UltraBlame original commit: a87a214e284e09b1a0b92c67da0c216fb669b979
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 11, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709

UltraBlame original commit: 0af13c7830739101a3df4966589731de34f3145a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 11, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761

UltraBlame original commit: e16198c2b2ad50d3dd5a027245381842c493176e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 11, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709

UltraBlame original commit: f1961afe6051c5b0249ab2facb5bd3182bb4c222
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Sep 11, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761

UltraBlame original commit: a87a214e284e09b1a0b92c67da0c216fb669b979
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 11, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709

UltraBlame original commit: 0af13c7830739101a3df4966589731de34f3145a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 11, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761

UltraBlame original commit: e16198c2b2ad50d3dd5a027245381842c493176e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 11, 2019
Bug 1517688 [wpt PR 14709] - [IndexedDB]: Add commit check to abortin…
…g txns., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Add commit check to aborting txns.

A check is added to ensure that transactions that have already been sent
a commit signal from the front end (for whatever reason) are committed
instead of aborted in the event of an abort.

Spec Change: w3c/IndexedDB#242

Explainer: https://andreas-butler.github.io/idb-transaction-commit/EXPLAINER
Change-Id: I942136c1bbfb8a5347bf7cf1f9702f5cbff66521
Reviewed-on: https://chromium-review.googlesource.com/c/1330692
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619859}

--

wpt-commits: 6d9079e341f777ae47b2f39c9d116bfc77d141e2
wpt-pr: 14709

UltraBlame original commit: f1961afe6051c5b0249ab2facb5bd3182bb4c222
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 11, 2019
Bug 1523562 [wpt PR 14761] - [IndexedDB]: Explicit commit error timin…
…g handling., a=testonly

Automatic update from web-platform-tests
[IndexedDB]: Explicit commit error timing handling.

When a transaction is explicitly committed, there may yet be unhandled
errors that the browser has sent but the renderer has not yet seen. This
can cause strange behaviour (see the Github discussion link below). This
patch keeps track of the 'handled errors' in the renderer and the 'sent
errors' in the backend. The 'handled errors' number is sent when the
transaction is explicitly committed, and the browser can compare this
with the 'sent errors'. If they don't match, the transaction is aborted

GitHub Discussion: w3c/IndexedDB#242

Bug: 911877
Change-Id: I7ea7b9e20c70528de3f363e961f87a3d8f5798d3
Reviewed-on: https://chromium-review.googlesource.com/c/1378806
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Andreas Butler <andreasbutler@google.com>
Cr-Commit-Position: refs/heads/master@{#625780}

--

wpt-commits: b0fbbb9ff451b18bf8a69fd54027bf59d21c2667
wpt-pr: 14761

UltraBlame original commit: a87a214e284e09b1a0b92c67da0c216fb669b979
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.