Open Bug 1275496 Opened 4 years ago Updated 4 months ago

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

Categories

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

defect

Tracking

()

People

(Reporter: bevis, Assigned: sg)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(3 files)

No description provided.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
See Also: → 785884
Whiteboard: btpp-active
set wrong status unexpectedly.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → ASSIGNED
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.
Depends on: 785884
Flags: needinfo?(jvarga)
(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.
Blocks: 785884
No longer depends on: 785884
Duplicate of this bug: 1253634
Depends on: 1276576
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()
Flags: needinfo?(jvarga)
(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.
Depends on: 1286717
Assignee: btseng → nobody
Status: ASSIGNED → NEW
Priority: -- → P3
Assignee: nobody → sgiesecke

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.

Depends on: 1566758

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.

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

Flags: needinfo?(bugmail)

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

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.

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

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

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