1275496 - Fix failure of idbfactory_open9.htm in web-platform test.

Status: RESOLVED FIXED

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

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

set wrong status unexpectedly.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

After further investigation, this failure is related to the change in bug 785884, because I can get positive result if I roll back the interface change of the open methods in IDBFactory.webidl.

The reason is that:
1. In idbfactory_open9.htm, there are several tests to verify the behaviour of the IDBFactory.open(name, *optional* version), and we failed at the following given versions:
false, 
{
  toString: function() { 
    assert_unreached("toString should not be called for ToPrimitive [Number]"); },
  }
  valueOf: function() { return 0; }
},
{
  toString: function() { return 0; },
  valueOf: function() { return {}; }
},
{
  toString: function() { return {}; },
  valueOf: function() { return {}; },
},

2. However, in bug 785884, we defined a new overloaded *open* method(Gecko-specific) as IDBFactory.open(name, IDBOpenDBOptions) and removed the *optional* attribute of the version in orignal open() to prevent the ambiguity of the these 2 methods.

Hence, all open invocation with the non-numeric versions in the test case will be directed to the open method with IDBOpenOptions as input.

Maybe we have to redefine this open method without overloading to the one defined in the IDB spec.

NI Jan to see if there is any better suggestion to fix this problem.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #2)
> 1. In idbfactory_open9.htm, there are several tests to verify the behaviour
> of the IDBFactory.open(name, *optional* version), and we failed at the
> following given versions:
TypeError is expected to be thrown but was not.
> false, 
it's *null* instead of false.

Comment 5

[Jan Varga [:janv]]

I think we have to live with this for now. The only way how to get persistent storage for IndexedDB is to pass { storage: "persistent" } as the second argument to open().

We will be able to remove the special argument once we have fully working implementation of navigator.storage.persist()

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #5)
> I think we have to live with this for now. The only way how to get
> persistent storage for IndexedDB is to pass { storage: "persistent" } as the
> second argument to open().
> 
> We will be able to remove the special argument once we have fully working
> implementation of navigator.storage.persist()

Thanks for clarifying this.
Add dependency on bug 1286717 per comment 5.

Comment 7

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

Now that open(name, options) is no longer exposed publicly, we can resolve the clashes with the portable open(name, optional version) overload. I will rename open(name, options) to openWithOptions(name, options) and adapt all internal uses.

Comment 8

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

Actually, open(name, options) is still available publicly if enabled via pref dom.indexedDB.storageOption.enabled, so we should first re-enable and evaluate the telemetry for the use of this overload before resolving this issue.

Comment 9

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

Attachment: Bug 1275496 - Remove custom overload of Open with options. r=#dom-storage

Comment 10

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

Attachment: Bug 1275496 - Re-enable previously failing idbfactory_open9 tests. r=#dom-storage

Depends on D38293

Comment 11

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

Attachment: Bug 1275496 - Change existing uses of custom IDBFactory.open to the standard overload. r=#dom-storage

Depends on D38294

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Simon Giesecke [:sg] from comment #8)

Actually, open(name, options) is still available publicly if enabled via pref dom.indexedDB.storageOption.enabled, so we should first re-enable and evaluate the telemetry for the use of this overload before resolving this issue.

I don't think this is necessary, as the pref is off by default and was added precisely to remove the feature in way that could easily be reverted - and there hasn't been a need to revert it in the past year. I guess asuth can make the call.

Comment 13

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

I understand there to be 2 things going on:

1. Removing support for the storage attribute in the options dictionary. This is tracked by bug 1354500. And it's what the pref "dom.indexedDB.storageOption.enabled" is about. The attribute will be ignored if you're not using the system principal or an addon principal and the preference is not set. However, callers can still use it.
   • We added a deprecation warning in bug 1442560 that shipped in Firefox 60 on May 7th, 2018.
   • We sent an intent to unship "storage" attribute in the options dict: https://groups.google.com/d/msg/mozilla.dev.platform/3b700_oeAzo/4hzZDEJQCwAJ on March 6, 2018.
   • This is the only thing we've been gathering telemetry on.
2. Removing support for the options dictionary. I don't think we're actually pursuing this yet, and it's fairly likely the IDB spec will end up using this signature in the future, so it's not clear it's worth trying to remove it at this time. We could certainly start gathering telemetry on the usage to be able to move forward on removing support for the options dictionary. We would want to make sure to either filter out or bin separately chrome principal and addon-principal usages. Perhaps also distinguish when "file" schemes are using IndexedDB, as we've seen evidence of Firefox being used directly for kiosk-type applications where Firefox's implementation may be assumed. (We may not get telemetry from these, however...)
   • Specifically, it's clear we want to use multiple storage buckets (https://github.com/whatwg/storage/issues/2) but it's not clear that we've decided how to expose them and how they'll interact with existing APIs. The two obvious directions are expanding each API so that IDBFactory.open() could take an options dict of { bucket: "bucket name" } or having navigator.storage.getBucket("bucket name") exist that returns an object with an IDBFactory on it.

Comment 14

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

What I discussed with :tt is actually related to your point 2, and until now I thought that it was the goal to remove support for the options dictionary in the end. It makes sense that the IDB spec will be extended to accept a dictionary on an IDBFactory.open* operation in the future, but I wasn't aware of it yet. However, if the spec is extended in a backwards-compatible way, it will also need to supply this operation under a different name than the existing open(name, version). The existence of two overloads is exactly the reason why the wpt tests under discussion here are currently failing for Firefox.

Comment 15

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

However, if the spec is extended in a backwards-compatible way, it will also need to supply this operation under a different name than the existing open(name, version).

I'm not sure I agree with this. That IDBFactory.open("name", { version: 2 }) is currently expected to throw per spec does not require that it continues to throw going forward. We just can't make IDBFactory.open("name", 2) start throwing.

That said, there are rules about overloading at https://heycam.github.io/webidl/#idl-overloading that people sometimes disobey and shouldn't. (Specifically, all the overloads have to be on the same interface, so specs can't monkeypatch overloads into an interface.)

Comment 16

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #13)

I understand there to be 2 things going on:

1. Removing support for the storage attribute in the options dictionary. This is tracked by bug 1354500. And it's what the pref "dom.indexedDB.storageOption.enabled" is about. The attribute will be ignored if you're not using the system principal or an addon principal and the preference is not set. However, callers can still use it.
   • We added a deprecation warning in bug 1442560 that shipped in Firefox 60 on May 7th, 2018.
   • We sent an intent to unship "storage" attribute in the options dict: https://groups.google.com/d/msg/mozilla.dev.platform/3b700_oeAzo/4hzZDEJQCwAJ on March 6, 2018.
   • This is the only thing we've been gathering telemetry on.
2. Removing support for the options dictionary. I don't think we're actually pursuing this yet, and it's fairly likely the IDB spec will end up using this signature in the future, so it's not clear it's worth trying to remove it at this time. We could certainly start gathering telemetry on the usage to be able to move forward on removing support for the options dictionary. We would want to make sure to either filter out or bin separately chrome principal and addon-principal usages. Perhaps also distinguish when "file" schemes are using IndexedDB, as we've seen evidence of Firefox being used directly for kiosk-type applications where Firefox's implementation may be assumed. (We may not get telemetry from these, however...)
   • Specifically, it's clear we want to use multiple storage buckets (https://github.com/whatwg/storage/issues/2) but it's not clear that we've decided how to expose them and how they'll interact with existing APIs. The two obvious directions are expanding each API so that IDBFactory.open() could take an options dict of { bucket: "bucket name" } or having navigator.storage.getBucket("bucket name") exist that returns an object with an IDBFactory on it.

Thanks for clarifying!

I was talking to Simon that we are going to deprecate storage attribute, but I wasn't aware that the spec might take the dictionary approach for other uses. So that I was thinking maybe we could start doing this. If we can remove storage attribute in the dictionary and make a specific function for persistent indexedDB database for internal callers, then we can maybe start to reduce the checks for PERSISTENCE_TYPE_TEMPORARY in the quota manager (and maybe the GroupInfoPairs object). And, maybe we can let our initialization slightly faster by this.

Comment 17

[Shane Caraveo (:mixedpuppy)]

These patches are all quite old, can they be abandoned?

Comment 18

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)

These patches are all quite old, can they be abandoned?

Hm, I am not sure, why are you asking? I have not continued work on this for quite some time, true, but what would be the benefit of abandoning the patches?

Comment 19

[Shane Caraveo (:mixedpuppy)]

I assumed by unassigned yourself that you were abandoning the bug, so why not also close out the patches. They can still be viewed.

Comment 20

[Jari Jalkanen [:jari]]

Attachment: Bug 1275496 - Deprecate non-standard IDB Factory Open overload. r=#dom-storage

Comment 22

[BugBot [:suhaib / :marco]]

The severity field for this bug is set to S4. However, the following bug duplicate has higher severity:

• Bug 1606210: S3

:jjalkanen, could you consider increasing the severity of this bug to S3?

For more information, please visit BugBot documentation.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4d94d28f45b
Change existing uses of custom IDBFactory.open to the standard overload. r=dom-storage-reviewers,home-newtab-reviewers,janv,thecount
https://hg.mozilla.org/integration/autoland/rev/3a688afd6ea8
Remove custom overload of Open with options. r=webidl,janv,saschanaz,smaug,asuth
https://hg.mozilla.org/integration/autoland/rev/3029b48a3450
Re-enable previously failing idbfactory_open9 tests. r=dom-storage-reviewers,janv

Comment 24

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c4d94d28f45b
https://hg.mozilla.org/mozilla-central/rev/3a688afd6ea8
https://hg.mozilla.org/mozilla-central/rev/3029b48a3450