Implement IDBTransaction commit() method
Categories
(Core :: Storage: IndexedDB, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: asuth, Assigned: sg)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(16 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
On https://github.com/w3c/IndexedDB/issues/234 with spec changes in https://github.com/w3c/IndexedDB/pull/242 the plan is to add a commit() method to IDBTransaction that allows for performance optimizations and more predictable behavior when a global is going away. I just realized while pondering sending an intent-to-implement email that we haven't determined in the PR whether or not to mark the new method with [SecureContext] so I raised the issue. Once that's resolved, we should send an intent-to-implement.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
The question of whether or not the method should be marked with [SecureContext] is still an open issue: https://github.com/w3c/IndexedDB/issues/254 (as of now, it is not).
Should we proceed with this anyway?
Reporter | ||
Comment 2•5 years ago
|
||
Restrict to SecureContext for now. We can always relax that, but it's feature-detectable for content, so even if Chrome doesn't restrict it, we can. We'll discuss at TPAC either way.
Reporter | ||
Comment 3•5 years ago
|
||
Uh, and it's fine to send an intent to implement and indicate that the spec decision is still up in the air but we believe we can ship it SecureContext regardless.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)
Restrict to SecureContext for now.
The web-platform-tests won't work as-is if I declare the method with SecureContext.
Assignee | ||
Comment 5•5 years ago
|
||
What exactly means intent-to-implement here? Where do I send that, is there some template for that?
Reporter | ||
Comment 6•5 years ago
|
||
https://wiki.mozilla.org/ExposureGuidelines covers the "intent" emails and templates.
(In reply to Simon Giesecke [:sg] [he/him] from comment #4)
The web-platform-tests won't work as-is if I declare the method with SecureContext.
Oh, right. I was thinking there'd just be a single failure, but of course if all the tests usually run non-secure then we'd need to change that, etc.
I suppose for now don't mark with SecureContext and maybe we should just send an intent to implement without SecureContext and see if anyone is particularly concerned. I feel like this exists in a bit of a gray space.
Assignee | ||
Comment 7•5 years ago
|
||
Assignee | ||
Comment 8•5 years ago
|
||
Depends on D46275
Assignee | ||
Comment 9•5 years ago
|
||
Depends on D46276
Assignee | ||
Comment 10•5 years ago
|
||
Assignee | ||
Comment 11•5 years ago
|
||
Depends on D46945
Assignee | ||
Comment 12•5 years ago
|
||
Depends on D46946
Assignee | ||
Comment 13•5 years ago
|
||
Depends on D46947
Assignee | ||
Comment 14•5 years ago
|
||
Depends on D46948
Assignee | ||
Comment 15•5 years ago
|
||
Depends on D46949
Assignee | ||
Comment 16•5 years ago
|
||
Depends on D47230
Assignee | ||
Comment 17•5 years ago
|
||
Depends on D47231
Comment 18•5 years ago
|
||
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/49422c0bd800 Remove obsolete test cases from test exclusion list. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/918242871866 Removed extra word in test case description. r=ttung,asuth
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19666 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/19666 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/amfNtQ3JRjiwgyQlrdFDww)
Comment 21•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/49422c0bd800
https://hg.mozilla.org/mozilla-central/rev/918242871866
Assignee | ||
Comment 22•5 years ago
|
||
(In reply to Natalia Csoregi [:nataliaCs] from comment #21)
https://hg.mozilla.org/mozilla-central/rev/49422c0bd800
https://hg.mozilla.org/mozilla-central/rev/918242871866
There are still open patches attached, so this bug is not yet FIXED.
Updated•5 years ago
|
Upstream PR merged by jgraham
Updated•5 years ago
|
Assignee | ||
Comment 24•5 years ago
|
||
Web-platform-test failure is a known Chrome instability as of https://github.com/web-platform-tests/wpt/pull/19666#issuecomment-542187841, upstream PR was merged.
Assignee | ||
Comment 25•4 years ago
|
||
Depends on D47232
Comment 26•4 years ago
|
||
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3d9e86698c9f Use const where easily possible. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/3f96e71be2e3 Extracted HasTransactionChild and DoWithTransactionChild functions. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/562d3cda9fa3 Minor improvements. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/98ac48b5f1ef Replace custom for loops by range-based for and appropriate algorithms. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/be22a95de04b Extracted GetIndexedDBThreadLocal function. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/7304fae8c436 Use IDBTransaction::IsWriteAllowed to simplify code. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/7056932f6422 Added explaining comments. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/15de0d72f1c4 Mark group of methods exposed via webidl. r=ttung,asuth
Comment 27•4 years ago
|
||
Backed out 8 changesets (Bug 1497007) for causing build bustages in /builds/worker/workspace/build/src/dom/indexedDB/IDBTransaction.cpp CLOSED TREE
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=275322946&repo=autoland&lineNumber=35478
Assignee | ||
Comment 28•4 years ago
|
||
I think this is caused by this gcc bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66735, which is fixed in gcc 7. I will try to accomodate for a workaround for the time being.
Comment 29•4 years ago
|
||
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e58433e75e95 Backed out 8 changesets for causing build bustages in /builds/worker/workspace/build/src/dom/indexedDB/IDBTransaction.cpp CLOSED TREE
Comment 30•4 years ago
|
||
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/63821de85ac5 Use const where easily possible. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/badb3cb7e361 Extracted HasTransactionChild and DoWithTransactionChild functions. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/44af22f9c5f4 Minor improvements. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/4ea3f8367672 Replace custom for loops by range-based for and appropriate algorithms. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/9c38294bdc94 Extracted GetIndexedDBThreadLocal function. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/6fa7cace6260 Use IDBTransaction::IsWriteAllowed to simplify code. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/a470382c3253 Added explaining comments. r=ttung,asuth https://hg.mozilla.org/integration/autoland/rev/aab04214324f Mark group of methods exposed via webidl. r=ttung,asuth
Assignee | ||
Updated•4 years ago
|
Comment 31•4 years ago
|
||
Method documented; see https://github.com/mdn/sprints/issues/2269#issuecomment-552598975 for the full details.
Let me know if you think this needs any more updates. Thanks!
Comment 32•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/63821de85ac5
https://hg.mozilla.org/mozilla-central/rev/badb3cb7e361
https://hg.mozilla.org/mozilla-central/rev/44af22f9c5f4
https://hg.mozilla.org/mozilla-central/rev/4ea3f8367672
https://hg.mozilla.org/mozilla-central/rev/9c38294bdc94
https://hg.mozilla.org/mozilla-central/rev/6fa7cace6260
https://hg.mozilla.org/mozilla-central/rev/a470382c3253
https://hg.mozilla.org/mozilla-central/rev/aab04214324f
Comment 33•4 years ago
|
||
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f2db82830fec Improve exception messages. r=ttung
Comment 34•4 years ago
|
||
bugherder |
Assignee | ||
Comment 35•4 years ago
|
||
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #31)
Method documented; see https://github.com/mdn/sprints/issues/2269#issuecomment-552598975 for the full details.
Let me know if you think this needs any more updates. Thanks!
This has not been completely implemented and has not yet shipped, but the MDN page https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction/commit claims it is available in FF 71.
Comment 36•4 years ago
|
||
(In reply to Simon Giesecke [:sg] [he/him] from comment #35)
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #31)
Method documented; see https://github.com/mdn/sprints/issues/2269#issuecomment-552598975 for the full details.
Let me know if you think this needs any more updates. Thanks!
This has not been completely implemented and has not yet shipped, but the MDN page https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction/commit claims it is available in FF 71.
Ah, sorry, my bad. This being the case, I've reverted the change to browser-compat-data:
https://github.com/mdn/browser-compat-data/pull/5231
And removed it from the Fx 71 rel notes:
https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/71
Updated•4 years ago
|
Assignee | ||
Comment 37•4 years ago
|
||
Firefox doesn't allow running readwrite transactions in parallel, even with a
different scope. For the purpose of this particular test, it shouldn't matter
if it is a readwrite or readonly transaction.
Assignee | ||
Comment 38•4 years ago
|
||
Depends on D46277
Updated•4 years ago
|
Comment 39•4 years ago
|
||
Comment 40•4 years ago
|
||
bugherder |
Assignee | ||
Comment 41•4 years ago
|
||
Also fix the behaviour of IDBTransaction::CanAcceptRequests, which, after the
new state model introduced along with the addition of IDBTransaction.commit to
the spec, should only depend on mReadyState. This makes the mCreated flag
redundant, which is removed by this patch.
Assignee | ||
Comment 42•4 years ago
|
||
Depends on D55673
Comment 43•4 years ago
|
||
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/787c340dd3d0 Change transaction in different scope to a readonly transaction. r=dom-workers-and-storage-reviewers,asuth,janv https://hg.mozilla.org/integration/autoland/rev/99fc4eedacc7 Fix state model, return to Inactive state after creation when returning to event loop. r=dom-workers-and-storage-reviewers,asuth https://hg.mozilla.org/integration/autoland/rev/72bc4f39b659 Implemented IDBTransaction.commit. r=dom-workers-and-storage-reviewers,ytausky
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21160 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment 46•4 years ago
|
||
Backed out for assertion failures on IDBTransaction.cpp
backout: https://hg.mozilla.org/integration/autoland/rev/41ae9684c66e50558e5cd6d2f0397bd9ab2b09eb
failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284840146&repo=autoland&lineNumber=21297
[task 2020-01-14T13:12:10.410Z] 13:12:10 INFO - GECKO(4651) | Assertion failure: mWasExplicitlyCommitted, at /builds/worker/workspace/build/src/dom/indexedDB/IDBTransaction.cpp:983
[task 2020-01-14T13:12:10.411Z] 13:12:10 INFO - GECKO(4651) | #01: non-virtual thunk to mozilla::dom::IDBTransaction::Run() [dom/indexedDB/IDBTransaction.cpp:0]
[task 2020-01-14T13:12:10.411Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.412Z] 13:12:10 INFO - GECKO(4651) | #02: mozilla::CycleCollectedJSContext::CleanupIDBTransactions(unsigned int) [xpcom/base/CycleCollectedJSContext.cpp:420]
[task 2020-01-14T13:12:10.412Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.413Z] 13:12:10 INFO - GECKO(4651) | #03: mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) [xpcom/base/CycleCollectedJSContext.cpp:567]
[task 2020-01-14T13:12:10.413Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.414Z] 13:12:10 INFO - GECKO(4651) | #04: mozilla::dom::CallbackObject::CallSetup::~CallSetup() [dom/bindings/CallbackObject.cpp:415]
[task 2020-01-14T13:12:10.414Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.414Z] 13:12:10 INFO - GECKO(4651) | #05: mozilla::dom::PromiseJobCallback::Call(mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) [s3:gecko-generated-sources:8ff597fee16137a41c5c768d4731e63150ba2f32aaccb2a1985be307f0fad868e61bb63f08de085a377133bcfcb41787e072b5d1f0cda75a29d00492f676fce1/dist/include/mozilla/dom/PromiseBinding.h::92]
[task 2020-01-14T13:12:10.414Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #06: mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) [xpcom/base/CycleCollectedJSContext.cpp:205]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #07: mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) [xpcom/base/CycleCollectedJSContext.cpp:624]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #08: mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) [xpcom/base/CycleCollectedJSContext.cpp:455]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #09: XPCJSContext::AfterProcessTask(unsigned int) [js/xpconnect/src/XPCJSContext.cpp:1328]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #10: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:0]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #11: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:486]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #12: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:87]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #13: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #14: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #15: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:139]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #16: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:273]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #17: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4599]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #18: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:4736]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #19: XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:4817]
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #20: _fini
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #21: libc.so.6 + 0x20830
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO - GECKO(4651) | #22: _fini
[task 2020-01-14T13:12:10.415Z] 13:12:10 INFO -
[task 2020-01-14T13:12:10.430Z] 13:12:10 INFO - GECKO(4651) | ExceptionHandler::GenerateDump cloned child 5104
[task 2020-01-14T13:12:10.431Z] 13:12:10 INFO - GECKO(4651) | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2020-01-14T13:12:10.432Z] 13:12:10 INFO - GECKO(4651) | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2020-01-14T13:12:10.606Z] 13:12:10 INFO - GECKO(4651) | Exiting due to channel error.
Upstream PR was closed without merging
Assignee | ||
Comment 48•4 years ago
|
||
I fixed this issue and another race condition I encountered and am now waiting for re-review.
Comment 49•4 years ago
|
||
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/38516e564ad9 Change transaction in different scope to a readonly transaction. r=dom-workers-and-storage-reviewers,asuth,janv https://hg.mozilla.org/integration/autoland/rev/adb6cb2041b0 Fix state model, return to Inactive state after creation when returning to event loop. r=dom-workers-and-storage-reviewers,asuth https://hg.mozilla.org/integration/autoland/rev/970b98bbd003 Implemented IDBTransaction.commit. r=dom-workers-and-storage-reviewers,ytausky https://hg.mozilla.org/integration/autoland/rev/30ffe7479d06 Replace CanAcceptRequests by the now equivalent IsActive. r=dom-workers-and-storage-reviewers,janv
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment 51•4 years ago
|
||
bugherder |
Upstream PR merged by moz-wptsync-bot
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 53•4 years ago
|
||
OK, 2nd try at getting the docs done, now this has properly landed; see https://github.com/mdn/sprints/issues/2749#issuecomment-585351292 for the particulars.
Description
•