+clamy, nasko, creis

I don't quite agree with this approach. We have tried to avoid passing an id for navigations to the renderer, which is exactly what you suggest we do. Can't we just pass the GlobalRequestID to the renderer at commit time and have the renderer use it instead? Or since the WebContentsObserver method is called from RenderFrameHost, we could store it in RFH and just modify the structure returned by the renderer.

For reference, we went to great effort to remove page IDs as navigation identifiers, since they caused a large number of problems with the browser and renderer getting out of sync, confusing session history and leading to URL spoofs.  This is one reason we're reluctant to pass navigation IDs to the renderer, and we're focusing on Camille's proposal to have Mojo handles to navigations instead.  (Camille, do you have a link to that proposal or bug you can post here for reference?)

If I understand your 1st suggestion correctly, you mean pass the GlobalRequestID (GRID) to the renderer and have it use that request ID instead of the one it right now creates? The problem is that the renderer deals with request IDs, not GlobalRequestIDs. Meaning the child_id is always the one for the renderer. So when a renderer reports a request ID, we would have no way of knowing whether that request ID was issued by the browser or the renderer. We could change the renderer to use full GlobalRequestIDs. It seemed a bit overkill to me when I considered it first, and I was worried about security implications (a compromised renderer could send resource load notifications to the browser process for other renderers)
nasko@, creis@, I'd be interested on your perspective on this.

Regarding your 2nd suggestion, we would be storing that browser issued GRID in the RenderFrameHost and associate it back to the resource loaded when RenderFrameHostImpl::ResourceLoadComplete is invoked. I am not sure how we would know which navigation is associated to that ResourceLoadComplete notification? We can't use URLs (as we may have several frames with the same URL), and we don't know the request ID (it was created by the renderer).

(And just to clarify, I am talking about request resource IDs here, not navigation IDs.)

I wonder why we have RenderFrameHostImpl::ResourceLoadComplete in RFH? Poking at it I see that we also have subresource tracking there too. Is there a design doc on why these are routed through the RFH? Or in general how PageLoadMetrics will work with Network Service?

There is a short document regarding page load metrics and the network service:
https://docs.google.com/document/d/1980iNJjM6R09Q7_bgAqIJC8QEvQbi5wErAoNDuswS8I

Do you think there is a better place than the RFH for the resource load complete notifications to happen?

Short answer re ResourceLoadComplete: it's needed to get information about resource loads (navigation + subresource) for page load metrics and some other features, when the network service is in use.

Re IDs, Jay/I agree we want to avoid that. The PLM team was concerned that if they key data per navigation URL, in the case of reloads or multiple navigations at the same time this could fail (can renderer also commit a slightly different URL than what it was asked to commit?). If it can be guaranteed that there won't be multiple navigations at the same time with the same URL, then that's another option.

In reply to comment 2: creis@ I think you are talking about this document:
https://docs.google.com/document/d/1rnbtgCCQQU4-G3zjlZZ83scMXYNoiQHkOxl_N4QVi74/edit#heading=h.ujlzpfzb3n85

To summarize, when the resource load complete notification happens we know:
- the RFH for that resource
- the type of that resource (main frame, sub frame...)
- the URL of that resource

And we need to associate it with a previously committed navigation for a specific WebContents, for which we have a NavigationHandle.

My understanding is that using the 3 pieces of data (RFH, resource type, URL) might not guarantee we associate the resource load complete notification to its corresponding navigation handle.

nasko@, creis@ do you have any other suggestions?

Does the PageNavigation metrics team care about navigations that don't commit? If not, can you pass the request ID assigned by the renderer in the DidCommitProvisional message, store it in the RFH along with the GlobalRequest ID from the corresponding NavigationHandle and replace the ID sent by the renderer in the load complete notification message?

Great suggestion

Thanks Jay for working through this and thanks everyone else for the suggestions.

Page load metrics would prefer to know about resource loads for both committed and failed navs, but committed are likely most important, so if failed navs are difficult to support, we might be ok with just committed data.

Adding a few people who depend on this callback to get their feedback.

Josh and Ryan, do methods like
AdsPageLoadMetricsObserver::ProcessLoadedResource
CorePageLoadMetricsObserver::OnLoadedResource
DataReductionProxyMetricsObserver::OnLoadedResource
LoFiPageLoadMetricsObserver::OnLoadedResource
and other implementers of OnLoadedResource depend on it being invoked for frame resources that fail to commit? Or do we really only care about this for cases where we commit?

For AdsPageLoadMetricsObserver, I'm okay with missing document resources for failed loads.

raising priority to match bug that it blocks

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36164bd9230614c1425bcf4655e2a8c1da1fbd9d

commit 36164bd9230614c1425bcf4655e2a8c1da1fbd9d
Author: Clark DuVall <cduvall@chromium.org>
Date: Thu Aug 09 22:49:08 2018

Track page load metrics with network service

This uses the strategy suggested in  crbug.com/842445#c10  to map the
renderer request IDs to global request IDs so PageLoadMetrics can
associate ResourceLoadComplete events with the correct PageLoadTracker.

The LoadingMetricsFailed test is still failing because
DidFinishNavigation is called before ResourceLoadComplete, which means
the PageLoadTracker has already been destroyed before we can track the
failed request. From the discussion in  crbug.com/842445 , this doesn't
seem like a big deal.

Bug:  842445 , 816684
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ifb1c0d689840f626c0612ddab26a10d8ea9eb078
Reviewed-on: https://chromium-review.googlesource.com/1155800
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581942}
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/net/network_request_metrics_browsertest.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/predictors/loading_predictor_tab_helper.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/predictors/loading_predictor_tab_helper.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/predictors/loading_predictor_tab_helper_unittest.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/safe_browsing/client_side_detection_host.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/safe_browsing/client_side_detection_host.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/browser/frame_host/render_frame_host_delegate.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/common/frame_messages.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/public/browser/web_contents_observer.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/renderer/loader/web_url_loader_impl.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/renderer/loader/web_url_loader_impl.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/renderer/loader/web_url_loader_impl_unittest.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/renderer/loader/weburlresponse_extradata_impl.h
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/content/renderer/service_worker/service_worker_context_client.cc
[modify] https://crrev.com/36164bd9230614c1425bcf4655e2a8c1da1fbd9d/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

I'm going to close this bug, and we can move discussion on whether page load metrics for failed navs are necessary to issue 816684 since its the main tracking bug.