Last Comment Bug 599479 - Accessing window.localStorage with dom.storage.enabled=false causes NS_ERROR_DOM_SECURITY_ERR
: Accessing window.localStorage with dom.storage.enabled=false causes NS_ERROR_...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Windows XP
: -- major (vote)
: ---
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
: 527970 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-24 13:23 PDT by Cacycle
Modified: 2010-12-06 12:49 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
.13-fixed
.16-fixed


Attachments
Testpage (267 bytes, text/html)
2010-09-24 13:24 PDT, Cacycle
no flags Details
v1 (5.37 KB, patch)
2010-09-26 05:28 PDT, Honza Bambas (:mayhemer)
jst: review+
jst: approval2.0+
Details | Diff | Splinter Review
v1.1 [Check in comment 9] [1.9.2 comment 22] [1.9.1 comment 23] (5.49 KB, patch)
2010-10-06 09:47 PDT, Honza Bambas (:mayhemer)
honzab.moz: review+
christian: approval1.9.2.13+
christian: approval1.9.1.16+
Details | Diff | Splinter Review

Description Cacycle 2010-09-24 13:23:06 PDT
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 2010-09-24 13:24:02 PDT
Created attachment 478380 [details]
Testpage
Comment 2 Honza Bambas (:mayhemer) 2010-09-24 14:37:04 PDT
--> me

Seems simple to fix.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2010-09-24 15:14:22 PDT
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) 2010-09-26 05:28:49 PDT
Created attachment 478636 [details] [diff] [review]
v1
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2010-09-27 13:02:18 PDT
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) 2010-09-27 13:06:20 PDT
Is this [1] then a bug?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7660
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2010-09-27 13:49:04 PDT
Yes. Please fix that as well.
Comment 8 Honza Bambas (:mayhemer) 2010-10-06 09:47:27 PDT
Created attachment 481249 [details] [diff] [review]
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) 2010-10-07 08:05:51 PDT
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) 2010-11-03 23:02:54 PDT
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) 2010-11-09 11:32:31 PST
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) 2010-11-09 11:48:30 PST
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) 2010-11-09 11:49:51 PST
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) 2010-11-10 11:58:09 PST
(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 15 Josh Matthews [:jdm] (away until 9/3) 2010-11-11 09:33:03 PST
*** Bug 527970 has been marked as a duplicate of this bug. ***
Comment 16 al_9x 2010-11-11 09:56:52 PST
needs to be backported as it breaks sites like youtube, google images
Comment 17 Daniel Veditz [:dveditz] 2010-11-12 10:36:12 PST
(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 2010-11-12 11:12:51 PST
(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] 2010-11-12 12:34:02 PST
> 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 christian 2010-11-15 13:24:55 PST
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 2010-11-17 17:12:11 PST
I saw that the 3.6.13 code freeze is tomorrow, has this been approved?
Comment 22 Honza Bambas (:mayhemer) 2010-11-18 12:25:13 PST
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) 2010-11-18 13:04:39 PST
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

Note You need to log in before you can comment on or make changes to this bug.