599479 - Accessing window.localStorage with dom.storage.enabled=false causes NS_ERROR_DOM_SECURITY_ERR

Status: RESOLVED FIXED

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

Description

[Cacycle]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729)

In JavaScript, accessing window.localStorage when dom.storage.enabled is set to false in about:config causes an NS_ERROR_DOM_SECURITY_ERR and terminates the script.

This happens even for typeof(window.localStorage). This breaks web pages and web applications for users that have changed their browser settings. The behaviour is absolutely unexpected and unneccessary. There is no intuitive and/or simple way for script authors to test the availability of local storage (= web storage, = offline storage). There seem to be MANY users with this browser setting around, perhaps due to the web storage scare a while ago.

Reproducible: Always

Steps to Reproduce:
1. Open about:config, toggle dom.storage.enabled
2. Load the attaches testcase html page containing an "typeof(window.localStorage)"
3. Check the error console
Actual Results:  
Error console shows the following error:

Error: uncaught exception: [Exception... "Security error" code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"

Expected Results:  
No error should be thrown, perhaps the window.localStorage object should be undefined so that script authors can simply test for the availability of local storage.

There is a workaround for desperate script authors: 

setTimeout('storage = typeof(window.localStorage);', 0);

Comment 1

[Cacycle]

Attachment: Testpage

Comment 2

[Honza Bambas (:mayhemer)]

--> me

Seems simple to fix.

Comment 3

[Johnny Stenback (:jst)]

Yup, sounds simple to fix, but I wouldn't block on it. If I see an approval request for a safe patch I'll likely approve it though :)

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 478636 [details] [diff] [review]
v1

+  if (!nsContentUtils::GetBoolPref(kStorageEnabled)) {
+    return NS_OK;

All places that do this need to make sure that the out param (*aSessionStorage) is set to nsnull.

r+a=jst with that.

Comment 6

[Honza Bambas (:mayhemer)]

Is this [1] then a bug?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7660

Comment 7

[Johnny Stenback (:jst)]

Yes. Please fix that as well.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [Check in comment 9] [1.9.2 comment 22] [1.9.1 comment 23]

Approved for 2.0 already.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 481249 [details] [diff] [review]
v1.1 [Check in comment 9] [1.9.2 comment 22] [1.9.1 comment 23]

http://hg.mozilla.org/mozilla-central/rev/db10eec8f8ab

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This bug is the same as bug 527970, right?  

If so, is this fix branch-safe?

It seems[1] like this "throwing NS_ERROR_FOO" behavior has been causing lots of problems for users with DOM storage off (or who have cookies set to "Ask", bug 365772/bug 517778), and an increasing number of sites are starting to use DOM storage without catching exceptions (bug 527970 comment 4, bug 527970 comment 5, bug 606830).

If this fix branch-safe, it would make a lot of the pain go away for both users and authors, since they'd get consistent behavior against development and supported release versions of Gecko, and authors wouldn't have to remember to try/catch for 1.9.x (which they weren't remembering to do, anyway).

[1] (well, more than "seems" to those of us who hit it; bug 365772 comment 12)

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

See comment 10 for my rationale for making this request--but obviously this is moot if the patch isn't branch-safe (which no one has yet answered).

Comment 12

[Honza Bambas (:mayhemer)]

The patch doesn't modify API, doesn't add new strings, has test, and is quit simple, only changes behavior of DOM storage getters of the window object.  From my point of view it is branch safe.

Rather request approval on the patch then block the bug.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 481249 [details] [diff] [review]
v1.1 [Check in comment 9] [1.9.2 comment 22] [1.9.1 comment 23]

For compatibility reasons it would be handy to have this also on branches.  Read the description for details of what the patch fixes.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #12)
> Rather request approval on the patch then block the bug.

Thanks, Honza.  I don't like to request approval on other people's patches randomly, certainly without knowing all the answers ;) so doing the branch flags on the bug seemed like a sane compromise.

Comment 16

[al_9x]

needs to be backported as it breaks sites like youtube, google images

Comment 17

[Daniel Veditz [:dveditz]]

(In reply to comment #12)
> The patch doesn't modify API, doesn't add new strings, has test, [...]

But it _does_ change the behavior, the "API" if you will, of how a page has to use this object. It's much easier to explain "Firefox 3.6 works this way, Firefox 4 works that way" than to have developers understand some Firefox 3.6 users get one behavior and others get the new behavior. Either way web sites have to write try{}catch{} blocks or significant numbers of people break.

If you can get folks like Blizzard, Johnath, and Beltzner to sign off on this kind of mid-version change we can take it.

Comment 18

[al_9x]

(In reply to comment #17)
> (In reply to comment #12)
> > The patch doesn't modify API, doesn't add new strings, has test, [...]
> 
> But it _does_ change the behavior, the "API" if you will, of how a page has to
> use this object.

All sites that use dom storage already handle the null case (have never encountered one that was broken by null), it's the exception that many don't handle.  So this change will not break anything but only fix broken sites.

Comment 19

[Daniel Veditz [:dveditz]]

> So this change will not break anything but only fix broken sites.

That seems likely, but it's a web-compatibility change and needs higher-order approval from the product/project leads. I'm just the security guy.

Comment 20

[LegNeato]

Comment on attachment 481249 [details] [diff] [review]
v1.1 [Check in comment 9] [1.9.2 comment 22] [1.9.1 comment 23]

a=LegNeato for 1.9.2.13 and 1.9.1.16

Comment 21

[al_9x]

I saw that the 3.6.13 code freeze is tomorrow, has this been approved?

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 481249 [details] [diff] [review]
v1.1 [Check in comment 9] [1.9.2 comment 22] [1.9.1 comment 23]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ba187e04e20e

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 481249 [details] [diff] [review]
v1.1 [Check in comment 9] [1.9.2 comment 22] [1.9.1 comment 23]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8d9e038d30bf