Make PageLoadMetricsWaiter more resilient

Make PageLoadMetricsWaiter more resilient

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level, and only starting waiting once a load commits, we have a
page load level view of updates and can more precisely guarantee we are
waiting on the right page load's updates.

BUG=725998
Review-Url: https://codereview.chromium.org/2903693004
Cr-Commit-Position: refs/heads/master@{#474996}
Committed: https://chromium.googlesource.com/chromium/src/+/ab243857c7a70b2c42c521896c588f01ac2eae46

[Bryan McQuade, 2017-05-24 16:29:06]

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

[commit-bot: I haz the power, 2017-05-24 16:29:46]

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-24 17:47:59]

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

[commit-bot: I haz the power, 2017-05-24 17:48:57]

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-24 17:54:58]

Description was changed from

==========
Make browser tests wait on PLMO-level timing callbacks.

BUG=
==========

to

==========
Make browser tests wait on PLMO-level timing callbacks.

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level we have a page load level view of updates and can
more precisely guarantee we are waiting on the right page load's updates.

BUG=
==========

[Bryan McQuade, 2017-05-24 17:59:50]

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

[commit-bot: I haz the power, 2017-05-24 17:59:58]

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-24 18:01:20]

Description was changed from

==========
Make browser tests wait on PLMO-level timing callbacks.

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level we have a page load level view of updates and can
more precisely guarantee we are waiting on the right page load's updates.

BUG=
==========

to

==========
Make PageLoadMetricsWaiter more resilient

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level we have a page load level view of updates and can
more precisely guarantee we are waiting on the right page load's updates.

BUG=
==========

[Bryan McQuade, 2017-05-24 18:02:15]

Description was changed from

==========
Make PageLoadMetricsWaiter more resilient

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level we have a page load level view of updates and can
more precisely guarantee we are waiting on the right page load's updates.

BUG=
==========

to

==========
Make PageLoadMetricsWaiter more resilient

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level, and only starting waiting once a load commits, we have a
page load level view of updates and can more precisely guarantee we are
waiting on the right page load's updates.

BUG=
==========

[Bryan McQuade, 2017-05-24 18:02:34]

Description was changed from

==========
Make PageLoadMetricsWaiter more resilient

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level, and only starting waiting once a load commits, we have a
page load level view of updates and can more precisely guarantee we are
waiting on the right page load's updates.

BUG=
==========

to

==========
Make PageLoadMetricsWaiter more resilient

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level, and only starting waiting once a load commits, we have a
page load level view of updates and can more precisely guarantee we are
waiting on the right page load's updates.

BUG=725998
==========

[Bryan McQuade, 2017-05-24 18:04:27]

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

[Bryan McQuade, 2017-05-24 18:05:47]

PTAL, thanks!

This builds on pending change https://codereview.chromium.org/2904533002 to
improve the resiliency of the PageLoadMetricsWaiter.

This is a general improvement to our test infra, and will be needed should we
decide to
batch and defer timing updates due to out of order paint updates.

[commit-bot: I haz the power, 2017-05-24 19:32:13]

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

[commit-bot: I haz the power, 2017-05-24 19:32:14]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-25 01:05:56]

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

[commit-bot: I haz the power, 2017-05-25 01:07:06]

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-25 02:59:35]

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

[commit-bot: I haz the power, 2017-05-25 02:59:36]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-25 14:45:22]

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

[commit-bot: I haz the power, 2017-05-25 14:45:43]

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-25 14:57:03]

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

[commit-bot: I haz the power, 2017-05-25 14:57:07]

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-25 15:54:23]

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

[commit-bot: I haz the power, 2017-05-25 15:54:42]

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-25 16:46:22]

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

[commit-bot: I haz the power, 2017-05-25 16:46:48]

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-25 18:30:48]

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

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

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-05-25 19:35:34]

LGTM % comments

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

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:436: for
(auto& observer : testing_observers_)
nit: for loops in the file generally have braces.

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:189:
embedder_interface_ = new TestPageLoadMetricsEmbedderInterface();
optional nit to avoid bare new (I know this is old code)

auto embedder_interface = base::MakeUnique<TestInterface>();
embedder_interface_ = embedder_interface.get();

then std::move(embedder_interface) below

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:84:
CHECK(did_add_observer_);
#include "base/logging"

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:84:
CHECK(did_add_observer_);
Should we ASSERT instead of CHECK here?

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:114:
run_loop_.reset(new base::RunLoop());
nit: #include "base/run_loop" and use base::MakeUnique

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:144: //
metrics updates.
Can you explain why you need weak ptrs?

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:340: // the page
or subframe level. This method may be called multiple times over
Aside: It's weird to me to distinguish "page" vs "subframe" when usually we're
comparing main frame vs subframe. I think we really do want that distinction
because for "page" we aggregate, is that intuition correct?

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:345: virtual void
OnTimingUpdate(bool is_subframe,
optional: For consistency would it make sense to have two methods OnSubframe...
and OnMainFrame... like how we do it in the dispatcher client?

[Bryan McQuade, 2017-05-25 19:53:47]

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

[Bryan McQuade, 2017-05-25 19:54:06]

Thanks! Addressed comments.

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

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:436: for
(auto& observer : testing_observers_)
On 2017/05/25 at 19:35:33, Charlie Harrison wrote:
> nit: for loops in the file generally have braces.

All of the other testing observer for loops don't use braces, so if it's ok I'd
like to stay consistent with those.

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
(right):

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:189:
embedder_interface_ = new TestPageLoadMetricsEmbedderInterface();
On 2017/05/25 at 19:35:33, Charlie Harrison wrote:
> optional nit to avoid bare new (I know this is old code)
> 
> auto embedder_interface = base::MakeUnique<TestInterface>();
> embedder_interface_ = embedder_interface.get();
> 
> then std::move(embedder_interface) below

Good idea, this makes it clearer that we're holding on to a unique_ptr. Done,
thanks!

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:84:
CHECK(did_add_observer_);
On 2017/05/25 at 19:35:33, Charlie Harrison wrote:
> #include "base/logging"

added include

ASSERT macros fail to compile in destructors, so I used CHECKs here.

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:118:
EXPECT_TRUE(expectations_satisfied());
the codereview site is refusing to show me your comment a few lines earlier, but
I've updated run_loop_ to use MakeUnique and added the include.

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:144: //
metrics updates.
On 2017/05/25 at 19:35:34, Charlie Harrison wrote:
> Can you explain why you need weak ptrs?

Done - added comment to constructor.

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:340: // the page
or subframe level. This method may be called multiple times over
On 2017/05/25 at 19:35:34, Charlie Harrison wrote:
> Aside: It's weird to me to distinguish "page" vs "subframe" when usually we're
comparing main frame vs subframe. I think we really do want that distinction
because for "page" we aggregate, is that intuition correct?

Correct - page is essentially main frame + aggregated data for a few page-level
fields (paint timings). I agree this isn't super clear. I added a comment to
explain.

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:345: virtual void
OnTimingUpdate(bool is_subframe,
On 2017/05/25 at 19:35:34, Charlie Harrison wrote:
> optional: For consistency would it make sense to have two methods
OnSubframe... and OnMainFrame... like how we do it in the dispatcher client?

I did this initially but decided that since this was really intended only for
testing, keeping the testing API thin was beneficial.

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

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-25 19:59:44]

LGTM

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

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:436: for
(auto& observer : testing_observers_)
On 2017/05/25 19:54:05, Bryan McQuade wrote:
> On 2017/05/25 at 19:35:33, Charlie Harrison wrote:
> > nit: for loops in the file generally have braces.
> 
> All of the other testing observer for loops don't use braces, so if it's ok
I'd
> like to stay consistent with those.

Ah yea, only looked at those for loops close to this one. We should standardize
it in PLM code at some point but this is fine.

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (right):

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:84:
CHECK(did_add_observer_);
On 2017/05/25 19:54:05, Bryan McQuade wrote:
> On 2017/05/25 at 19:35:33, Charlie Harrison wrote:
> > #include "base/logging"
> 
> added include
> 
> ASSERT macros fail to compile in destructors, so I used CHECKs here.

Weird... fine by me

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2903693004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:345: virtual void
OnTimingUpdate(bool is_subframe,
On 2017/05/25 19:54:06, Bryan McQuade wrote:
> On 2017/05/25 at 19:35:34, Charlie Harrison wrote:
> > optional: For consistency would it make sense to have two methods
> OnSubframe... and OnMainFrame... like how we do it in the dispatcher client?
> 
> I did this initially but decided that since this was really intended only for
> testing, keeping the testing API thin was beneficial.

Makes sense to me

[commit-bot: I haz the power, 2017-05-26 00:29:18]

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

[commit-bot: I haz the power, 2017-05-26 00:29:19]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Bryan McQuade, 2017-05-26 01:33:46]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2017-05-26 01:35:08]

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-26 03:23:37]

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

[commit-bot: I haz the power, 2017-05-26 03:23:38]

Failed to apply patch for
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:
While running git apply --index -3 -p1;
  error: patch failed:
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc:192
  Falling back to three-way merge...
  Applied patch to
'chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc'
with conflicts.
  U chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc

Patch:      
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
Index:
chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
diff --git
a/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
b/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
index
ba0fbf0d6b9ddaeb76cca2ee656e642e998dc6e6..8951471af54af426f030b319af0a22148a64d840
100644
--- a/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
+++ b/chrome/browser/page_load_metrics/metrics_web_contents_observer_unittest.cc
@@ -38,18 +38,14 @@ const char kFilteredCommitUrl[] =
"https://whatever.com/ignore-on-commit";
 
 // Simple PageLoadMetricsObserver that copies observed PageLoadTimings into the
 // provided std::vector, so they can be analyzed by unit tests.
-class TestPageLoadMetricsObserver
-    : public PageLoadMetricsObserver,
-      public MetricsWebContentsObserver::TestingObserver {
+class TestPageLoadMetricsObserver : public PageLoadMetricsObserver {
  public:
   TestPageLoadMetricsObserver(
-      content::WebContents* web_contents,
       std::vector<mojom::PageLoadTimingPtr>* updated_timings,
       std::vector<mojom::PageLoadTimingPtr>* updated_subframe_timings,
       std::vector<mojom::PageLoadTimingPtr>* complete_timings,
       std::vector<GURL>* observed_committed_urls)
-      : MetricsWebContentsObserver::TestingObserver(web_contents),
-        updated_timings_(updated_timings),
+      : updated_timings_(updated_timings),
         updated_subframe_timings_(updated_subframe_timings),
         complete_timings_(complete_timings),
         observed_committed_urls_(observed_committed_urls) {}
@@ -61,17 +57,13 @@ class TestPageLoadMetricsObserver
     return CONTINUE_OBSERVING;
   }
 
-  void OnTimingUpdate(const mojom::PageLoadTiming& timing,
+  void OnTimingUpdate(bool is_subframe,
+                      const mojom::PageLoadTiming& timing,
                       const PageLoadExtraInfo& extra_info) override {
-    updated_timings_->push_back(timing.Clone());
-  }
-
-  void OnTimingUpdated(bool is_main_frame,
-                       const mojom::PageLoadTiming& timing,
-                       const mojom::PageLoadMetadata& metadata) override {
-    if (!is_main_frame) {
+    if (is_subframe)
       updated_subframe_timings_->push_back(timing.Clone());
-    }
+    else
+      updated_timings_->push_back(timing.Clone());
   }
 
   void OnComplete(const mojom::PageLoadTiming& timing,
@@ -126,16 +118,14 @@ class FilteringPageLoadMetricsObserver : public
PageLoadMetricsObserver {
 class TestPageLoadMetricsEmbedderInterface
     : public PageLoadMetricsEmbedderInterface {
  public:
-  explicit TestPageLoadMetricsEmbedderInterface(
-      content::WebContents* web_contents)
-      : web_contents_(web_contents), is_ntp_(false) {}
+  TestPageLoadMetricsEmbedderInterface() : is_ntp_(false) {}
 
   bool IsNewTabPageUrl(const GURL& url) override { return is_ntp_; }
   void set_is_ntp(bool is_ntp) { is_ntp_ = is_ntp; }
   void RegisterObservers(PageLoadTracker* tracker) override {
     tracker->AddObserver(base::MakeUnique<TestPageLoadMetricsObserver>(
-        web_contents_, &updated_timings_, &updated_subframe_timings_,
-        &complete_timings_, &observed_committed_urls_));
+        &updated_timings_, &updated_subframe_timings_, &complete_timings_,
+        &observed_committed_urls_));
     tracker->AddObserver(base::MakeUnique<FilteringPageLoadMetricsObserver>(
         &completed_filtered_urls_));
   }
@@ -166,7 +156,6 @@ class TestPageLoadMetricsEmbedderInterface
   std::vector<mojom::PageLoadTimingPtr> complete_timings_;
   std::vector<GURL> observed_committed_urls_;
   std::vector<GURL> completed_filtered_urls_;
-  content::WebContents* web_contents_;
   bool is_ntp_;
 };
 
@@ -192,20 +181,18 @@ class MetricsWebContentsObserverTest : public
ChromeRenderViewHostTestHarness {
 
   void SimulateTimingUpdate(const mojom::PageLoadTiming& timing,
                             content::RenderFrameHost* render_frame_host) {
-    observer_->OnTimingUpdated(render_frame_host, timing,
-                               mojom::PageLoadMetadata());
+    observer()->OnTimingUpdated(render_frame_host, timing,
+                                mojom::PageLoadMetadata());
   }
 
   void AttachObserver() {
-    embedder_interface_ =
-        new TestPageLoadMetricsEmbedderInterface(web_contents());
-    // Owned by the web_contents. Tests must be careful not to call
-    // SimulateTimingUpdate after they call DeleteContents() without also
-    // calling AttachObserver() again. Otherwise they will use-after-free the
-    // observer_.
-    observer_ = MetricsWebContentsObserver::CreateForWebContents(
-        web_contents(), base::WrapUnique(embedder_interface_));
-    observer_->WasShown();
+    auto embedder_interface =
+        base::MakeUnique<TestPageLoadMetricsEmbedderInterface>();
+    embedder_interface_ = embedder_interface.get();
+    MetricsWebContentsObserver* observer =
+        MetricsWebContentsObserver::CreateForWebContents(
+            web_contents(), std::move(embedder_interface));
+    observer->WasShown();
   }
 
   void CheckErrorEvent(InternalErrorLoadEvent error, int count) {
@@ -255,9 +242,12 @@ class MetricsWebContentsObserverTest : public
ChromeRenderViewHostTestHarness {
   }
 
  protected:
+  MetricsWebContentsObserver* observer() {
+    return MetricsWebContentsObserver::FromWebContents(web_contents());
+  }
+
   base::HistogramTester histogram_tester_;
   TestPageLoadMetricsEmbedderInterface* embedder_interface_;
-  MetricsWebContentsObserver* observer_;
 
  private:
   int num_errors_;
@@ -648,7 +638,7 @@ TEST_F(MetricsWebContentsObserverTest,
FlushMetricsOnAppEnterBackground) {
   histogram_tester_.ExpectTotalCount(
       internal::kPageLoadCompletedAfterAppBackground, 0);
 
-  observer_->FlushMetricsOnAppEnterBackground();
+  observer()->FlushMetricsOnAppEnterBackground();
 
   histogram_tester_.ExpectTotalCount(
       internal::kPageLoadCompletedAfterAppBackground, 1);

[Bryan McQuade, 2017-05-26 03:29:15]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-05-26 03:29:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2903693004/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-26 03:30:00]

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-26 03:42:36]

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

[commit-bot: I haz the power, 2017-05-26 03:42:37]

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

[Bryan McQuade, 2017-05-26 12:10:47]

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

[commit-bot: I haz the power, 2017-05-26 12:11:02]

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-26 12:21:30]

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

[commit-bot: I haz the power, 2017-05-26 12:21:31]

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

[Bryan McQuade, 2017-05-26 12:28:32]

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

[commit-bot: I haz the power, 2017-05-26 12:28:43]

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-26 13:27:44]

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

[commit-bot: I haz the power, 2017-05-26 13:27:45]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-26 13:36:02]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-05-26 13:36:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2903693004/#ps240001
(title: "fix compile")

[commit-bot: I haz the power, 2017-05-26 13:36:16]

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-26 13:40:12]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495805762137020,
"parent_rev": "9ca00e8a7155d18f064358504bc460fece4232dc", "commit_rev":
"ab243857c7a70b2c42c521896c588f01ac2eae46"}

[commit-bot: I haz the power, 2017-05-26 13:40:25]

Description was changed from

==========
Make PageLoadMetricsWaiter more resilient

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level, and only starting waiting once a load commits, we have a
page load level view of updates and can more precisely guarantee we are
waiting on the right page load's updates.

BUG=725998
==========

to

==========
Make PageLoadMetricsWaiter more resilient

Our current test infrastructure exposes timing update callbacks at a layer just
above the IPC processing layer, in MetricsWebContentsObserver via the
TestingObserver interface.

This works fine when timing updates are dispatched synchronously to
PageLoadMetricsObservers as part of the IPC processing flow.

However, this strategy no longer works if updates are buffered before
being dispatched. We may buffer updates in the browser process to allow
for correct processing of out of order inter frame timing updates. This would
require observing callbacks at a higher level than the MWCO timing update
processing flow.

The simplest way to address this is to allow the test infrastructure
to inject a PageLoadMetricsObserver that can be used to wait for timing
updates. This guarantees that our waiting logic is waiting on the same logic
as other observers, which makes the tests more resilient.

In addition, we can restrict waiting to a single page load - there have been
some subtle browsertest bugs in the past where waiters waiting on a timing
from one page load incorrectly stopped waiting too early because a
previous page load dispatched a matching timing update. By observing
at the PLMO level, and only starting waiting once a load commits, we have a
page load level view of updates and can more precisely guarantee we are
waiting on the right page load's updates.

BUG=725998

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

[commit-bot: I haz the power, 2017-05-26 13:40:27]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/ab243857c7a70b2c42c521896c58...