Bug 180883 – Add optional logging of per-resource cookie information

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Keith Rollin]

In order to support the tracking of the efficacy of Intelligent Tracking Protection, add some logging of the cookie information associated with each loaded resource. This logging is off by default, and is enabled with `sudo defaults write -g WebKitLogCookieInformation -bool true`.

[Keith Rollin]

Created attachment 329520 [details]
Patch

[EWS Watchlist]

Attachment 329520 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 329521 [details]
Fix GTK build.

[Alex Christensen]

Comment on attachment 329520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329520&action=review

Do we care to measure the creation of cookies that are made from a Set-Cookie header field and never leave CFNetwork?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:759
> +                "{ \"name\""       ": \"%{public}s\""
> +                ", \"value\""      ": \"%{public}s\""
> +                ", \"domain\""     ": \"%{public}s\""

What are these logs going to?  They contain privacy sensitive information.

[EWS Watchlist]

Attachment 329521 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 329710 [details]
Patch

[EWS Watchlist]

Attachment 329710 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:733:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:734:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:735:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:745:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:746:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:747:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:748:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:754:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:755:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 10 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 329743 [details]
Patch

[EWS Watchlist]

Attachment 329743 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:740:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:741:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:742:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:752:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:753:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:754:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:755:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:761:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:762:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 10 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 329744 [details]
Patch

[EWS Watchlist]

Attachment 329744 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:740:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:741:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:742:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:752:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:753:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:754:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:755:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:761:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:762:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 10 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 329774 [details]
Patch

[EWS Watchlist]

Attachment 329774 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:740:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:741:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:742:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:752:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:753:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:754:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:755:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:761:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:762:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 10 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 329825 [details]
Patch

[EWS Watchlist]

Attachment 329825 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:740:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:741:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:742:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:752:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:753:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:754:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:755:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:761:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:762:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 10 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Rollin]

Created attachment 329865 [details]
Patch

[EWS Watchlist]

Attachment 329865 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:740:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:741:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:742:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:752:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:753:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:754:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:755:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:761:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:762:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:76:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 10 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

<rdar://problem/35802295>

[Brent Fulgham]

(In reply to Keith Rollin from comment #0)
> In order to support the tracking of the efficacy of Intelligent Tracking
> Protection, add some logging of the cookie information associated with each
> loaded resource. This logging is off by default, and is enabled with `sudo
> defaults write -g WebKitLogCookieInformation -bool true`.

Unless you are running Safari as root, you probably don't need the 'sudo'. :-)

[Brent Fulgham]

Comment on attachment 329865 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329865&action=review

Looks (and works!) well. r=me, but please consider not dumping cookie logging when there are no cookies.

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:60
> +    id value = props[@"Created"];

Maybe:

id value = cookie.props[@"Created"];

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:67
> +        return toCanonicalFormat(((NSNumber*) value).doubleValue);

Please use ugly Objective-C syntax: "((NSNumber *)value)" and elsewhere.

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:108
> +    // FIXME: Set Created property

Do we need to do this in this patch?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:743
> +        LOCAL_LOG(R"(  "cookies": [)");

Do we want to output this information when the cookie.size() == 0? Maybe that's just noise in the log?

[Keith Rollin]

(In reply to Brent Fulgham from comment #20)
> Comment on attachment 329865 [details]
> > Source/WebCore/platform/network/cocoa/CookieCocoa.mm:60
> > +    id value = props[@"Created"];
> 
> Maybe:
> 
> id value = cookie.props[@"Created"];

OK.

> > Source/WebCore/platform/network/cocoa/CookieCocoa.mm:67
> > +        return toCanonicalFormat(((NSNumber*) value).doubleValue);
> 
> Please use ugly Objective-C syntax: "((NSNumber *)value)" and elsewhere.

I don't understand this comment. I don't know what you're suggesting I do.

> > Source/WebCore/platform/network/cocoa/CookieCocoa.mm:108
> > +    // FIXME: Set Created property
> 
> Do we need to do this in this patch?

It matched a similar comment already in the function. I could take it out.

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:743
> > +        LOCAL_LOG(R"(  "cookies": [)");
> 
> Do we want to output this information when the cookie.size() == 0? Maybe
> that's just noise in the log?

OK.

[Keith Rollin]

(In reply to Keith Rollin from comment #21)
> (In reply to Brent Fulgham from comment #20)
> > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:743
> > > +        LOCAL_LOG(R"(  "cookies": [)");
> > 
> > Do we want to output this information when the cookie.size() == 0? Maybe
> > that's just noise in the log?
> 
> OK.

Actually, I think I'd like to leave it in. By leaving it in, we make the client parsing easier (it doesn't have to test to see if 'cookies' exists). Also, while making the suggested change, I found that the code because more complex. If we make 'cookies' optional, then the preceding comma needs to be elided, and the line that closes the array and the object ("]}") at the same time needs to be split up. Because of the os_log works, it's problematic to handle these variations by building up text lines, and instead we need to handle all of the variations with separate condition logic that outputs crafted constant lines. I could do it, but the code is a lot uglier, harder to follow, and harder to extend.

[Brent Fulgham]

(In reply to Keith Rollin from comment #22)
> (In reply to Keith Rollin from comment #21)
> > (In reply to Brent Fulgham from comment #20)
> > > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:743
> > > > +        LOCAL_LOG(R"(  "cookies": [)");
> > > 
> > > Do we want to output this information when the cookie.size() == 0? Maybe
> > > that's just noise in the log?
> > 
> > OK.
> 
> Actually, I think I'd like to leave it in. By leaving it in, we make the
> client parsing easier (it doesn't have to test to see if 'cookies' exists).
> Also, while making the suggested change, I found that the code because more
> complex. If we make 'cookies' optional, then the preceding comma needs to be
> elided, and the line that closes the array and the object ("]}") at the same
> time needs to be split up. Because of the os_log works, it's problematic to
> handle these variations by building up text lines, and instead we need to
> handle all of the variations with separate condition logic that outputs
> crafted constant lines. I could do it, but the code is a lot uglier, harder
> to follow, and harder to extend.

Fair enough!

[Brent Fulgham]

(In reply to Keith Rollin from comment #21)
> (In reply to Brent Fulgham from comment #20)
> > Comment on attachment 329865 [details]
> 
> > > Source/WebCore/platform/network/cocoa/CookieCocoa.mm:67
> > > +        return toCanonicalFormat(((NSNumber*) value).doubleValue);
> > 
> > Please use ugly Objective-C syntax: "((NSNumber *)value)" and elsewhere.
> 
> I don't understand this comment. I don't know what you're suggesting I do.

Instead of this:
((NSNumber*) value).doubleValue

Do:
((NSNumber *)value).doubleValue

... and similar changes elsewhere,

[Keith Rollin]

(In reply to Brent Fulgham from comment #19)
> (In reply to Keith Rollin from comment #0)
> > .. and is enabled with `sudo
> > defaults write -g WebKitLogCookieInformation -bool true`.
> 
> Unless you are running Safari as root, you probably don't need the 'sudo'.
> :-)

Huh. I thought I determined that it was needed. But you're right -- it's not.

[Keith Rollin]

Created attachment 330017 [details]
Patch

[EWS Watchlist]

Attachment 330017 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:740:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:741:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:742:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:752:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:753:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:754:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:755:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:761:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:762:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/cocoa/CookieCocoa.mm:75:  Comma should be at the beginning of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 10 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 330017 [details]
Patch

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

New failing tests:
fast/mediastream/MediaStream-MediaElement-setObject-null.html

[EWS Watchlist]

Created attachment 330023 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[WebKit Commit Bot]

Comment on attachment 330017 [details]
Patch

Clearing flags on attachment: 330017

Committed r226226: <https://trac.webkit.org/changeset/226226>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.