1.09 KB, patch
|Details | Diff | Splinter Review|
1.11 KB, patch
|Details | Diff | Splinter Review|
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.
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
Created attachment 8771982 [details] [diff] [review] 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
I'll need to update test expectations in test_storagePermissionsAccept.html as well.
Created attachment 8772517 [details] [diff] [review] 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 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.
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?
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.
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?
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.
Set P3 per comment 9. Please feel free to modify the priority, thanks!