Bug 32115

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

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

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.

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

> * 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.

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

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+.

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

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

Created attachment 44688 [details] Patch with extended test case Now tests documents created using createDocument(), documents from deleted iframes, and documents from XMLHttpRequest.

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

(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?

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.

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?

(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.

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

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?

(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

Our current behavior matches Firefox on the given test case.