New issue
Advanced search Search tips

Issue 674730 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug
Proj-Servicification

Blocking:
issue 368813



Sign in to add a comment

Fix telemetry tests with PlzNavigate

Project Member Reported by jam@chromium.org, Dec 15 2016

Issue description

There are a few tests that are failing with PlzNavigate. See https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/350875 as an example:

telemetry_perf_unittests (with patch) telemetry_perf_unittests (with patch)
Run on OS: 'Windows-7-SP1'
failures:
core.stacktrace_unittest.TabStackTraceTest.testCrashMinimalSymbols


telemetry_unittests (with patch) telemetry_unittests (with patch)
Run on OS: 'Windows-7-SP1'
failures:
telemetry.page.page_run_end_to_end_unittest.ActualPageRunEndToEndTests.testWebPageReplay
 




https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/357185
telemetry_perf_unittests (with patch) telemetry_perf_unittests (with patch)
Run on OS: 'Ubuntu-12.04'
failures:
core.about_tracing_integration_test.AboutTracingIntegrationTest.testBasicTraceRecording
core.stacktrace_unittest.TabStackTraceTest.testCrashMinimalSymbols
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 16 2016

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

commit 4b93a7628e18e3c93cce9e0107bbd5573b1c9248
Author: jam <jam@chromium.org>
Date: Fri Dec 16 16:23:11 2016

Fix testCrashSymbols with PlzNavigate.

The test was looking for a specific stack trace in the renderer. With PlzNavigate, the stack trace is different. Use a method name that's common to both.

BUG= 674730 

Review-Url: https://codereview.chromium.org/2581833002
Cr-Commit-Position: refs/heads/master@{#439119}

[modify] https://crrev.com/4b93a7628e18e3c93cce9e0107bbd5573b1c9248/tools/perf/core/stacktrace_unittest.py

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 19 2016

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

commit af4c3f9c6ca41f00427a0f1f8de02782ce227629
Author: jam <jam@chromium.org>
Date: Mon Dec 19 22:24:27 2016

Fix telemetry hangs with PlzNavigate.

This was caused because some inspector protocol messages were being dropped with PlzNavigate with cross-process navigations. The scenario is that
1) FrameHostHolder::DispatchProtocolMessage was being called before the navigation happens. It adds the message to sent_messages_.
2) AboutToNavigate(NavigationHandle* navigation_handle) is called
3) FrameHostHolder::SendMessageToClient is called since the first renderer has responded to the protocol message from 1). It removes the sent message from sent_messages_ and forwards the reply using SendMessageToClient.
4) ReadyToCommitNavigation is called, which only then creates the pending FrameHostHolder
5) DidFinishNavigation is called which sets current_ to pending_.

This manifested in hangs with telemetry, since in cross-process navigations the Page.navigate command was being recieved incorrectly in the first renderer and it was sending a frameId from that renderer. The catapult code was waiting for a navigation event for that frameId which would never come (it would get it from the new process instead).

There were two bugs here with PlzNavigate, compared to the non-PlzNavigate case.
1 The first FrameHostHolder (i.e. current_ during the navigation) was not being paused. This was leading to responses to messages that were sent to the first renderer to respond to the inspector client. Since the response arrived, sent_messages_ was being cleared and we never got to send the message to the new renderer.
2) Even with the above fixed, we had to keep track of messages that were sent to the first renderer while the FrameHostHolder is (now) suspended. This didn't need to happen in the non-PlzNavigate case because the pending FrameHostHolder is created immediately (in the equivalent to step 2 above). At that point in time, SetPending ends up calling Reattach which then grabs the sent_messages_ from the current_ FrameHostHolder. The Page.navigate message was therefore being sent correctly to the second process. When the first process replies, it's ignored since it goes into pending_messages_ buffer. With PlzNavigate, there's a period of time between 2) and 4) where we know we have a navigation but we don't have the RenderFrameHost yet. So we need to emulate the non-PlzNavigate behavior by holding on to sent messages during that window, so that we can send them to the new FrameHostHolder once it commits.

This fixes telemetry_unittests' testWebPageReplay, gpu_process_launch_tests, hardware_accelerated_feature_tests and the page cyclers with PlzNavigate.

BUG= 674730 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2585053002
Cr-Commit-Position: refs/heads/master@{#439592}

[modify] https://crrev.com/af4c3f9c6ca41f00427a0f1f8de02782ce227629/content/browser/devtools/render_frame_devtools_agent_host.cc

Comment 3 by jam@chromium.org, Dec 19 2016

The issues should now be fixed (i.e. see this try run: https://codereview.chromium.org/2585053002/#ps180001)

Comment 4 by jam@chromium.org, Dec 19 2016

Status: Fixed (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 3 2017

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

commit 6d3cce3b1fbf2b1ea96e8cadab8a71a4875b1357
Author: jam <jam@chromium.org>
Date: Tue Jan 03 18:12:54 2017

Fix crash in AncestorThrottle that was causing a crash in telemetry perf unittests.

The test below started failing after 440055. There was a null dereference in AncestorThrottle for resources that didn't render (i.e. 204s) and so there was no RenderFrameHost being created.

This fixes
telemetry_perf_unittests's benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_desktop.load:media:9gag
with PlzNavigate.

BUG= 674730 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2606933002
Cr-Commit-Position: refs/heads/master@{#441156}

[modify] https://crrev.com/6d3cce3b1fbf2b1ea96e8cadab8a71a4875b1357/content/browser/frame_host/ancestor_throttle.cc

Comment 6 by laforge@google.com, Nov 7 2017

Components: Internals>Network>Service

Comment 7 by laforge@google.com, Nov 7 2017

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.

Sign in to add a comment