458498 - nsIDOMStorage::GetLength shouldn't throw when DOM storage is disabled

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Simon Bünzli]

The following throws at |storage.length| when DOM storage is disabled:

let storage = DOCSHELL.getSessionStorageForURI(URI);
if (!storage || storage.length == 0)
  continue;

This seems wrong from an API point of view. IMO either getSessionStorageForURI should return null when storage isn't available for a given URI, or there should be a way of determining if storage is available in the first place (without try-catching). nsIDOMStorage::GetLength returning 0 would be fine by me as well.

Comment 1

[(no longer active)]

(In reply to comment #0)
> IMO either getSessionStorageForURI
> should return null when storage isn't available for a given URI, or there
> should be a way of determining if storage is available in the first place
> (without try-catching).

I think both of these should be done.  The former would prevent starting to use DOM storage when it's not available for any reason, and the latter would help prevent using it when it's become unavailable *after* getSessionStorageForURI has been called.

What do you think, Dave?

Once we come up with a solution here, I'd be happy to implement it.

Comment 2

[Dave Camp (:dcamp)]

Yeah, doing both seems reasonable.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Yeah, doing both seems reasonable.

OK then, taking over.

Comment 4

[(no longer active)]

We need to expose this to scripts, so adding this to nsPIDOMStorage is not an option.  Also, nsIDOMStorage is a WHATWG API, and I'm not sure if it's OK to modify that interface or not.  If it's not OK, I can probably add a new interface named like nsIDOMStorageStatus.  Dave: what do you think?

Comment 5

[(no longer active)]

Based on IRC comments from Dave, I'm WONTFIX-ing this.  I'll file another bug in order to add the required exception handling to the code landed in bug 339445, and that bug would block my work on Private Browsing, instead of this one.

Comment 6

[Simon Bünzli]

Please cite that IRC conversation which led to this WONTFIX. I'm really no fan of having to try-catch non-exceptions (i.e. statements which could throw frequently without that throwing being reasonably avoidable).

Comment 7

[(no longer active)]

Sure:

	<campd>	ok, thinking..
	so my gut reaction is that testing up front whether you have access
	and then not catching access errors later
	is generally a fragile thing to do
	<ehsan>	yes, I agree
	<campd>	so my gut feeling is that you need exception handlers anyway
	but I don't disagree that it'd be nice to not Need them in a known common case.
	try out-negativing THAT sentence.
	I'll rephrase: "but yeah, it'd be nice to not rely on that for a common case"
	<ehsan>	like, for example, the usage of .length?
	<campd>	so, do we clear session storage upon entering private mode?
	<ehsan>	no, the current implementation just disables it
	<campd>	ok
	so has this code always been broken if dom storage is turned off?
	<ehsan>	yes
	<campd>	ok
	so I'd suggest one of two things:
	a) Just deal with exceptions
	or b) you could add CanAccessStorage(nsIPrincipal, nsIDOMStorage) to nsIDOMStorageManager
	there are a few things that'd be cleaner than b)
	but I don't know about adding a whole new interface for that :/
	https://bugzilla.mozilla.org/show_bug.cgi?id=457985
	that bug is related, fwiw
	(ie, access checks are going to become a bit more complicated)
	<ehsan>	hmmm
	<campd>	I think I'd lean toward a)
	though, I'
	though, I'm curious;
	<ehsan>	adding CanAccessStorage doesn't prevent the case that our access is changed after we obtained the storage initially
	<campd>	does session store not know about private pbrowsing mode?
	no, it doesn't
	a) is sort of a prerequisite no matter what.
	you really should be catching and handling exceptions.
	I guess I'm leaving whether to add a preemptive check as a decision for the reader
	personally, I'd just handle exceptions and be done with it.
[Private browsing specific stuff snipped]
	<campd>	I don't think it's a problem to allow access to .length
	when storage is disabled
	but I don't think we could make it 0 just because access is disallowed
	it'd have to be whatever the actual length of it is
	I guess, more generally:
	For what CanUseStorage currently is
	an "is the storage feature available for this domain"
	I think allowing read-only access to it is ok.
	(but I'm not sure of that)
	for an actual security check, read-only access is clearly NOT ok.
	<ehsan>	that should kind of fall out of the scope for bug 458498, doesn't it?
	<campd>	yeah.
	so I think we can WONTFIX that one.
	it's closer to the scope of 457985

Comment 8

[Simon Bünzli]

Alright, thanks.

Comment 9

[Dave Camp (:dcamp)]

Part of that irc conversation that was snipped, is that I actually think this SHOULD be an exceptional case.

The situation I described was:

a) A page puts data in sessionStorage
b) I put the browser in private browsing mode
c) I close the page
d) I reopen the page using History->Recently Closed Tabs
e) I take the page out of private browsing mode
f) sessionStorage is now empty, when it wouldn't have been had I left the page open.

So, I actually think it might be a bug that chrome isn't allowed access to session storage while in private browsing mode.  I think not having access should actually be an exceptional condition, and we shouldn't have code to specifically do the wrong thing.