Issue metadata
Sign in to add a comment
|
Fix telemetry tests with PlzNavigate |
||||||||||||||||||||||
Issue descriptionThere 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
,
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
,
Dec 19 2016
The issues should now be fixed (i.e. see this try run: https://codereview.chromium.org/2585053002/#ps180001)
,
Dec 19 2016
,
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
,
Nov 7 2017
,
Nov 7 2017
Apologies, applied the wrong component in bulk. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Dec 16 2016