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.
Created attachment 108628 [details] Proposed Patch
Hi Jeremy, Please review the patch.
Seems reasonable, but you should write a test.
Created attachment 113162 [details] Updated patch Patch with test.
Hi Jeremy,Please review the patch.
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.
Created attachment 117595 [details] Updated patch Patch with review comments.
Comment on attachment 117595 [details] Updated patch The current behavior of not throwing is intentional. What’s the rationale for the change?
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.
(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.
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.
(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?
(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.
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.
(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.
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."
(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.
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.
(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. :-)
Comment on attachment 117595 [details] Updated patch cq- given r-.