Bug 63545 – dom/html/level2/html/HTMLDocument12.html and dom/xhtml/level2/html/HTMLDocument12.xhtml fail on my machine

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

dom/html/level2/html/HTMLDocument12.html and dom/xhtml/level2/html/HTMLDocument12.xhtml fail on my machine

They've failed for as long as I can remember.  With both new-run-webkit-tests and old-run-webkit-tests.


--- /tmp/layout-test-results/dom/html/level2/html/HTMLDocument12-expected.txt	2011-06-28 10:32:51.000000000 -0700
+++ /tmp/layout-test-results/dom/html/level2/html/HTMLDocument12-actual.txt	2011-06-28 10:32:51.000000000 -0700
@@ -1,2 +1,3 @@
-Test:	http://www.w3.org/2001/DOM-Test-Suite/level2/html/HTMLDocument12
-Status:	Success
+Test:	http://www.w3.org/2001/DOM-Test-Suite/level2/html/HTMLDocument12	
+Status:	failure
+Detail:	cookieLink: assertEquals failed, actual style=null, expected .


--- /tmp/layout-test-results/dom/xhtml/level2/html/HTMLDocument12-expected.txt	2011-06-28 10:33:20.000000000 -0700
+++ /tmp/layout-test-results/dom/xhtml/level2/html/HTMLDocument12-actual.txt	2011-06-28 10:33:20.000000000 -0700
@@ -1,2 +1,3 @@
 Test	http://www.w3.org/2001/DOM-Test-Suite/level2/html/HTMLDocument12
-Status	Success
+Status	failure
+Message	cookieLink: assertEquals failed, actual style=null, expected .

I suspect I may have some stray cookie somewhere.  I'm not sure.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Darin Adler]

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

[Alexey Proskuryakov]

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

[Eric Seidel (no email)]

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.

[Adam Barth]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

Created attachment 105395 [details]
proposed patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 105395 [details]
proposed patch

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

[Alexey Proskuryakov]

Created attachment 105412 [details]
updated patch

Forgot to save a file with some minor last minute changes.

[Jessie Berlin]

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.

[Alexey Proskuryakov]

Created attachment 105418 [details]
with Tools changes

Thanks Jessie! I forgot to include Tools subdirectory when making the patch, uploading a new one.

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Comment on attachment 105418 [details]
with Tools changes

Attachment 105418 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9539392

[Darin Adler]

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.

[Darin Adler]

Maybe: SwitchNetworkLoaderToNewEmptyTransientDataStorage

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Build fix attempt in r93992.

[Alexey Proskuryakov]

Windows build fix in r93995.

[Alexey Proskuryakov]

Lion build fix in r93998.

[Alexey Proskuryakov]

More Windows build fix in r93999.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

Comment on attachment 105550 [details]
new fix

r=me assuming you fix the Windows build

[Alexey Proskuryakov]

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

[Ryosuke Niwa]

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

[Darin Adler]

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

[Darin Adler]

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

[Ryosuke Niwa]

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