Bug 56703 – window.localStorage should return undefined if LocalStorage is 100% disabled (right now it returns null)

• Component: DOM
• Status: UNCONFIRMED

[Jeff Johnson]

According to http://dev.w3.org/html5/webstorage/#dom-localstorage

    When the localStorage attribute is accessed, the user agent must run the following steps:
    1. The user agent may throw a SECURITY_ERR exception instead of returning a Storage object if the request violates a policy decision (e.g. if the user agent is configured to not allow the page to persist data).
    2. If the Document's origin is not a scheme/host/port tuple, then throw a SECURITY_ERR exception and abort these steps.
    3. Check to see if the user agent has allocated a local storage area for the origin of the Document of the Window object on which the attribute was accessed. If it has not, create a new storage area for that origin.
    4. Return the Storage object associated with that origin's local storage area. Each Document object must have a separate object for its Window's localStorage attribute.

Thus, if the user agent disabled localStorage and does not return a localStorage object, it is expected to throw a SECURITY_ERR.

However, DOMWindow::localStorage(ExceptionCode& ec) from Source/WebCore/page/DomWindow.cpp contains this code:

    if (!document->securityOrigin()->canAccessLocalStorage()) {
        ec = SECURITY_ERR;
        return 0;
    }

    Page* page = document->page();
    if (!page)
        return 0;

    if (!page->settings()->localStorageEnabled())
        return 0;

When local storage is not enabled, it does not raise SECURITY_ERR, it just returns null.

[Antaryami Pandia]

Created attachment 108628 [details]
Proposed Patch

[Antaryami Pandia]

Hi Jeremy,
Please review the patch.

[Jeremy Orlow]

Seems reasonable, but you should write a test.

[Antaryami Pandia]

Created attachment 113162 [details]
Updated patch

Patch with test.

[Antaryami Pandia]

Hi Jeremy,Please review the patch.

[Hajime Morrita]

Comment on attachment 113162 [details]
Updated patch

Code itself looks nice.
Please use text-based test. you can search dumpAsText() in the existing test cases to see how it works.

[Antaryami Pandia]

Created attachment 117595 [details]
Updated patch

Patch with review comments.

[Darin Adler]

Comment on attachment 117595 [details]
Updated patch

The current behavior of not throwing is intentional. What’s the rationale for the change?

[Darin Adler]

Comment on attachment 117595 [details]
Updated patch

I see, it’s a “match the specification” change. I suspect this will cause problems with some websites. Not sure what to do about that.

[Brady Eidson]

(In reply to comment #9)
> (From update of attachment 117595 [details])
> I see, it’s a “match the specification” change. I suspect this will cause problems with some websites. Not sure what to do about that.

I do not agree that this is a "match the specification" change.

[Maciej Stachowiak]

I think it's likely that this change will cause sites that use local storage to be broken in private browsing mode. The intent is probably to let sites gracefully handle the case where they are not allowed to store anything, but in practice many will ignore this case. This will make private browsing a less effective feature.

[Brady Eidson]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 117595 [details] [details])
> > I see, it’s a “match the specification” change. I suspect this will cause problems with some websites. Not sure what to do about that.
> 
> I do not agree that this is a "match the specification" change.

To elaborate:

I agree that the spec suggests that if a user agent supports LocalStorage but LocalStorage is currently disabled, a security exception should be thrown.

WebKit is not a "web browser" engine but rather a "web content rendering* engine.  LocalStorage is something that a user agent can support but many don't want to support it.

In Safari, for example, the LocalStorage object is always there because Safari nominally supports it.  Even when accessing it might throw an exception such as when private browsing is enabled, or might fail for other reasons such as when the quota has been lowered below the current usage.

But in another WebKit application they might never want LocalStorage to work.  They are a user agent that renders web content but not with LocalStorage support.  LocalStorage isn't just disabled;  It is meant to not exist at all within that user agent.

There's many WebKit apps besides browsers.  Many of those have never had LocalStorage support and don't want it.  This change will break them by causing them to start throwing exceptions where they didn't in the past.

I think some confusion might be over the naming of "localStorageEnabled()" as a setting.  That suggests this represents some checkbox that a browser user could flip.  We could rename the setting in code to "localStorageSupported()" to alleviate this confusion in the future.

If we want to support this edge case of the spec in WebKit I think we should add a new setting that is explicitly about "LocalStorage is supported in this user agent, but it's use is currently disallowed."

===

All of that said...  this is clearly a "may" clause in the spec, not a "has to" clause.  Is there any reason we think this is important?

[Brady Eidson]

(In reply to comment #12)
> (In reply to comment #10)
> I agree that the spec suggests that if a user agent supports LocalStorage but LocalStorage is currently disabled, a security exception should be thrown.
...
> All of that said...  this is clearly a "may" clause in the spec, not a "has to" clause.  Is there any reason we think this is important?

At the top of my comment, I should have said "spec suggests that if a user agent supports LocalStorage but LocalStorage is currently disabled, a security exception *COULD* be thrown."

Setting aside my arguments that WebKit is more than just about browsers and Maciejs objections regarding private browsing...  I don't see why this change is important.

[Jeff Johnson]

As background, if that helps, I discovered the difference with the spec while working on this bug:

https://bugs.webkit.org/show_bug.cgi?id=56354 Web Inspector: empty, non-functional window

Although it's true that there's a "may" in step 1, there's a "must" prefacing the steps, and if you go through all of the steps, it seems as far as I can tell that the only options are to throw an exception or return a local storage object. Returning null doesn't seem to be a valid option according to the spec.

I should note that in my experience of running Safari with local storage disabled, which I continue to do to this day, the choice is usually not between throwing an exception or having a web site work correctly. The web sites just assume they can use local storage and don't even check for null, so the choice is between throwing an exception or getting a javascript error.

[Brady Eidson]

(In reply to comment #14)
> As background, if that helps, I discovered the difference with the spec while working on this bug:
> 
> https://bugs.webkit.org/show_bug.cgi?id=56354 Web Inspector: empty, non-functional window
> 
> Although it's true that there's a "may" in step 1, there's a "must" prefacing the steps, and if you go through all of the steps, it seems as far as I can tell that the only options are to throw an exception or return a local storage object. 

I think you're referring to "User agents must have a set of local storage areas, one for each origin" and yes - to be a *full* HTML5 compliant user agent it would have to have the local storage areas.

But WebKit is not *only* a 100% HTML5 engine.  It supports plenty of modes where various HTML5 features are missing entirely.  This is one of those modes we need to continue supporting.

>Returning null doesn't seem to be a valid option according to the spec.

This made me realize that yes, returning null for a missing feature is incorrect behavior.  We should be returning undefined here, which is the appropriate behavior for any user agent and any missing feature.

> I should note that in my experience of running Safari with local storage disabled, which I continue to do to this day, the choice is usually not between throwing an exception or having a web site work correctly. The web sites just assume they can use local storage and don't even check for null, so the choice is between throwing an exception or getting a javascript error.

How is it you run Safari with LocalStorage disabled?  AFAIK, Safari doesn't support doing this.  Twiddling the hidden WebKit default yourself gets you in to unsupported power-user land.

And this may be true for web sites where you notice it, but I'm not suggesting there *might be* web content that properly and wisely do feature testing and change their behavior to match.  I'm saying there *is* such content that this change would break.

[Ian 'Hixie' Hickson]

Indeed. Note in particular this paragraph from the conformance subsection of the HTML standard:

"When support for a feature is disabled (e.g. as an emergency measure to mitigate a security problem, or to aid in development, or for performance reasons), user agents must act as if they had no support for the feature whatsoever, and as if the feature was not mentioned in this specification. For example, if a particular feature is accessed via an attribute in a Web IDL interface, the attribute itself would be omitted from the objects that implement that interface — leaving the attribute on the object but making it return null or throw an exception is insufficient."

[Brady Eidson]

(In reply to comment #16)
> Indeed. Note in particular this paragraph from the conformance subsection of the HTML standard:
> 
> "When support for a feature is disabled (e.g. as an emergency measure to mitigate a security problem, or to aid in development, or for performance reasons), user agents must act as if they had no support for the feature whatsoever, and as if the feature was not mentioned in this specification. For example, if a particular feature is accessed via an attribute in a Web IDL interface, the attribute itself would be omitted from the objects that implement that interface — leaving the attribute on the object but making it return null or throw an exception is insufficient."

Thanks Hixie!

Retitling this bug to reflect the real problem which is that we return null instead of undefined in this case.

[Brady Eidson]

Comment on attachment 117595 [details]
Updated patch

We've confirmed this is not the behavior the spec calls for and that it's not the behavior we want.

[Jeff Johnson]

(In reply to comment #15)
> >Returning null doesn't seem to be a valid option according to the spec.
> 
> This made me realize that yes, returning null for a missing feature is incorrect behavior.  We should be returning undefined here, which is the appropriate behavior for any user agent and any missing feature.

In light of Hixie's comment, that sounds fine to me.

> How is it you run Safari with LocalStorage disabled?  AFAIK, Safari doesn't support doing this.  Twiddling the hidden WebKit default yourself gets you in to unsupported power-user land.

I'm not looking for support. I just noticed a bug and filed it. :-)

[Ryosuke Niwa]

Comment on attachment 117595 [details]
Updated patch

cq- given r-.