155054 2016-03-04 16:56:46 -0800 Resource load statistics are not honoring private browsing 2016-03-04 21:24:40 -0800 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 bfulgham bfulgham aestes bfulgham cdumez commit-queue esprehn+autocc japhet kangil.han webkit-bug-importer wilander oldest_to_newest 1171111 0 bfulgham 2016-03-04 16:56:46 -0800 The Resource Load Statistics stuff should skip private browsing sessions (at least for now). 1171113 1 webkit-bug-importer 2016-03-04 16:57:36 -0800 <rdar://problem/24987873> 1171132 2 273063 bfulgham 2016-03-04 17:52:35 -0800 Created attachment 273063 Patch 1171134 3 273063 aestes 2016-03-04 18:16:14 -0800 Comment on attachment 273063 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. 1171144 4 273063 bfulgham 2016-03-04 20:37:52 -0800 Comment on attachment 273063 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. 1171145 5 273063 aestes 2016-03-04 20:39:33 -0800 Comment on attachment 273063 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. 1171147 6 bfulgham 2016-03-04 21:24:40 -0800 Committed r197608: <http://trac.webkit.org/changeset/197608> 273063 2016-03-04 17:52:35 -0800 2016-03-04 18:16:14 -0800 Patch bug-155054-20160304175221.patch text/plain 3367 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5NzYwNSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDE2LTAzLTA0ICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJlc291cmNlIGxvYWQgc3Rh dGlzdGljcyBhcmUgbm90IGhvbm9yaW5nIHByaXZhdGUgYnJvd3NpbmcKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1NTA1NAorICAgICAgICA8cmRhcjov L3Byb2JsZW0vMjQ5ODc4NzM+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgTW9kaWZ5IHRoZSBwb2ludHMgd2hlcmUgd2UgY2FwdHVyZSByZXNvdXJjZSBs b2FkIHN0YXRpc3RpY3MgdG8gaWdub3JlCisgICAgICAgIGxvYWRzIG1hZGUgZHVyaW5nIHByaXZh dGUgYnJvd3NpbmcuCisKKyAgICAgICAgKiBkb20vRG9jdW1lbnQuY3BwOgorICAgICAgICAoV2Vi Q29yZTo6RG9jdW1lbnQ6OnVwZGF0ZUxhc3RIYW5kbGVkVXNlckdlc3R1cmVUaW1lc3RhbXApOgor ICAgICAgICAqIGxvYWRlci9Eb2N1bWVudExvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpE b2N1bWVudExvYWRlcjo6d2lsbFNlbmRSZXF1ZXN0KToKKyAgICAgICAgKiBsb2FkZXIvU3VicmVz b3VyY2VMb2FkZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U3VicmVzb3VyY2VMb2FkZXI6Ondp bGxTZW5kUmVxdWVzdEludGVybmFsKToKKwogMjAxNi0wMy0wNCAgU2FtIFdlaW5pZyAgPHNhbUB3 ZWJraXQub3JnPgogCiAgICAgICAgIFtXZWJLaXQyXSBBZGQgV2ViS2l0MiBlcXVpdmFsZW50IG9m IC1bV2ViVmlldyBfaW5zZXJ0TmV3bGluZUluUXVvdGVkQ29udGVudF0KSW5kZXg6IFNvdXJjZS9X ZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvZG9t L0RvY3VtZW50LmNwcAkocmV2aXNpb24gMTk3NTk3KQorKysgU291cmNlL1dlYkNvcmUvZG9tL0Rv Y3VtZW50LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjQwMiw3ICs2NDAyLDggQEAgRG9jdW1lbnQ6 OlJlZ2lvbkZpeGVkUGFpciBEb2N1bWVudDo6YWJzbwogCiB2b2lkIERvY3VtZW50Ojp1cGRhdGVM YXN0SGFuZGxlZFVzZXJHZXN0dXJlVGltZXN0YW1wKCkKIHsKLSAgICBpZiAoIW1fbGFzdEhhbmRs ZWRVc2VyR2VzdHVyZVRpbWVzdGFtcCkKKyAgICBib29sIG5lZWRQcml2YWN5ID0gcGFnZSgpID8g cGFnZSgpLT51c2VzRXBoZW1lcmFsU2Vzc2lvbigpIDogZmFsc2U7CisgICAgaWYgKCFtX2xhc3RI YW5kbGVkVXNlckdlc3R1cmVUaW1lc3RhbXAgJiYgIW5lZWRQcml2YWN5KQogICAgICAgICBSZXNv dXJjZUxvYWRPYnNlcnZlcjo6c2hhcmVkT2JzZXJ2ZXIoKS5sb2dVc2VySW50ZXJhY3Rpb24oKnRo aXMpOwogCiAgICAgbV9sYXN0SGFuZGxlZFVzZXJHZXN0dXJlVGltZXN0YW1wID0gbW9ub3Rvbmlj YWxseUluY3JlYXNpbmdUaW1lKCk7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvRG9jdW1l bnRMb2FkZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2xvYWRlci9Eb2N1bWVu dExvYWRlci5jcHAJKHJldmlzaW9uIDE5NzU5NykKKysrIFNvdXJjZS9XZWJDb3JlL2xvYWRlci9E b2N1bWVudExvYWRlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTUzMSw4ICs1MzEsMTAgQEAgdm9p ZCBEb2N1bWVudExvYWRlcjo6d2lsbFNlbmRSZXF1ZXN0KFJlcwogCiAgICAgQVNTRVJUKG1fZnJh bWUtPmRvY3VtZW50KCkpOwogICAgIEFTU0VSVCh0b3BGcmFtZS5kb2N1bWVudCgpKTsKKyAgICBB U1NFUlQodG9wRnJhbWUucGFnZSgpKTsKIAotICAgIGlmIChTZXR0aW5nczo6cmVzb3VyY2VMb2Fk U3RhdGlzdGljc0VuYWJsZWQoKSkKKyAgICBib29sIG5lZWRQcml2YWN5ID0gdG9wRnJhbWUucGFn ZSgpID8gdG9wRnJhbWUucGFnZSgpLT51c2VzRXBoZW1lcmFsU2Vzc2lvbigpIDogZmFsc2U7Cisg ICAgaWYgKFNldHRpbmdzOjpyZXNvdXJjZUxvYWRTdGF0aXN0aWNzRW5hYmxlZCgpICYmICFuZWVk UHJpdmFjeSkKICAgICAgICAgUmVzb3VyY2VMb2FkT2JzZXJ2ZXI6OnNoYXJlZE9ic2VydmVyKCku bG9nRnJhbWVOYXZpZ2F0aW9uKCFyZWRpcmVjdFJlc3BvbnNlLmlzTnVsbCgpLCBtX2ZyYW1lLT5k b2N1bWVudCgpLT51cmwoKSwgbmV3UmVxdWVzdC51cmwoKSwgbV9mcmFtZS0+aXNNYWluRnJhbWUo KSwgdG9wRnJhbWUuZG9jdW1lbnQoKS0+dXJsKCkpOwogICAgIAogICAgIC8vIFVwZGF0ZSBjb29r aWUgcG9saWN5IGJhc2UgVVJMIGFzIFVSTCBjaGFuZ2VzLCBleGNlcHQgZm9yIHN1YmZyYW1lcywg d2hpY2ggdXNlIHRoZQpJbmRleDogU291cmNlL1dlYkNvcmUvbG9hZGVyL1N1YnJlc291cmNlTG9h ZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvU3VicmVzb3VyY2VM b2FkZXIuY3BwCShyZXZpc2lvbiAxOTc1OTcpCisrKyBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvU3Vi cmVzb3VyY2VMb2FkZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xOTEsNyArMTkxLDggQEAgdm9p ZCBTdWJyZXNvdXJjZUxvYWRlcjo6d2lsbFNlbmRSZXF1ZXN0SQogICAgIGlmIChuZXdSZXF1ZXN0 LmlzTnVsbCgpKQogICAgICAgICBjYW5jZWwoKTsKIAotICAgIGlmIChTZXR0aW5nczo6cmVzb3Vy Y2VMb2FkU3RhdGlzdGljc0VuYWJsZWQoKSkKKyAgICBib29sIG5lZWRQcml2YWN5ID0gKG1fZnJh bWUgJiYgbV9mcmFtZS0+cGFnZSgpKSA/IG1fZnJhbWUtPnBhZ2UoKS0+dXNlc0VwaGVtZXJhbFNl c3Npb24oKSA6IGZhbHNlOworICAgIGlmIChTZXR0aW5nczo6cmVzb3VyY2VMb2FkU3RhdGlzdGlj c0VuYWJsZWQoKSAmJiAhbmVlZFByaXZhY3kpCiAgICAgICAgIFJlc291cmNlTG9hZE9ic2VydmVy OjpzaGFyZWRPYnNlcnZlcigpLmxvZ1N1YnJlc291cmNlTG9hZGluZyghcmVkaXJlY3RSZXNwb25z ZS5pc051bGwoKSwgcmVkaXJlY3RSZXNwb25zZS51cmwoKSwgbmV3UmVxdWVzdC51cmwoKSwgbV9m cmFtZSA/IG1fZnJhbWUtPm1haW5GcmFtZSgpLmRvY3VtZW50KCktPnVybCgpIDogVVJMKCkpOwog fQogCg==