Bug 180883 - Add optional logging of per-resource cookie information
Summary: Add optional logging of per-resource cookie information
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-15 14:06 PST by Keith Rollin
Modified: 2018-01-11 13:26 PST (History)
10 users (show)

See Also:


Attachments
Patch (17.95 KB, patch)
2017-12-15 14:39 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Fix GTK build. (19.15 KB, patch)
2017-12-15 14:51 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (18.38 KB, patch)
2017-12-18 16:45 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (18.95 KB, patch)
2017-12-19 00:47 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (18.95 KB, patch)
2017-12-19 00:53 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (19.27 KB, patch)
2017-12-19 10:51 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (21.99 KB, patch)
2017-12-19 14:26 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (21.98 KB, patch)
2017-12-19 18:21 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (21.79 KB, patch)
2017-12-21 00:19 PST, Keith Rollin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.55 MB, application/zip)
2017-12-21 01:30 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2017-12-15 14:06:35 PST
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`.
Comment 1 Keith Rollin 2017-12-15 14:39:33 PST
Created attachment 329520 [details]
Patch
Comment 2 Build Bot 2017-12-15 14:40:35 PST
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.
Comment 3 Keith Rollin 2017-12-15 14:51:22 PST
Created attachment 329521 [details]
Fix GTK build.
Comment 4 Alex Christensen 2017-12-15 14:51:42 PST
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.
Comment 5 Build Bot 2017-12-15 14:53:39 PST
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.
Comment 6 Keith Rollin 2017-12-18 16:45:03 PST
Created attachment 329710 [details]
Patch
Comment 7 Build Bot 2017-12-18 16:46:41 PST
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.
Comment 8 Keith Rollin 2017-12-19 00:47:02 PST
Created attachment 329743 [details]
Patch
Comment 9 Build Bot 2017-12-19 00:48:15 PST
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.
Comment 10 Keith Rollin 2017-12-19 00:53:57 PST
Created attachment 329744 [details]
Patch
Comment 11 Build Bot 2017-12-19 00:57:22 PST
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.
Comment 12 Keith Rollin 2017-12-19 10:51:24 PST
Created attachment 329774 [details]
Patch
Comment 13 Build Bot 2017-12-19 10:53:56 PST
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.
Comment 14 Keith Rollin 2017-12-19 14:26:43 PST
Created attachment 329825 [details]
Patch
Comment 15 Build Bot 2017-12-19 17:04:07 PST
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.
Comment 16 Keith Rollin 2017-12-19 18:21:22 PST
Created attachment 329865 [details]
Patch
Comment 17 Build Bot 2017-12-19 18:22:50 PST
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.
Comment 18 Brent Fulgham 2017-12-20 15:27:53 PST
<rdar://problem/35802295>
Comment 19 Brent Fulgham 2017-12-20 16:38:55 PST
(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'. :-)
Comment 20 Brent Fulgham 2017-12-20 16:52:41 PST
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?
Comment 21 Keith Rollin 2017-12-20 19:41:36 PST
(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.
Comment 22 Keith Rollin 2017-12-20 19:48:35 PST
(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.
Comment 23 Brent Fulgham 2017-12-20 19:51:04 PST
(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!
Comment 24 Brent Fulgham 2017-12-20 19:53:09 PST
(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,
Comment 25 Keith Rollin 2017-12-20 20:08:53 PST
(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.
Comment 26 Keith Rollin 2017-12-21 00:19:59 PST
Created attachment 330017 [details]
Patch
Comment 27 Build Bot 2017-12-21 00:21:47 PST
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.
Comment 28 Build Bot 2017-12-21 01:30:32 PST
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
Comment 29 Build Bot 2017-12-21 01:30:33 PST
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
Comment 30 WebKit Commit Bot 2017-12-21 09:53:53 PST
Comment on attachment 330017 [details]
Patch

Clearing flags on attachment: 330017

Committed r226226: <https://trac.webkit.org/changeset/226226>
Comment 31 WebKit Commit Bot 2017-12-21 09:53:55 PST
All reviewed patches have been landed.  Closing bug.