Closed Bug 1497007 Opened 6 years ago Closed 5 years ago

Implement IDBTransaction commit() method

Categories

(Core :: Storage: IndexedDB, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla74
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.
Blocks: IDBv3
Assignee: nobody → sgiesecke

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?

Flags: needinfo?(bugmail)

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.

Flags: needinfo?(bugmail)

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.

Status: NEW → ASSIGNED

(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.

What exactly means intent-to-implement here? Where do I send that, is there some template for that?

Flags: needinfo?(bugmail)

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.

Flags: needinfo?(bugmail)

Depends on D46946

Depends on D47230

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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

(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.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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.

Flags: needinfo?(sgiesecke)
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

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.

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
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
Flags: needinfo?(sgiesecke)
Keywords: leave-open

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!

(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.

Flags: needinfo?(cmills)
Target Milestone: mozilla71 → Future

(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

Flags: needinfo?(cmills)

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.

Attachment #9093555 - Attachment description: Bug 1497007 - Added IDBTransaction.commit method, with a minimal implementation. r=ttung → Bug 1497007 - Added IDBTransaction.commit method, with a minimal implementation. r=#dom-workers-and-storage

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.

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.

Backed out for assertion failures on IDBTransaction.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/41ae9684c66e50558e5cd6d2f0397bd9ab2b09eb

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64%2Fdebug-mochitest-browser-chrome-fis-e10s-14%2Cm-fis%28bc14%29&tochange=5c7486e74abd3310613094dc86cdf6ea2253ade0&fromchange=b102c4cb93aff35e931c4fa4555fbc5157847812&selectedJob=284840146

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.

Flags: needinfo?(sgiesecke)
Upstream PR was closed without merging

I fixed this issue and another race condition I encountered and am now waiting for re-review.

Flags: needinfo?(sgiesecke)
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.
Upstream PR merged by moz-wptsync-bot
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: Future → mozilla74
Regressed by: 1609957
Regressions: 1612289
No longer regressed by: 1609957
Regressions: 1609957

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.

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

Attachment

General

Created:
Updated:
Size: