1287440 - Is "typeof indexedDB" allowed to throw a SecurityError?

Status: NEW

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

Description

[Jukka Jylänki]

Hey, double-checking the following behavior that I found surprising:

STR:

1. Disable cookies via the instructions from https://support.mozilla.org/en-US/kb/enable-and-disable-cookies-website-preferences, i.e.
set Firefox will: "Use custom settings for history", and uncheck "Accept cookies from sites"

2. Close preferences and restart the browser (for good measure). Open a new tab and the web page console in it, and type

> typeof indexedDB

Observed:

The expression will throw "SecurityError: The operation is insecure."

Does this sound correct and expected behavior? The two things I found surprising here is that a) the cookies setting affects how IndexedDB behaves, and b) I would not have expected the typeof operator to throw, but either return "object" or "undefined". All good if this is as intended, though just wanted to confirm.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

IDB access is dependent on cookie settings because it can be used for tracking, just like cookies.

I'm guessing we should probably be returning NS_ERROR_DOM_NOT_SUPPORTED_ERR from here instead of a SecurityError:

https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBFactory.cpp#343

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make window.indexedDB return null instead of throwing when cookies are disabled. r=khuey

IDBFactory::CreateForWindow() specifically tries to return null instead of throwing if NS_ERROR_DOM_NOT_SUPPORTED_ERR is returned from AllowedForWindowInternal:

https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBFactory.cpp#139

But AllowedForWindowInternal() return NS_ERROR_DOM_SECURITY in this situation:

https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBFactory.cpp#343

It seems like it might be nicer to use _NOT_SUPPORTED_ERR here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d877451f4d33

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d877451f4d33

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I'll need to update test expectations in test_storagePermissionsAccept.html as well.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update test to expect .indexedDB to be null when storage is not available. r=khuey

Update tests to match new behavior.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5686213a6b74

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8772517 [details] [diff] [review]
P2 Update test to expect .indexedDB to be null when storage is not available. r=khuey

There are more tests to fix.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Actually, I'm less sure about this fix.  All the other storage types allow you to access the storage factory object, but then throw/reject when you try to use it.  Should we just conform to that behavior instead of continuing to differ?

Personally I think hiding or returning null from the property makes feature detection a bit nicer.  But if we are going to do that we should consider it for all the storage types.

If we think that is the right path, though, I'm not sure I have the time to fix all the storage APIs.

Kyle, what do you think we should do?

Comment 7

[Jukka Jylänki]

Thanks for grabbing this item on the board so quickly!

I'm ok with either way, as long as we know what is the good and proper way to feature test if IndexedDB is available in a browser. We develop a mini-SDK for Mozilla games partners to use as a reference for telemetry dashboarding browser features, and one detected bit there is the availability of IndexedDB. The current code looks like this: https://github.com/Mozilla-Games/browser_feature_test/blob/master/featuretest.js#L145. 

Whichever behavior will be converged on here, please let me know what would be the proper snippet to test IndexedDB availability with, and if I should change the above code line at some point.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

localStorage also returns SECURITY_ERR.  Whatever we do it seems like it should be consistent.  Is there a compelling reason to change from SECURITY_ERR?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

I think its ok to leave for now.  In the future we maybe we can revisit hiding these when storage is not available.  I'm going to leave this open for now.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Set P3 per comment 9. Please feel free to modify the priority, thanks!