1612289 - IDBTransaction.commit() needs to be unexposed on Firefox 73 because it feature detects as present but is not implemented

Status: VERIFIED FIXED

(Core :: Storage: IndexedDB, defect, P1)

Description

[Andrew Sutherland [:asuth] (he/him)]

https://phabricator.services.mozilla.com/D46277 landed a stub implementation that always returns errors as part of Firefox 73 in bug 1497007 without being conditionally guarded by a preference. This method is expected to be feature-detectable, with sites assuming that if (transaction.commit) is true, that the method will be implemented. We need to unexpose this method on beta by removing it from the WebIDL or adding an exposure guard annotation like Pref or Func.

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1612289 - Remove IDBTransaction.commit from webidl, since its implementation is only a stub. r=#dom-workers-and-storage

Disable tests for IDBTransaction.commit to avoid intermittent timeouts.

Comment 2

[Simon Giesecke [:sg] [he/him]]

Comment on attachment 9123672 [details]
Bug 1612289 - Remove IDBTransaction.commit from webidl, since its implementation is only a stub. r=#dom-workers-and-storage

Beta/Release Uplift Approval Request

• User impact if declined: Feature test for 'IDBTransaction.commit' will be a false-positive, as the method is only implemented as a stub.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Create a IndexedDB database and object store

2. Open a transaction for the object store
3. Check that transaction.commit is undefined

This could be tested automatically, but since it will be actually implemented in the next release, so it doesn't appear worthwhile to add an automated test.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch restores the previous behaviour.

Alternative might be to uplift the real implementation of IDBTransaction.commit, but this would be more risky.

• String changes made/needed:

Comment 3

[Julien Cristau [:jcristau]]

Comment on attachment 9123672 [details]
Bug 1612289 - Remove IDBTransaction.commit from webidl, since its implementation is only a stub. r=#dom-workers-and-storage

approved for 73.0b12

Comment 4

[Julien Cristau [:jcristau]]

Comment on attachment 9123672 [details]
Bug 1612289 - Remove IDBTransaction.commit from webidl, since its implementation is only a stub. r=#dom-workers-and-storage

woops, needs review first.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Julien Cristau [:jcristau] from comment #4)

woops, needs review first.

Reviews acquired!

Comment 6

[Julien Cristau [:jcristau]]

Comment on attachment 9123672 [details]
Bug 1612289 - Remove IDBTransaction.commit from webidl, since its implementation is only a stub. r=#dom-workers-and-storage

that was fast :)

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/14ac73797dd8

Comment 8

[Daniel Bodea [:danibodea]]

Simon, I am attempting to confirm this fix, but the steps to reproduce written in the uplift request in comment 2 is beyond the level of my understanding. Can you somehow provide a detailed list of steps that I can use to verify this fix? Thank you.

Comment 9

[Simon Giesecke [:sg] [he/him]]

(In reply to Bodea Daniel [:danibodea] from comment #8)

Simon, I am attempting to confirm this fix, but the steps to reproduce written in the uplift request in comment 2 is beyond the level of my understanding. Can you somehow provide a detailed list of steps that I can use to verify this fix? Thank you.

I am not an expert in doing this from the Console either, but the following should do the job in the Web Console (Ctrl-Shift-K):

var openDB = indexedDB.open("MyDatabase", 1);

  openDB.onupgradeneeded = function() {
    var db = {}
    db.result = openDB.result;
    db.store = db.result.createObjectStore("MyObjectStore", {keyPath: "id"});
  };

then

var tx = openDB.result.transaction("MyObjectStore", "readwrite");

and then

tx.commit

should show undefined now. Before this patch, it would have shown function commit().

Comment 10

[Daniel Bodea [:danibodea]]

I have reproduced this issue in Beta11 v73.0b11, then I have verified the fix in Firefox Beta (Release Candidate) v73.0 and I have also tested the Release v72.0.2 which it is unaffected AND the Nightly v74.0a1 which is AFFECTED. This fix was verified in Windows 10, Mac OS 10.14.6 and Ubuntu 18.04.

Comment 11

[Daniel Bodea [:danibodea]]

[Tracking Requested - why for this release]:
It appears that this bug still occurs in nightly, was fixed on beta and release is unaffected. Do we need to track this in firefox74 as well?

Comment 12

[Julien Cristau [:jcristau]]

AIUI this is not relevant for 74 because that has a real (non-stub) implementation.