[PageLoadMetrics] Relax invariants and log the exceptions

[PageLoadMetrics] Relax invariants and log the exceptions

It turns out that:

1) We can get resource loads for frames that don't belong to the page
and
2) We can get notice of frame commits for frames that don't belong to the page.

We hope this is rare (and record histograms to verify) and relax the constraints.

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

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

Description was changed from

==========
[PageLoadMetrics] Loosen invariants and log the exceptions

It turns out that:

1) We can get resource loads for frames that don't belong to the page
and
2) We can get notice of frame commits for frames that don't belong to the page.

We hope this is rare (and record histograms to verify) and relax the
constraints.

BUG=721369
==========

to

==========
[PageLoadMetrics] Relax invariants and log the exceptions

It turns out that:

1) We can get resource loads for frames that don't belong to the page
and
2) We can get notice of frame commits for frames that don't belong to the page.

We hope this is rare (and record histograms to verify) and relax the
constraints.

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

[jkarlin, 2017-05-15 18:18:16]

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

[commit-bot: I haz the power, 2017-05-15 18:19: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-15 18:21:39]

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

[jkarlin, 2017-05-15 18:21:53]

Patchset #1 (id:1) has been deleted

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

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-15 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-15 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...

[jkarlin, 2017-05-15 18:50:06]

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

[commit-bot: I haz the power, 2017-05-15 18:50:23]

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-15 18:50:26]

Patchset #2 (id:40001) has been deleted

[jkarlin, 2017-05-15 18:52:34]

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

[jkarlin, 2017-05-15 18:52:44]

Patchset #1 (id:20001) has been deleted

[jkarlin, 2017-05-15 18:52:52]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-05-15 18:54:00]

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-15 22:47:07]

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

[commit-bot: I haz the power, 2017-05-15 22:47:08]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-05-16 11:04:40]

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

[commit-bot: I haz the power, 2017-05-16 11:04: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-16 11:06:19]

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

[jkarlin, 2017-05-16 11:06:19]

bmcquade: PTAL at everything
holte: PTAL at histograms.xml and enums.xml

Thanks!

[commit-bot: I haz the power, 2017-05-16 11:51:48]

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

[commit-bot: I haz the power, 2017-05-16 11:51:49]

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_...)

[Bryan McQuade, 2017-05-16 16:22:36]

https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:221: //
condition. See https://crbug.com/711352.
Does csharrison's https://codereview.chromium.org/2885453003 address this?

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

https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:128:
UMA_HISTOGRAM_BOOLEAN("PageLoad.Clients.Ads.Google.ParentExistsForSubFrame",
histogram reviewers typically ask me to factor this out into a helper e.g.
RecordParentExistsForSubFrame(bool exists); so the histogram declaration exists
only once. apparently the macro expands into a lot of code.

https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:208:
"PageLoad.Clients.Ads.Google.ResourceTypeWhenNoFrameFound",
can we use a different histogram name here, to distinguish the expected case
above from the unexpected case here? that way we get to verify that this latter
case truly is rare/nonexistent.

[jkarlin, 2017-05-16 17:16:03]

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

[jkarlin, 2017-05-16 17:16:09]

https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:221: //
condition. See https://crbug.com/711352.
On 2017/05/16 16:22:36, Bryan McQuade wrote:
> Does csharrison's https://codereview.chromium.org/2885453003 address this?

It addresses it for tests that use Navigation Simulator, but not tests that use
WebContentsTester which is the majority. I've updated the comment.

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

https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:128:
UMA_HISTOGRAM_BOOLEAN("PageLoad.Clients.Ads.Google.ParentExistsForSubFrame",
On 2017/05/16 16:22:36, Bryan McQuade wrote:
> histogram reviewers typically ask me to factor this out into a helper e.g.
> RecordParentExistsForSubFrame(bool exists); so the histogram declaration
exists
> only once. apparently the macro expands into a lot of code.

Done.

https://codereview.chromium.org/2884753002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:208:
"PageLoad.Clients.Ads.Google.ResourceTypeWhenNoFrameFound",
On 2017/05/16 16:22:36, Bryan McQuade wrote:
> can we use a different histogram name here, to distinguish the expected case
> above from the unexpected case here? that way we get to verify that this
latter
> case truly is rare/nonexistent.

They'll be different, as the case above will have resource type MAIN_FRAME or
SUB_FRAME and these will comprise the rest.

[commit-bot: I haz the power, 2017-05-16 17:16: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-16 17:21:23]

Patchset #3 (id:120001) has been deleted

[jkarlin, 2017-05-16 17:21:43]

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

[commit-bot: I haz the power, 2017-05-16 17:22:18]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-16 17:28:39]

lgtm, thanks!

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

https://codereview.chromium.org/2884753002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:203:
"PageLoad.Clients.Ads.Google.ResourceTypeWhenNoFrameFound",
ah, sorry, didn't realize this was already varying on resource type and thus we
don't need a separate histo name. here like the bool case i think the metrics
team asks that we factor histograms into a single helper function, so we should
try to do that here.

[jkarlin, 2017-05-16 17:33:20]

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

[jkarlin, 2017-05-16 17:33:59]

Thanks!

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

https://codereview.chromium.org/2884753002/diff/140001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.cc:203:
"PageLoad.Clients.Ads.Google.ResourceTypeWhenNoFrameFound",
On 2017/05/16 17:28:39, Bryan McQuade wrote:
> ah, sorry, didn't realize this was already varying on resource type and thus
we
> don't need a separate histo name. here like the bool case i think the metrics
> team asks that we factor histograms into a single helper function, so we
should
> try to do that here.

Done.

[commit-bot: I haz the power, 2017-05-16 17:34:22]

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-16 17:54:34]

The CQ bit was unchecked by jkarlin@chromium.org

[jkarlin, 2017-05-16 17:54:38]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2017-05-16 17:54:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2884753002/#ps160001
(title: "Reduce histogram calls")

[commit-bot: I haz the power, 2017-05-16 17:54:51]

CQ is trying da patch.

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

[jkarlin, 2017-05-16 18:15:36]

The CQ bit was unchecked by jkarlin@chromium.org

[Steven Holte, 2017-05-16 19:34:38]

histograms lgtm

[jkarlin, 2017-05-16 20:36:10]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2017-05-16 20:38:24]

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-16 22:20:10]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494966970646900,
"parent_rev": "396a1266fb7549e03e5bcff793ac138654e52d3a", "commit_rev":
"c33a8ddef6ca33f58a8a76b24db5a00073e5a9aa"}

[commit-bot: I haz the power, 2017-05-16 22:20:24]

Description was changed from

==========
[PageLoadMetrics] Relax invariants and log the exceptions

It turns out that:

1) We can get resource loads for frames that don't belong to the page
and
2) We can get notice of frame commits for frames that don't belong to the page.

We hope this is rare (and record histograms to verify) and relax the
constraints.

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

to

==========
[PageLoadMetrics] Relax invariants and log the exceptions

It turns out that:

1) We can get resource loads for frames that don't belong to the page
and
2) We can get notice of frame commits for frames that don't belong to the page.

We hope this is rare (and record histograms to verify) and relax the
constraints.

BUG=708570

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

[commit-bot: I haz the power, 2017-05-16 22:20:25]

Committed patchset #4 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/c33a8ddef6ca33f58a8a76b24db5...