Bug 32115 – document.cookie accessors should raise INVALID_STATE_ERR if there is no browsing context

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: DOM

[Patrik Persson]

(In reply to comment #51 in bug 21288)
> 
> > +    // FIXME: The HTML5 DOM spec states that this attribute can raise an
> > +    // INVALID_STATE_ERR exception on getting if the Document has no
> > +    // browsing context.
> 
> Loose end for after this lands: Test case showing the problem? Bug report for
> the problem?

This was originally located in Document.idl, and moved into
Document.cpp during the work with #21288. I'm posting this to
highlight the issue, but I don't have a strong opinion on how to
resolve it.

http://www.w3.org/TR/html5/dom.html#dom-document-cookie says (slightly
edited):

On getting/setting, if the document is not associated with a browsing
context then the user agent must raise an INVALID_STATE_ERR exception.

I made some quick browser tests using (an instrumented version of)
this code:

    var dom = document.implementation.createDocument('http://www.w3.org/1999/xhtml', 'html', null);

    dom.cookie = 'invalid';
    cookiecopy = dom.cookie;

These tests show:

* Firefox 3.5.5 (Windows) raises no exception, and actually seems to
  allow both setting and getting the cookie attribute.

* Safari 4.0.4 (Mac OS X) raises no exception, but prevents the
  attribute from being modified and/or read. (Reading it back gives an
  empty string.) WebKit r51596 behaves the same way.

* IE8 does not have a document.implementation.createDocument method.

[Patrik Persson]

Created attachment 44231 [details]
Patch to raise exception in document.cookie accessors

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44231 [details] without any errors.

[Patrik Persson]

Created attachment 44239 [details]
Revised more tests.

I completely missed the fact that three existing tests broke with this
change. Sorry about this. This patch includes those tests.

The tests didn't expect exceptions to be raised. Rather, they simply
expected to receive an empty string when reading the cookie attribute
of a document created using document.implementation.create[HTML]Document.

One new test case replaces another case. I placed the new case in
'fast/cookies' to be able to use the JavaScript framework
(shouldThrow, shouldBe) for checks.

Anyway, before we get into the details of testing this, we should
decide whether to raise an exception at all.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44239 [details] without any errors.

[Alexey Proskuryakov]

> * IE8 does not have a document.implementation.createDocument method.

You could test its behavior by loading an iframe, taking a reference to its document, and removing the iframe.

Raising an exception where other browsers don't raise it can cause significant compatibility problems.

[Adam Barth]

You can also get a frameless document from the responseXML property of XMLHttpRequest.

[Darin Adler]

Comment on attachment 44239 [details]
Revised more tests.

The coding of the patch is fine.

Before we make the code change we must have information about the behavior of other browsers. So I am going to say review- on a patch that otherwise would be review+.

[Patrik Persson]

Created attachment 44401 [details]
Code to check browser behavior wrt. cookie access

Absolutely agree that other browsers' behavior is a key issue here. Thanks for the hints on creating documents without browsing contexts.

I accessed the cookie of such a document (by loading an iframe, picking its document, and removing the frame) in Firefox 3.5 and IE8. The test code is attached. I got the following results:

FF3.5
-----
cookie assigned, no exception
cookie read, no exception
cookiecopy=ASSIGNED

IE8
---
cookie assignment exception: Error (Permission denied)
cookie read exception: Error (Permission denied)
cookiecopy=not assigned

To validate the test, I removed the assignment line document.getElementById("frame_holder")... This restores the document's browsing context. I then got the expected results:

FF3.5 & IE8
-----------
cookie assigned, no exception
cookie read, no exception
cookiecopy=ASSIGNED

[Darin Adler]

So the change is going from behavior that matches Firefox 3.5 to behavior that matches IE 8 and the HTML5 specification?

[Patrik Persson]

Created attachment 44688 [details]
Patch with extended test case

Now tests documents created using createDocument(), documents from deleted iframes, and documents from XMLHttpRequest.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44688 [details] without any errors.

[Patrik Persson]

(In reply to comment #9)
> So the change is going from behavior that matches Firefox 3.5 to behavior that
> matches IE 8 and the HTML5 specification?

Almost.  Based on the hints provided earlier in this thread, I have
tested three types of documents: documents created using
createDocument(), documents from deleted iframes, and documents from
XMLHttpRequest.  Here's a summary of the behavior.  (I also extended
the test case to include all three cases.)


IE8:

  * deleted iframe: exception on _any_ access ("permission denied")
  * createDocument: not supported in IE8 (no createDocument method)
  * XMLHttpRequest: exception on setting, getting returns undefined

  (The XMLHttpRequest document in IE8 does not seem to support a
  cookie attribute at all.  The responseXML object supports a
  getElementsByTagName() method, so it seems to really be a document,
  and not the dummy object IE8 returns upon parsing errors.)


FF3.5:

  * deleted iframe: no exception, setting and getting allowed
  * createDocument: no exception, setting and getting allowed
  * XMLHttpRequest: no exception, setting and getting allowed


Safari 4.04/WebKit r51759:

  * deleted iframe: no exception, setting and getting allowed
  * createDocument: no exception, setting silently ignored, getting
                    returns empty string
  * XMLHttpRequest: no exception, setting silently ignored, getting
                    returns empty string


Patch:

  * createDocument: exception
  * XMLHttpRequest: exception
  * deleted iframe: exception


My interpretation of HTML5 is that an exception should be raised in
all these three cases.  Am I reading the spec right?  Is this is how
WebKit should behave?

[Ian 'Hixie' Hickson]

per spec a deleted iframe can still have a browsing context, but that's a bug in webkit (which makes not enough deleted iframes have browsing contexts) and the spec (which makes too many deleted iframes have browsing contexts) so probably an issue for another day.

[Darin Adler]

Comment on attachment 44688 [details]
Patch with extended test case

The mechanics of the patch seem great, and the test cases look good.

If the idea here is changing to match HTML5, then I think we might have a problem because Hixie claims that Safari treats documents as having no browsing context in cases where HTML5 would want it to have a browsing context.

In general, raising an exception in cases where we previously failed in a quieter way incurs a significant risk of breaking some website that accidentally depends on the behavior. Especially in light of the fact that Firefox never raises in an exception in these cases, I'm worried. We should find some way to mitigate this risk.

Is there some kind of testing we can do? Is there some WebKit contributor who has some idea about how to vet this change short of shipping it and waiting for the complaints to come in?

[Maciej Stachowiak]

(In reply to comment #12)
> (In reply to comment #9)
> > So the change is going from behavior that matches Firefox 3.5 to behavior that
> > matches IE 8 and the HTML5 specification?
> 
> Almost.  Based on the hints provided earlier in this thread, I have
> tested three types of documents: documents created using
> createDocument(), documents from deleted iframes, and documents from
> XMLHttpRequest.  Here's a summary of the behavior.  (I also extended
> the test case to include all three cases.)
> 
> 
> IE8:
> 
>   * deleted iframe: exception on _any_ access ("permission denied")
>   * createDocument: not supported in IE8 (no createDocument method)
>   * XMLHttpRequest: exception on setting, getting returns undefined
> 
>   (The XMLHttpRequest document in IE8 does not seem to support a
>   cookie attribute at all.  The responseXML object supports a
>   getElementsByTagName() method, so it seems to really be a document,
>   and not the dummy object IE8 returns upon parsing errors.)
> 
> 
> FF3.5:
> 
>   * deleted iframe: no exception, setting and getting allowed
>   * createDocument: no exception, setting and getting allowed
>   * XMLHttpRequest: no exception, setting and getting allowed
> 
> 
> Safari 4.04/WebKit r51759:
> 
>   * deleted iframe: no exception, setting and getting allowed
>   * createDocument: no exception, setting silently ignored, getting
>                     returns empty string
>   * XMLHttpRequest: no exception, setting silently ignored, getting
>                     returns empty string
> 
> 
> Patch:
> 
>   * createDocument: exception
>   * XMLHttpRequest: exception
>   * deleted iframe: exception
> 
> 
> My interpretation of HTML5 is that an exception should be raised in
> all these three cases.  Am I reading the spec right?  Is this is how
> WebKit should behave?

Given the test results, I think the HTML5 spec is in error and should be changed. The Firefox behavior seems best - throwing more exceptions is a Web compatibility risk. And since many sites have dual code paths, being like IE is not as good as being like Firefox for practical Web compatibility. If we change this, it should be to better match Firefox, and we should file a bug against the HTML5 spec.

[Maciej Stachowiak]

Comment on attachment 44688 [details]
Patch with extended test case

r-

[Ian 'Hixie' Hickson]

I can't reproduce your results with Firefox. Do you have a test case online that I can check that shows that Firefox allows .cookie changes on XHR docs?

[Patrik Persson]

(In reply to comment #17)
> I can't reproduce your results with Firefox. Do you have a test case online
> that I can check that shows that Firefox allows .cookie changes on XHR docs?

Sorry for not responding earlier. I've now put my (simple) test case at

http://hem.bredband.net/patrikpersson/cookie/cookie.html

[Adam Barth]

Our current behavior matches Firefox on the given test case.