Wireup SafeBrowsingNavigationObserverManager to help PPAPI download attribution

Identify download referrer chain of PPAPI download based on initiating frame url.

Also address attribution corner case where new tab navigation immediately
followed by a server side redirection (and add browser test for it).

BUG=676691
Review-Url: https://codereview.chromium.org/2601303002
Cr-Commit-Position: refs/heads/master@{#441794}
Committed: https://chromium.googlesource.com/chromium/src/+/af664d30ba29fd8e744f5b94fc3d9702ccdf09f6

[Jialiu Lin, 2016-12-29 21:49:13]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 21:49:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 22:52:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 22:52:08]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-01-03 18:28:32]

Description was changed from

==========
Identify download referrer chain of PPAPI download based on frame url.

BUG=676691
==========

to

==========
Identify download referrer chain of PPAPI download based on initiating frame
url.

BUG=676691
==========

[Jialiu Lin, 2017-01-03 18:29:36]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 18:29:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-01-03 18:38:33]

jialiul@chromium.org changed reviewers:
+ asanka@chromium.org, nparker@chromium.org

[Jialiu Lin, 2017-01-03 18:38:34]

Happy New Year! 
This CL wire up SafeBrowsingNavigationObserverManager to trace back navigation
events that lead to PPAPI download.

[commit-bot: I haz the power, 2017-01-03 19:30:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 19:30:39]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2017-01-04 19:49:12]

lgtm

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:550:
WebContents::FromRenderFrameHost(render_frame_host_),
possible idea: Do you want to just pass render_frame_host_?

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.h (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.h:303: // Then add
referrer chain info of to ClientDownloadRequest proto. This
s/ of//

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc
(right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:48:
int current_user_gesture_count,
What is current_user_gesture_count? Is that how many we've seen so far stepping
backward through navigations?

Would it be simpler to have one arg, as
  bool at_user_gesture_limit
and have the callers compute it? Not sure.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:463:
if (last_nav_event_traced->source_url.is_empty() &&
q: Up above in a few places is_valid() is used.  Is there a reason to use one vs
the other?

[Jialiu Lin, 2017-01-04 21:53:34]

jialiul@chromium.org changed reviewers:
+ bbudge@chromium.org, holte@chromium.org, jochen@chromium.org,
raymes@chromium.org

[Jialiu Lin, 2017-01-04 21:53:35]

Thanks nparker!  asanka@, let me know if you have any comments. 


+holte@ for tiny change in histograms.xml
+raymes@/bbudge@ for owner review of 
chrome/test/ppapi/ppapi_filechooser_browsertest.cc
+jochen@ for chrome/browser/file_select_helper.cc

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:550:
WebContents::FromRenderFrameHost(render_frame_host_),
On 2017/01/04 19:49:12, Nathan Parker wrote:
> possible idea: Do you want to just pass render_frame_host_?

I thought of that. There is one risk though, during the checking (since it's a
remote call), the GetLastCommittedURL() might return a different URL, if this
bad frame tries to quickly redirect to another irrelevant site.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.h (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.h:303: // Then add
referrer chain info of to ClientDownloadRequest proto. This
On 2017/01/04 19:49:12, Nathan Parker wrote:
> s/ of//

Done.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc
(right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:48:
int current_user_gesture_count,
On 2017/01/04 19:49:12, Nathan Parker wrote:
> What is current_user_gesture_count? Is that how many we've seen so far
stepping
> backward through navigations?
> 
> Would it be simpler to have one arg, as
>   bool at_user_gesture_limit
> and have the callers compute it? Not sure.

Good suggestion. Thanks!

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:463:
if (last_nav_event_traced->source_url.is_empty() &&
On 2017/01/04 19:49:12, Nathan Parker wrote:
> q: Up above in a few places is_valid() is used.  Is there a reason to use one
vs
> the other?

invalid url could be empty or malformatted. Here I use is_empty() is because the
source url and source main frame url of browser initiated navigation can only be
empty.

BTW, I need to do a rebase here, since now FindNavigationEvent takes 3 args.

[Steven Holte, 2017-01-04 22:02:25]

histograms lgtm

[jochen (gone - plz use gerrit), 2017-01-04 22:24:55]

lgtm

[Jialiu Lin, 2017-01-05 01:03:36]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 01:04:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-01-05 01:14:55]

Description was changed from

==========
Identify download referrer chain of PPAPI download based on initiating frame
url.

BUG=676691
==========

to

==========
Identify download referrer chain of PPAPI download based on initiating frame
url.

Also address attribution corner case where new tab navigation immediately
followed by a server side redirection (and add browser test for it).

BUG=676691
==========

[Jialiu Lin, 2017-01-05 01:15:37]

jialiul@chromium.org changed reviewers:
- bbudge@chromium.org, raymes@chromium.org

[asanka, 2017-01-05 18:00:44]

whoops. I thought I sent out these comments already.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:550:
WebContents::FromRenderFrameHost(render_frame_host_),
web_contents_, but you'd need to be prepared for the possibility that the
web_contents_ may have been destroyed at this point. If so web_contents_ would
be nullptr.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1243:
web_contents_(web_contents),
not safe to store web_contents without observing it. It might get destroyed
while PPAPIDownloadRequest is thread-hopping.

If possible, considering capturing the attribution chain here instead of waiting
until later.

[Jialiu Lin, 2017-01-05 20:06:56]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[Jialiu Lin, 2017-01-05 20:07:15]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-01-05 20:07:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-01-05 20:12:21]

Thanks, asanka@! 
I moved the tab_id and has_user_gesture computation to the PPAPIDownloadRequest
constructor as you suggested.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:550:
WebContents::FromRenderFrameHost(render_frame_host_),
On 2017/01/05 18:00:44, asanka wrote:
> web_contents_, but you'd need to be prepared for the possibility that the
> web_contents_ may have been destroyed at this point. If so web_contents_ would
> be nullptr.

That's fine. If web_contents_ is null,  
SessionTabHelper::IdForTab(web_contents_) will return -1. Then identifying
referrer chain will be purely based on url.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1243:
web_contents_(web_contents),
On 2017/01/05 18:00:44, asanka wrote:
> not safe to store web_contents without observing it. It might get destroyed
> while PPAPIDownloadRequest is thread-hopping.
> 
> If possible, considering capturing the attribution chain here instead of
waiting
> until later.

Though my current code can work with null web_contents_, your suggestion is
definitely much better. In my attribution code, web_contents_ is used to
determine two things: (1) tab_id (default -1, if web_contents is not available,
then attribution will be based only on url), (2) whether this web_content_ has
any user gesture (default false).  So I'll compute these two things here instead
of later. Thanks!

[asanka, 2017-01-05 20:35:31]

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:550:
WebContents::FromRenderFrameHost(render_frame_host_),
On 2017/01/05 at 20:12:21, Jialiu Lin wrote:
> On 2017/01/05 18:00:44, asanka wrote:
> > web_contents_, but you'd need to be prepared for the possibility that the
> > web_contents_ may have been destroyed at this point. If so web_contents_
would
> > be nullptr.
> 
> That's fine. If web_contents_ is null,  
SessionTabHelper::IdForTab(web_contents_) will return -1. Then identifying
referrer chain will be purely based on url.

Yup. But we are still indirecting through a nullptr in the case of
render_frame_host_ here (render_frame_host_->GetLastCommitedURL() will crash).
Both web_contents_ and render_frame_host_ can become nullptr due to a number of
external factors. Unfortunately we need to check and handle these cases each
time we hop back on the UI thread.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1243:
web_contents_(web_contents),
On 2017/01/05 at 20:12:21, Jialiu Lin wrote:
> On 2017/01/05 18:00:44, asanka wrote:
> > not safe to store web_contents without observing it. It might get destroyed
> > while PPAPIDownloadRequest is thread-hopping.
> > 
> > If possible, considering capturing the attribution chain here instead of
waiting
> > until later.
> 
> Though my current code can work with null web_contents_, your suggestion is
definitely much better. In my attribution code, web_contents_ is used to
determine two things: (1) tab_id (default -1, if web_contents is not available,
then attribution will be based only on url), (2) whether this web_content_ has
any user gesture (default false).  So I'll compute these two things here instead
of later. Thanks!

Cool. Though my worry here was that web_contents_ would point to invalid memory
rather than be nullptr by the time the original code got around to using it. The
lifetime of these objects are quite finicky :-)

https://codereview.chromium.org/2601303002/diff/120001/chrome/browser/file_se...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2601303002/diff/120001/chrome/browser/file_se...
chrome/browser/file_select_helper.cc:549: requestor_url,
render_frame_host_->GetLastCommittedURL(),
render_frame_host_ could be nullptr :-(

[commit-bot: I haz the power, 2017-01-05 21:53:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 21:53:43]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-01-05 22:10:13]

Thanks, asanka@!

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/file_sel...
chrome/browser/file_select_helper.cc:550:
WebContents::FromRenderFrameHost(render_frame_host_),
On 2017/01/05 20:35:31, asanka wrote:
> On 2017/01/05 at 20:12:21, Jialiu Lin wrote:
> > On 2017/01/05 18:00:44, asanka wrote:
> > > web_contents_, but you'd need to be prepared for the possibility that the
> > > web_contents_ may have been destroyed at this point. If so web_contents_
> would
> > > be nullptr.
> > 
> > That's fine. If web_contents_ is null,  
> SessionTabHelper::IdForTab(web_contents_) will return -1. Then identifying
> referrer chain will be purely based on url.
> 
> Yup. But we are still indirecting through a nullptr in the case of
> render_frame_host_ here (render_frame_host_->GetLastCommitedURL() will crash).
> Both web_contents_ and render_frame_host_ can become nullptr due to a number
of
> external factors. Unfortunately we need to check and handle these cases each
> time we hop back on the UI thread.
Agree... I added checks here in case render_frame_host_ is nullptr. Function
WebContents::FromeRenderFrameHost(..) itself will check the validity of
renderframehost passed in.

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2601303002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1243:
web_contents_(web_contents),
On 2017/01/05 20:35:31, asanka wrote:
> On 2017/01/05 at 20:12:21, Jialiu Lin wrote:
> > On 2017/01/05 18:00:44, asanka wrote:
> > > not safe to store web_contents without observing it. It might get
destroyed
> > > while PPAPIDownloadRequest is thread-hopping.
> > > 
> > > If possible, considering capturing the attribution chain here instead of
> waiting
> > > until later.
> > 
> > Though my current code can work with null web_contents_, your suggestion is
> definitely much better. In my attribution code, web_contents_ is used to
> determine two things: (1) tab_id (default -1, if web_contents is not
available,
> then attribution will be based only on url), (2) whether this web_content_ has
> any user gesture (default false).  So I'll compute these two things here
instead
> of later. Thanks!
> 
> Cool. Though my worry here was that web_contents_ would point to invalid
memory
> rather than be nullptr by the time the original code got around to using it.
The
> lifetime of these objects are quite finicky :-)
We can probably trust WebContents::FromRenderFrameHost(rfh) (called in
FileSelectHelper::CheckDownloadRequestWithSafeBrowsing) to return the valid
web_contents or nullptr in case rfh is not avaialble. 
And there is no thread-hopping or delay between
CheckDownloadRequestWithSafeBrowsing() and this constructor here.
And now all the uses of web_contents are moved to this constructor. I think
we're quite safe here.

https://codereview.chromium.org/2601303002/diff/120001/chrome/browser/file_se...
File chrome/browser/file_select_helper.cc (right):

https://codereview.chromium.org/2601303002/diff/120001/chrome/browser/file_se...
chrome/browser/file_select_helper.cc:549: requestor_url,
render_frame_host_->GetLastCommittedURL(),
On 2017/01/05 20:35:31, asanka wrote:
> render_frame_host_ could be nullptr :-(

Added a check here. in case render_frame_host_ is nullptr

[asanka, 2017-01-05 22:30:51]

Thanks! lgtm

[Jialiu Lin, 2017-01-05 22:31:56]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-01-05 22:31:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, jochen@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2601303002/#ps140001
(title: "add check for rfh")

[commit-bot: I haz the power, 2017-01-05 22:32:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 01:34:55]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1483655516945190,
"parent_rev": "7759356c6d98a31d5e89a7165976e3104a858db8", "commit_rev":
"af664d30ba29fd8e744f5b94fc3d9702ccdf09f6"}

[commit-bot: I haz the power, 2017-01-06 01:35:34]

Description was changed from

==========
Identify download referrer chain of PPAPI download based on initiating frame
url.

Also address attribution corner case where new tab navigation immediately
followed by a server side redirection (and add browser test for it).

BUG=676691
==========

to

==========
Identify download referrer chain of PPAPI download based on initiating frame
url.

Also address attribution corner case where new tab navigation immediately
followed by a server side redirection (and add browser test for it).

BUG=676691

Review-Url: https://codereview.chromium.org/2601303002
Cr-Commit-Position: refs/heads/master@{#441794}
Committed:
https://chromium.googlesource.com/chromium/src/+/af664d30ba29fd8e744f5b94fc3d...
==========

[commit-bot: I haz the power, 2017-01-06 01:35:36]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/af664d30ba29fd8e744f5b94fc3d...