Bug 155054

Summary: Resource load statistics are not honoring private browsing
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bfulgham, cdumez, commit-queue, esprehn+autocc, japhet, kangil.han, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch aestes: review+

Description Brent Fulgham 2016-03-04 16:56:46 PST
The Resource Load Statistics stuff should skip private browsing sessions (at least for now).
Comment 1 Radar WebKit Bug Importer 2016-03-04 16:57:36 PST
<rdar://problem/24987873>
Comment 2 Brent Fulgham 2016-03-04 17:52:35 PST
Created attachment 273063 [details]
Patch
Comment 3 Andy Estes 2016-03-04 18:16:14 PST
Comment on attachment 273063 [details]
Patch

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

r=me, but I'd like to see the policy decisions of whether or not to log be made inside ResourceLoadObserver, not by the callers.

> Source/WebCore/dom/Document.cpp:6406
> -    if (!m_lastHandledUserGestureTimestamp)
> +    bool needPrivacy = page() ? page()->usesEphemeralSession() : false;
> +    if (!m_lastHandledUserGestureTimestamp && !needPrivacy)

You're already passing a Document reference to ResourceLoadObserver. Can't the observer figure out if the Document is part of an ephemeral session and not log?

> Source/WebCore/loader/DocumentLoader.cpp:537
> -    if (Settings::resourceLoadStatisticsEnabled())
> +    bool needPrivacy = topFrame.page() ? topFrame.page()->usesEphemeralSession() : false;
> +    if (Settings::resourceLoadStatisticsEnabled() && !needPrivacy)

I'm surprised you check Settings::resourceLoadStatisticsEnabled() here, since you also check it inside logFrameNavigation().

I'd prefer to have these checks be inside ResourceLoadObserver. If you changed logFrameNavigation() to take a ResourceRequest, ResourceResponse, and Frame then it could determine everything it needs from those three things.

> Source/WebCore/loader/SubresourceLoader.cpp:195
> -    if (Settings::resourceLoadStatisticsEnabled())
> +    bool needPrivacy = (m_frame && m_frame->page()) ? m_frame->page()->usesEphemeralSession() : false;
> +    if (Settings::resourceLoadStatisticsEnabled() && !needPrivacy)

Same comment as above.
Comment 4 Brent Fulgham 2016-03-04 20:37:52 PST
Comment on attachment 273063 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:6406
>> +    if (!m_lastHandledUserGestureTimestamp && !needPrivacy)
> 
> You're already passing a Document reference to ResourceLoadObserver. Can't the observer figure out if the Document is part of an ephemeral session and not log?

Yep. I'll do that.

>> Source/WebCore/loader/DocumentLoader.cpp:537
>> +    if (Settings::resourceLoadStatisticsEnabled() && !needPrivacy)
> 
> I'm surprised you check Settings::resourceLoadStatisticsEnabled() here, since you also check it inside logFrameNavigation().
> 
> I'd prefer to have these checks be inside ResourceLoadObserver. If you changed logFrameNavigation() to take a ResourceRequest, ResourceResponse, and Frame then it could determine everything it needs from those three things.

Brady suggested this, because we want to avoid computing multiple URLs if we have this diagnostic turned off. By checking this boolean first, we only make a very simple operation before doing any meaningful work.

However, I'm open to revisiting this. For example, maybe we pass these various values into the method and calculate URL's, etc., inside the method.
Comment 5 Andy Estes 2016-03-04 20:39:33 PST
Comment on attachment 273063 [details]
Patch

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

>>> Source/WebCore/loader/DocumentLoader.cpp:537
>>> +    if (Settings::resourceLoadStatisticsEnabled() && !needPrivacy)
>> 
>> I'm surprised you check Settings::resourceLoadStatisticsEnabled() here, since you also check it inside logFrameNavigation().
>> 
>> I'd prefer to have these checks be inside ResourceLoadObserver. If you changed logFrameNavigation() to take a ResourceRequest, ResourceResponse, and Frame then it could determine everything it needs from those three things.
> 
> Brady suggested this, because we want to avoid computing multiple URLs if we have this diagnostic turned off. By checking this boolean first, we only make a very simple operation before doing any meaningful work.
> 
> However, I'm open to revisiting this. For example, maybe we pass these various values into the method and calculate URL's, etc., inside the method.

Right, I was thinking you'd just pass in redirectResponse, newRequest, and m_frame. You'd only compute the URLs if the logging is enabled.
Comment 6 Brent Fulgham 2016-03-04 21:24:40 PST
Committed r197608: <http://trac.webkit.org/changeset/197608>