[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL now labels all finished frame navigations (regardless of success) and
removes an incorrect dcheck.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=708570
Review-Url: https://codereview.chromium.org/2861433005
Cr-Commit-Position: refs/heads/master@{#470701}
Committed: https://chromium.googlesource.com/chromium/src/+/6f3a10ddea2ee602ff61267ce3ed6a60c8e6ff71

[jkarlin, 2017-05-04 14:32:54]

Patchset #1 (id:1) has been deleted

[jkarlin, 2017-05-04 14:59:57]

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

[commit-bot: I haz the power, 2017-05-04 15:02:43]

Dry run: CQ is trying da patch.

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

[jkarlin, 2017-05-04 15:23:04]

Patchset #2 (id:40001) has been deleted

[jkarlin, 2017-05-04 15:23:09]

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

[commit-bot: I haz the power, 2017-05-04 15:23:32]

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, 2017-05-04 16:12:21]

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

[commit-bot: I haz the power, 2017-05-04 16:12:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkarlin, 2017-05-04 16:51:53]

Patchset #2 (id:60001) has been deleted

[jkarlin, 2017-05-04 16:53:49]

Patchset #2 (id:80001) has been deleted

[jkarlin, 2017-05-04 16:55:45]

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

[commit-bot: I haz the power, 2017-05-04 16:56:12]

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, 2017-05-04 18:20:26]

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

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

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-05-04 19:16:30]

Description was changed from

==========
[PageLoadMetrics] Fix dchecks in ads observer

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL replaces the dchecks with UMA (to see how often this happens) and
early returns.

BUG=708570
==========

to

==========
[PageLoadMetrics] Handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL replaces the dchecks with UMA (to see how often this happens) and
early returns.

BUG=708570
==========

[jkarlin, 2017-05-04 19:17:02]

jkarlin@chromium.org changed reviewers:
+ bmcquade@chromium.org, csharrison@chromium.org

[jkarlin, 2017-05-04 19:17:03]

Charles, Bryan: PTAL! Thanks!

[Charlie Harrison, 2017-05-04 20:21:14]

Looks good!

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:22:
const base::Feature kAdsFeature{"AdsMetrics", base::FEATURE_ENABLED_BY_DEFAULT};
Please mention in the description that this CL re-enables the feature.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:78:
navigation_handle->GetNetErrorCode() != net::ERR_ABORTED) {
#include net errors

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:79:
// We're not interested in tracking this navigation. In case we've seen a
IMO pulling this whole comment above the if statement is clearer and makes the
if statement easier to parse.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:86:
// navigation).
I imagine this is not the only way the navigation can be aborted, is it?

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:92:
// NavigationHandle->GetRenderFrameHost doesn't like being called for
Slightly awkward wording. It's helpful to differentiate between the FTN the
navigation is happening in, and the RFH. The canonical RFH associated with a
navigation is the one it commits in. "Doesn't like" peppers over this
distinction as arbitrary.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:16:
ASSERT_TRUE(embedded_test_server()->Start());
#include gtest

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:21:
DISALLOW_COPY_AND_ASSIGN(AdsPageLoadMetricsObserverBrowserTest);
#include base/macros

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:26:
// Test that a subframe that aborts (due to doc.write) doesn't cause a crash
optional: I prefer comments above test names.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:28:
ui_test_utils::NavigateToURL(
#include gurl

https://codereview.chromium.org/2861433005/diff/100001/chrome/test/data/ads_o...
File chrome/test/data/ads_observer/docwrite_provisional_frame.html (right):

https://codereview.chromium.org/2861433005/diff/100001/chrome/test/data/ads_o...
chrome/test/data/ads_observer/docwrite_provisional_frame.html:11:
doc.write('<html>Rewritten. <img src=pixel.png><img src=pixel2.png><img
src=pixel3.png></html>');
Can you confirm that the images are successfully loaded? I know it makes the
test more complicated but I am only vaguely sure that this even works.

My suggestion is to use document.write to write something like:
<img src="pixel.png" onload="postMessage('loaded');"

Then in the parent use a message listener + domAutomationController to send a
signal to the test harness.

[Bryan McQuade, 2017-05-04 20:53:14]

LGTM, thanks for addressing this case!

[jkarlin, 2017-05-05 14:15:12]

Description was changed from

==========
[PageLoadMetrics] Handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL replaces the dchecks with UMA (to see how often this happens) and
early returns.

BUG=708570
==========

to

==========
[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL replaces the dchecks with UMA (to see how often this happens) and
early returns.

BUG=708570
==========

[jkarlin, 2017-05-05 15:41:15]

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

[commit-bot: I haz the power, 2017-05-05 15:41:41]

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, 2017-05-05 15:52:15]

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

[commit-bot: I haz the power, 2017-05-05 15:52:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jkarlin, 2017-05-05 18:41:12]

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

[commit-bot: I haz the power, 2017-05-05 18:41:58]

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, 2017-05-05 19:48:26]

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

[commit-bot: I haz the power, 2017-05-05 19:48:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkarlin, 2017-05-05 19:56:19]

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

[jkarlin, 2017-05-05 19:56:29]

csharrison@ PTAL

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:22:
const base::Feature kAdsFeature{"AdsMetrics", base::FEATURE_ENABLED_BY_DEFAULT};
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> Please mention in the description that this CL re-enables the feature.

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:78:
navigation_handle->GetNetErrorCode() != net::ERR_ABORTED) {
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> #include net errors

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:79:
// We're not interested in tracking this navigation. In case we've seen a
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> IMO pulling this whole comment above the if statement is clearer and makes the
> if statement easier to parse.

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:86:
// navigation).
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> I imagine this is not the only way the navigation can be aborted, is it?

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:92:
// NavigationHandle->GetRenderFrameHost doesn't like being called for
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> Slightly awkward wording. It's helpful to differentiate between the FTN the
> navigation is happening in, and the RFH. The canonical RFH associated with a
> navigation is the one it commits in. "Doesn't like" peppers over this
> distinction as arbitrary.

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:16:
ASSERT_TRUE(embedded_test_server()->Start());
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> #include gtest

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:21:
DISALLOW_COPY_AND_ASSIGN(AdsPageLoadMetricsObserverBrowserTest);
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> #include base/macros

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:26:
// Test that a subframe that aborts (due to doc.write) doesn't cause a crash
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> optional: I prefer comments above test names.

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:28:
ui_test_utils::NavigateToURL(
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> #include gurl

Done.

https://codereview.chromium.org/2861433005/diff/100001/chrome/test/data/ads_o...
File chrome/test/data/ads_observer/docwrite_provisional_frame.html (right):

https://codereview.chromium.org/2861433005/diff/100001/chrome/test/data/ads_o...
chrome/test/data/ads_observer/docwrite_provisional_frame.html:11:
doc.write('<html>Rewritten. <img src=pixel.png><img src=pixel2.png><img
src=pixel3.png></html>');
On 2017/05/04 20:21:14, Charlie Harrison wrote:
> Can you confirm that the images are successfully loaded? I know it makes the
> test more complicated but I am only vaguely sure that this even works.
> 
> My suggestion is to use document.write to write something like:
> <img src="pixel.png" onload="postMessage('loaded');"
> 
> Then in the parent use a message listener + domAutomationController to send a
> signal to the test harness.

Sure. Done.

[commit-bot: I haz the power, 2017-05-05 19:57:04]

Dry run: CQ is trying da patch.

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

[jkarlin, 2017-05-05 19:57:30]

Patchset #3 (id:120001) has been deleted

[jkarlin, 2017-05-05 19:57:38]

Patchset #3 (id:140001) has been deleted

[jkarlin, 2017-05-05 19:57:46]

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

[commit-bot: I haz the power, 2017-05-05 19:58:20]

Dry run: CQ is trying da patch.

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

[Charlie Harrison, 2017-05-05 20:17:48]

LGTM thanks

https://codereview.chromium.org/2861433005/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2861433005/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:34:
content::DOMMessageQueue msg_queue;
Cool, didn't know about that one.

[commit-bot: I haz the power, 2017-05-05 21:32:07]

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

[commit-bot: I haz the power, 2017-05-05 21:32:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkarlin, 2017-05-08 15:31:12]

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

[commit-bot: I haz the power, 2017-05-08 15:31:34]

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, 2017-05-08 17:17:29]

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

[commit-bot: I haz the power, 2017-05-08 17:17:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkarlin, 2017-05-09 18:47:50]

Patchset #6 (id:220001) has been deleted

[jkarlin, 2017-05-09 18:48:40]

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

[commit-bot: I haz the power, 2017-05-09 18:49:18]

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, 2017-05-09 19:18:11]

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

[commit-bot: I haz the power, 2017-05-09 19:18:12]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jkarlin, 2017-05-10 11:48:24]

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

[commit-bot: I haz the power, 2017-05-10 11:48:55]

Dry run: CQ is trying da patch.

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

[jkarlin, 2017-05-10 11:49:05]

Patchset #6 (id:240001) has been deleted

[jkarlin, 2017-05-10 11:50:24]

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

[commit-bot: I haz the power, 2017-05-10 11:50:57]

Dry run: CQ is trying da patch.

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

[jkarlin, 2017-05-10 13:10:09]

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

[commit-bot: I haz the power, 2017-05-10 13:10:18]

Dry run: CQ is trying da patch.

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

[jkarlin, 2017-05-10 13:31:47]

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

[jkarlin, 2017-05-10 13:31:52]

Patchset #6 (id:260001) has been deleted

[jkarlin, 2017-05-10 13:32:00]

Patchset #6 (id:280001) has been deleted

[commit-bot: I haz the power, 2017-05-10 13:32:58]

Dry run: CQ is trying da patch.

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

[jkarlin, 2017-05-10 13:43:05]

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

[jkarlin, 2017-05-10 13:43:08]

Patchset #6 (id:300001) has been deleted

[commit-bot: I haz the power, 2017-05-10 13:43:38]

Dry run: CQ is trying da patch.

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

[jkarlin, 2017-05-10 13:54:14]

Description was changed from

==========
[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL replaces the dchecks with UMA (to see how often this happens) and
early returns.

BUG=708570
==========

to

==========
[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL replaces the dchecks with UMA (to see how often this happens) and
early returns.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=708570
==========

[jkarlin, 2017-05-10 15:24:25]

bmcquade@ and csharrison@ I made a few changes, PTAL. 

I'm now labeling frames (as an ad or not) after any navigation finishes
regardless of navigation success.

[jkarlin, 2017-05-10 15:28:09]

Description was changed from

==========
[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL replaces the dchecks with UMA (to see how often this happens) and
early returns.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=708570
==========

to

==========
[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL now labels all finished frame navigations (regardless of success) and
removes an incorrect dcheck.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=708570
==========

[commit-bot: I haz the power, 2017-05-10 15:54:16]

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

[commit-bot: I haz the power, 2017-05-10 15:54:17]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Bryan McQuade, 2017-05-10 16:21:28]

lgtm

[Charlie Harrison, 2017-05-10 16:47:21]

LGTM % nits

https://codereview.chromium.org/2861433005/diff/320001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2861433005/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:35:
navigation_handle->GetWebContents()->UnsafeFindFrameByFrameTreeNodeId(
Can you comment why the "Unsafe" is necessary?

https://codereview.chromium.org/2861433005/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:89:
// labels all of a page's frames, even those that fail to navigate.
s/navigate/commit

[jkarlin, 2017-05-10 18:18:46]

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

[jkarlin, 2017-05-10 18:19:24]

Thanks!

https://codereview.chromium.org/2861433005/diff/320001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2861433005/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:35:
navigation_handle->GetWebContents()->UnsafeFindFrameByFrameTreeNodeId(
On 2017/05/10 16:47:21, Charlie Harrison wrote:
> Can you comment why the "Unsafe" is necessary?

I commented on way it's safe to use in this case. It doesn't seem worth looking
up the pid if it's not necessary to do so.

https://codereview.chromium.org/2861433005/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:89:
// labels all of a page's frames, even those that fail to navigate.
On 2017/05/10 16:47:21, Charlie Harrison wrote:
> s/navigate/commit

Done.

[commit-bot: I haz the power, 2017-05-10 18:19:51]

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, 2017-05-10 18:35:31]

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

[commit-bot: I haz the power, 2017-05-10 18:35:32]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jkarlin, 2017-05-10 18:45:47]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2017-05-10 18:45:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2861433005/#ps340001
(title: "Address comments from PS6")

[commit-bot: I haz the power, 2017-05-10 18:47:31]

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-05-10 20:50:50]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1494441947864070,
"parent_rev": "2525d998c7c50bce5ca17fb934c2aeb8e27e1db0", "commit_rev":
"6f3a10ddea2ee602ff61267ce3ed6a60c8e6ff71"}

[commit-bot: I haz the power, 2017-05-10 20:50:58]

Description was changed from

==========
[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL now labels all finished frame navigations (regardless of success) and
removes an incorrect dcheck.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=708570
==========

to

==========
[PageLoadMetrics] Reenable AdsMetrics and handle case where navigation aborts

It turns out that a frame can abort and continue to load resources even if it
has no previous navigation. This happens if a doc.write overwrites the frame
navigation while the navigation is still provisional.

This CL now labels all finished frame navigations (regardless of success) and
removes an incorrect dcheck.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

BUG=708570

Review-Url: https://codereview.chromium.org/2861433005
Cr-Commit-Position: refs/heads/master@{#470701}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f3a10ddea2ee602ff61267ce3ed...
==========

[commit-bot: I haz the power, 2017-05-10 20:51:00]

Committed patchset #7 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/6f3a10ddea2ee602ff61267ce3ed...

[Nico, 2017-05-11 20:18:51]

A revert of this CL (patchset #7 id:340001) has been created in
https://codereview.chromium.org/2876823003/ by thakis@chromium.org.

The reason for reverting is: Test is flaky, see
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....