Bug 63545

http://trac.webkit.org/browser/trunk/LayoutTests/dom/html/level2/html/HTMLDocument12.html http://trac.webkit.org/browser/trunk/LayoutTests/dom/html/level2/html/HTMLDocument12.js http://trac.webkit.org/browser/trunk/LayoutTests/dom/xhtml/level2/html/HTMLDocument12.xhtml http://trac.webkit.org/browser/trunk/LayoutTests/dom/xhtml/level2/html/HTMLDocument12.js

I suspect we should just skip these tests, as document.cookie doesn't make any sense for file: urls. We're bleeding the cookie from some other test, I suspect. I tried deleting ~/Library/Application Support/DumpRenderTree (in effort to clear DRT's cookies), but that didn't help.

Checked in by Darin 5 years ago. Unless someone objects, I'm just going to skip these for everyone.

Sorry, I forgot to post the code in question: function HTMLDocument12() { 87 var success; 88 if(checkInitialization(builder, "HTMLDocument12") != null) return; 89 var nodeList; 90 var vcookie; 91 var doc; 92 93 var docRef = null; 94 if (typeof(this.doc) != 'undefined') { 95 docRef = this.doc; 96 } 97 doc = load(docRef, "doc", "document"); 98 vcookie = doc.cookie; 99 100 assertEquals("cookieLink","",vcookie); Assuming I"m reading the failure message correctly, I somehow have "style=null" set as my cookie on file urls. I'm not sure how to clear it. But checking document.cookie for file urls (at least assuming that its always empty) is bogus.

What was checked in by me 5 years ago? These tests pass for me unless I have stray cookies on my computer. I don’t think these should be skipped.

I think we need to make the testing machinery erase your local file cookies. Or at least make it easy for you to do so.

So you think that DRT shoudl clear cookies between every test? That's certainly another possible solution.

I'm happy to go either way. I don't see these tests as providing very much value, but clearing (at least file:) cookies between tests (if inexpensive) could be a win for all tests.

It looks like Cookies are shared across all applications on Mac OS X: http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Classes/NSHTTPCookieStorage_Class/Reference/Reference.html#//apple_ref/occ/cl/NSHTTPCookieStorage I suspect the right way to fix this is to make DRT use its own separate cookie storage, by overriding sharedCookieStorage: http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Classes/NSHTTPCookieStorage_Class/Reference/Reference.html#//apple_ref/occ/cl/NSHTTPCookieStorage similar to how we hijack the global pasteboard for DRT to prevent pollution. Darin, do you know who might best comment on this approach? I can certainly try, but without access to NSHTTPCookieStorage source code I may be stabbing blind here. :) (My mac-ninja skills are quite rusty at this point).

I suspect that WebView/Safari already has some fancy code for doing per-session/per-app cookies (for private browsing mode). Someone with access to that code could add that feature into DRT pretty easily I imagine.

Jessie, weren’t we already looking at this? Is there some bug to track this?

<rdar://problem/5666907> DumpRenderTree should begin each test with an empty cookie store

Entertainingly enough, it doesn't seem possible to clear document.cookie from a file url. I tried the following in Safari: <script> alert(document.cookie); document.cookie = ""; alert(document.cookie); document.cookie = "foo"; alert(document.cookie); </script> And showed the following on my machine: ALERT: style=null ALERT: style=null ALERT: foo=;style=null Eventually I just moved my ~/Library/Cookies/Cookies.plist out of the way to clear all the cookies.

> Entertainingly enough, it doesn't seem possible to clear document.cookie from a file url. That's not how document.cookie works. Assigning a string to document.cookie is like seeing a Set-Cookie header. Clearing cookie is hard unless you know how they were set.

I see. document.cookie = "" was what I gleaned from the (clearly equally confused) interblags.

I think we should fix DRT. :) But I also contend these tests are useless. They test document.cookie == "", that's it. The whole test. :) Currently an invalid assumption based on our DRT implementation.

Bug 64001 is another cookie failure which seems to occur on my machine.

Created attachment 105395 [details] proposed patch

Attachment 105395 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/network/cf/CookieStorageCFNet.h:35: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebCore/platform/network/cf/CookieStorageCFNet.h:39: The parameter name "cookieStorage" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 105395 [details] proposed patch Attachment 105395 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9548151

Created attachment 105412 [details] updated patch Forgot to save a file with some minor last minute changes.

Comment on attachment 105412 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=105412&action=review I think I am missing where usePrivateSessionForNetworkLoading is actually invoked. Otherwise unofficial r=me! > Source/WebCore/ChangeLog:49 > + adopt the session, becaus enothing in method name says that it will steal a reference from typo: “becaus enothing” > Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:31 > +#if USE(CFNETWORK) || USE(CFURLSTORAGESESSIONS) This confusingly makes it look like the whole file is in this #if. I brought it up with Alexey on IRC and he said he would add a comment to the #endif to make it clear that is not the case.

Created attachment 105418 [details] with Tools changes Thanks Jessie! I forgot to include Tools subdirectory when making the patch, uploading a new one.

Comment on attachment 105418 [details] with Tools changes View in context: https://bugs.webkit.org/attachment.cgi?id=105418&action=review > Tools/DumpRenderTree/mac/DumpRenderTree.mm:504 > + [WebPreferences _usePrivateSessionForNetworkLoading]; It's a little odd that this creates a new private session every time you call it. It reads like it should return a bool, telling if we are using a private session. Maybe something like _resetNetworkSessionToNewPrivateSessionForTesting? Not sure.

Comment on attachment 105418 [details] with Tools changes Attachment 105418 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9539392

Comment on attachment 105418 [details] with Tools changes View in context: https://bugs.webkit.org/attachment.cgi?id=105418&action=review The patch failed to compile on both Mac and Windows. > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:985 > + if (privateStorageSession()) Since this function uses privateStorageSession() twice we might want to use a local variable to hold the references. > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:989 > + RetainPtr<CFStringRef> cfIdentifier(AdoptCF, String::format("%s.PrivateBrowsing", base.utf8().data()).createCFString()); Seems possibly overkill to use String::format just to concatenate two strings. > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:168 > + RetainPtr<CFHTTPCookieStorageRef> cfCookieStorage = currentCFHTTPCookieStorage(); > + if (cfCookieStorage) I’d prefer to see the definition inside the if statement. > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:87 > +// For testing only, not to be confused with private browsing. > +WK_EXPORT void WKBundleUsePrivateSessionForNetworkLoading(WKBundleRef bundle); As Eric pointed out, the verb “use” here is not great; its tense is slightly ambiguous and makes it sound like this could be a poorly named boolean getter function. Maybe activatePrivateSessionForNetworkLoading? Not really sure the word “session” here is all that great. The fact that the machinery used is named “session” seems like an implementation detail. I’m also confused that there is a way to turn this on but not off. I’m not even sure what “private” means. Here’s one try at naming it: SwitchNetworkLoaderToNewNonPersistentDataStorage. OK, that wasn’t a very good try. > Source/WebKit/win/WebCookieManagerCFNet.cpp:45 > + // Need to retain locally to make sure the result is valid in caller. > + static RetainPtr<CFHTTPCookieStorageRef> result; > + result = currentCFHTTPCookieStorage(); Do you want to call currentCFHTTPCookieStorage one time or multiple times? If you call it a second time, and it returns a new value, then the return value from the first time will be released. Is that intentional? I think the comment ought to state that rule/intent a little more clearly.

Maybe: SwitchNetworkLoaderToNewEmptyTransientDataStorage

> Do you want to call currentCFHTTPCookieStorage one time or multiple times? Ideally, I would like to remove the API, but I doubt that I can. Other than that, I think that the proposed implementation is right - the only issue with it is abandoning an old session kept in the static variable when a client calls getter first, and then setter. > Maybe: SwitchNetworkLoaderToNewEmptyTransientDataStorage How about switchNetworkLoaderToNewTestingSession? Even though the word "session" comes from implementation, it seems like a very reasonable word, and the exposed concept matches implementation concept exactly. "Data storage" is misleading in that sessions also don't share connection cache, which is fairly important, and the session created by this function has name hardcoded for testing. > Seems possibly overkill to use String::format just to concatenate two strings. Moved code, but I'll fix it. > I’d prefer to see the definition inside the if statement. This function uses it again later.

Committed <http://trac.webkit.org/changeset/93987>. I fixed some build problems, and will watch bots for more.

Build fix attempt in r93992.

Windows build fix in r93995.

Lion build fix in r93998.

More Windows build fix in r93999.

Rolled out in <http://trac.webkit.org/changeset/94007>. I didn't realize the full meaning of test failures when I saw them first, this needs more work.

Created attachment 105550 [details] new fix LayoutTestController has a method to change cookie accept policy, which needed to be updated to support sessions. Not much new otherwise, but with all the build fixes, the patch became 20K larger, and is probably worth another look.

Comment on attachment 105550 [details] new fix r=me assuming you fix the Windows build

Committed <http://trac.webkit.org/changeset/94093> with (unverified) Windows build fix.

Is createPrivateBrowsingStorageSession only used in Mac? This patch appears to have caused a build failure on WinCairo. http://build.webkit.org/builders/WinCairo%20Debug%20%28Build%29/builds/10282/steps/compile-webkit/logs/stdio 6>d:\projects\buildslave\win-cairo-debug\build\webkitbuild\debug_cairo_cflite\include\webcore\ResourceHandle.h(207) : error C2143: syntax error : missing ';' before '<' 6>d:\projects\buildslave\win-cairo-debug\build\webkitbuild\debug_cairo_cflite\include\webcore\ResourceHandle.h(207) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int 6>d:\projects\buildslave\win-cairo-debug\build\webkitbuild\debug_cairo_cflite\include\webcore\ResourceHandle.h(207) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int 6>d:\projects\buildslave\win-cairo-debug\build\webkitbuild\debug_cairo_cflite\include\webcore\ResourceHandle.h(207) : error C2238: unexpected token(s) p

(In reply to comment #38) > Is createPrivateBrowsingStorageSession only used in Mac? On Mac and also on Windows if CFNetwork is used. > This patch appears to have caused a build failure on WinCairo. > > http://build.webkit.org/builders/WinCairo%20Debug%20%28Build%29/builds/10282/steps/compile-webkit/logs/stdio > > 6>d:\projects\buildslave\win-cairo-debug\build\webkitbuild\debug_cairo_cflite\include\webcore\ResourceHandle.h(207) : error C2143: syntax error : missing ';' before '<' > 6>d:\projects\buildslave\win-cairo-debug\build\webkitbuild\debug_cairo_cflite\include\webcore\ResourceHandle.h(207) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int > 6>d:\projects\buildslave\win-cairo-debug\build\webkitbuild\debug_cairo_cflite\include\webcore\ResourceHandle.h(207) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int > 6>d:\projects\buildslave\win-cairo-debug\build\webkitbuild\debug_cairo_cflite\include\webcore\ResourceHandle.h(207) : error C2238: unexpected token(s) p The main problem here is that RetainPtr.h is currently included in ResourceHandle.h only for Mac, and should also be included when USE(CFURLSTORAGESESSIONS) is true. But on the other hand, I don’t think CFURLSTORAGESESSIONS should be set for WinCairo, unless it uses CFNetwork for its I/O. I thought it did not.

I went to fix the build and saw that Ryosuke already did a different but similar fix.

(In reply to comment #39) > (In reply to comment #38) > > Is createPrivateBrowsingStorageSession only used in Mac? > > On Mac and also on Windows if CFNetwork is used. Ah, ok. Thanks for the clarification. I landed a speculative fix in http://trac.webkit.org/changeset/94138.