Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1012)

Issue 2720543002: Fix headless_browsertests failures with PlzNavigate. (Closed)

Created:
3 years, 10 months ago by jam
Modified:
3 years, 9 months ago
Reviewers:
dgozman, Sami
CC:
chromium-reviews, creis+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, nasko+codewatch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, devtools-reviews_chromium.org, darin (slow to review), pfeldman
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix headless_browsertests failures with PlzNavigate. The fixes are: -fire Network.loadingFailed event when a navigation fails. Since there's no ResourceLoader in the renderer for failed navigation requests, we have to fire this from the browser. -synchronously wait for confirmation that the renderer enabled network/page modules before navigating pages, otherwise the observers miss some devtools messages flakily. With PlzNavigate enabled, RenderFrameDevToolsAgentHost buffers devtools messages because of speculative RFHs. So when a WebContents is created, we have to wait for the reply for enabling a module which depends on the initial about:blank navigation committing. -change HeadlessBrowserTest::WaitForLoad to use content::TestNavigationObserver instead of depending on devtools messages, as those could be missed if they were fired before enabling the network module BUG=504347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2720543002 Cr-Commit-Position: refs/heads/master@{#453374} Committed: https://chromium.googlesource.com/chromium/src/+/6be7f531cadb6afd9d953ffae9a0572de0ee766f

Patch Set 1 : with PlzNavigate #

Patch Set 2 : without PlzNavigate #

Total comments: 17

Patch Set 3 : review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+153 lines, -18 lines) Patch
M content/browser/devtools/protocol/network_handler.h View 1 2 2 chunks +6 lines, -0 lines 0 comments Download
M content/browser/devtools/protocol/network_handler.cc View 1 2 4 chunks +50 lines, -0 lines 0 comments Download
M content/browser/devtools/render_frame_devtools_agent_host.h View 1 2 4 chunks +10 lines, -0 lines 0 comments Download
M content/browser/devtools/render_frame_devtools_agent_host.cc View 1 2 2 chunks +27 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_request.cc View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M headless/lib/embedder_mojo_browsertest.cc View 2 chunks +10 lines, -5 lines 0 comments Download
M headless/lib/headless_browser_context_browsertest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M headless/lib/headless_devtools_client_browsertest.cc View 8 chunks +33 lines, -7 lines 0 comments Download
M headless/test/headless_browser_test.cc View 2 chunks +8 lines, -3 lines 0 comments Download

Messages

Total messages: 42 (27 generated)
jam
Dmitry: please take a look at content Sami: please take a look at headless
3 years, 9 months ago (2017-02-26 21:25:51 UTC) #14
Sami
Thanks for digging into these failures! https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc#newcode231 headless/lib/embedder_mojo_browsertest.cc:231: devtools_client_->GetNetwork()->Enable(run_loop.QuitClosure()); nit: It ...
3 years, 9 months ago (2017-02-27 11:46:19 UTC) #18
jam
https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc#newcode231 headless/lib/embedder_mojo_browsertest.cc:231: devtools_client_->GetNetwork()->Enable(run_loop.QuitClosure()); On 2017/02/27 11:46:19, Sami wrote: > nit: It ...
3 years, 9 months ago (2017-02-27 15:18:18 UTC) #19
Sami
https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc#newcode231 headless/lib/embedder_mojo_browsertest.cc:231: devtools_client_->GetNetwork()->Enable(run_loop.QuitClosure()); On 2017/02/27 15:18:18, jam wrote: > On 2017/02/27 ...
3 years, 9 months ago (2017-02-27 16:22:53 UTC) #20
jam
https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc#newcode231 headless/lib/embedder_mojo_browsertest.cc:231: devtools_client_->GetNetwork()->Enable(run_loop.QuitClosure()); On 2017/02/27 16:22:53, Sami wrote: > On 2017/02/27 ...
3 years, 9 months ago (2017-02-27 16:29:43 UTC) #21
Sami
https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_mojo_browsertest.cc#newcode231 headless/lib/embedder_mojo_browsertest.cc:231: devtools_client_->GetNetwork()->Enable(run_loop.QuitClosure()); On 2017/02/27 16:29:43, jam wrote: > On 2017/02/27 ...
3 years, 9 months ago (2017-02-27 16:36:40 UTC) #22
jam
https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_browser_test.cc File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_browser_test.cc#newcode164 headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), On 2017/02/27 16:36:39, Sami wrote: > On ...
3 years, 9 months ago (2017-02-27 16:52:57 UTC) #23
jam
https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_browser_test.cc File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_browser_test.cc#newcode164 headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), On 2017/02/27 16:52:57, jam wrote: > On ...
3 years, 9 months ago (2017-02-27 16:53:45 UTC) #24
dgozman
On 2017/02/27 16:52:57, jam wrote: > https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_browser_test.cc > File headless/test/headless_browser_test.cc (right): > > https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_browser_test.cc#newcode164 > ...
3 years, 9 months ago (2017-02-27 17:23:40 UTC) #25
dgozman
https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtools/protocol/network_handler.cc File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtools/protocol/network_handler.cc#newcode679 content/browser/devtools/protocol/network_handler.cc:679: frontend_->LoadingFailed( We should send "requestWillBeSent" before "loadingFailed" with the ...
3 years, 9 months ago (2017-02-27 17:31:31 UTC) #26
Sami
lgtm. https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_browser_test.cc File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_browser_test.cc#newcode164 headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), On 2017/02/27 16:53:45, jam wrote: > ...
3 years, 9 months ago (2017-02-27 17:33:02 UTC) #27
jam
https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtools/protocol/network_handler.cc File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtools/protocol/network_handler.cc#newcode679 content/browser/devtools/protocol/network_handler.cc:679: frontend_->LoadingFailed( On 2017/02/27 17:31:31, dgozman wrote: > We should ...
3 years, 9 months ago (2017-02-27 20:47:35 UTC) #33
dgozman
lgtm, thank you!
3 years, 9 months ago (2017-02-27 22:49:12 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2720543002/100001
3 years, 9 months ago (2017-02-27 22:55:41 UTC) #39
commit-bot: I haz the power
3 years, 9 months ago (2017-02-27 23:06:31 UTC) #42
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6be7f531cadb6afd9d953ffae9a0...

Powered by Google App Engine
This is Rietveld 408576698