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.
(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.
Yeah, doing both seems reasonable.
(In reply to comment #2) > Yeah, doing both seems reasonable. OK then, taking over.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
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?
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.
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).
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
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.
You need to log in before you can comment on or make changes to this bug.