|
|
Created:
3 years, 10 months ago by jam Modified:
3 years, 9 months ago 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. |
DescriptionFix 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 #Messages
Total messages: 42 (27 generated)
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jam@chromium.org changed reviewers: + dgozman@chromium.org, skyostil@chromium.org
Dmitry: please take a look at content Sami: please take a look at headless
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:40001) has been deleted
Thanks for digging into these failures! https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... headless/lib/embedder_mojo_browsertest.cc:231: devtools_client_->GetNetwork()->Enable(run_loop.QuitClosure()); nit: It might make sense to add a helper method to do this to cut down on the repetition. https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), This part makes me a little nervous: if we can't rely on waiting for loads to complete just by looking at DevTools events, does that mean that headless users (who can only used DevTools) can't do this reliably either? I wonder if there's a way to make the messages related to this get deterministically delivered instead?
https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... 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 might make sense to add a helper method to do this to cut down on the > repetition. The issue is that the module that Enable is called on differs (i.e. GetNetwork, GetPage), so the code sharing would have to be an object that stores state and is called a few times, all to save a few lines. https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), On 2017/02/27 11:46:19, Sami wrote: > This part makes me a little nervous: if we can't rely on waiting for loads to > complete just by looking at DevTools events, does that mean that headless users > (who can only used DevTools) can't do this reliably either? I wonder if there's > a way to make the messages related to this get deterministically delivered > instead? The main difference with PlzNavigate, due to the buffering of devtools IPCs in RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered before enabling network module won't generate events (and similarly for other devtools messages). The solution for headless consumers is that they have to wait synchronously for module initialization like I did in the other devtools tests, if they care about devtools messages. Per Dmitry, this buffering could go away once the speculative RenderFrameHost in PlzNavigate is removed, but that is not happening before PlzNavigate launches, which is why I don't see any other solution.
https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... 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 11:46:19, Sami wrote: > > nit: It might make sense to add a helper method to do this to cut down on the > > repetition. > > The issue is that the module that Enable is called on differs (i.e. GetNetwork, > GetPage), so the code sharing would have to be an object that stores state and > is called a few times, all to save a few lines. Ah, I was just thinking of two helpers like EnablePageDomainSynchronously() and EnableNetworkDomainSynchronously(). WDYT? https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), On 2017/02/27 15:18:18, jam wrote: > On 2017/02/27 11:46:19, Sami wrote: > > This part makes me a little nervous: if we can't rely on waiting for loads to > > complete just by looking at DevTools events, does that mean that headless > users > > (who can only used DevTools) can't do this reliably either? I wonder if > there's > > a way to make the messages related to this get deterministically delivered > > instead? > > The main difference with PlzNavigate, due to the buffering of devtools IPCs in > RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered before > enabling network module won't generate events (and similarly for other devtools > messages). The solution for headless consumers is that they have to wait > synchronously for module initialization like I did in the other devtools tests, > if they care about devtools messages. Per Dmitry, this buffering could go away > once the speculative RenderFrameHost in PlzNavigate is removed, but that is not > happening before PlzNavigate launches, which is why I don't see any other > solution. I see, thanks for the explanation. I wonder if there's still a fundamental race condition here since these tests are mostly trying to catch the initial navigation of the WebContents (i.e., not an explicit Page.navigate) which could in theory happen way before we get around to attaching the DevTools client (in order to enable the network domain). Is there's any way around that -- or do we need to do something like specifying the list of domains to enable when creating the WebContents?
https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... 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 15:18:18, jam wrote: > > On 2017/02/27 11:46:19, Sami wrote: > > > nit: It might make sense to add a helper method to do this to cut down on > the > > > repetition. > > > > The issue is that the module that Enable is called on differs (i.e. > GetNetwork, > > GetPage), so the code sharing would have to be an object that stores state and > > is called a few times, all to save a few lines. > > Ah, I was just thinking of two helpers like EnablePageDomainSynchronously() and > EnableNetworkDomainSynchronously(). WDYT? The issue is we'd need more as other test use other devtools modules. IMO I didn't think it's necessary to have this helper; if you disagree and want it please let me know and I'll add it. https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), On 2017/02/27 16:22:53, Sami wrote: > On 2017/02/27 15:18:18, jam wrote: > > On 2017/02/27 11:46:19, Sami wrote: > > > This part makes me a little nervous: if we can't rely on waiting for loads > to > > > complete just by looking at DevTools events, does that mean that headless > > users > > > (who can only used DevTools) can't do this reliably either? I wonder if > > there's > > > a way to make the messages related to this get deterministically delivered > > > instead? > > > > The main difference with PlzNavigate, due to the buffering of devtools IPCs in > > RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered before > > enabling network module won't generate events (and similarly for other > devtools > > messages). The solution for headless consumers is that they have to wait > > synchronously for module initialization like I did in the other devtools > tests, > > if they care about devtools messages. Per Dmitry, this buffering could go away > > once the speculative RenderFrameHost in PlzNavigate is removed, but that is > not > > happening before PlzNavigate launches, which is why I don't see any other > > solution. > > I see, thanks for the explanation. I wonder if there's still a fundamental race > condition here since these tests are mostly trying to catch the initial > navigation of the WebContents (i.e., not an explicit Page.navigate) which could > in theory happen way before we get around to attaching the DevTools client (in > order to enable the network domain). Is there's any way around that -- or do we > need to do something like specifying the list of domains to enable when creating > the WebContents? I'm not really sure I understand this comment, sorry do you have specific steps? Before the change in this function, there was a lot of flakiness with PlzNavigate enabled. Afterwards I didn't see any after a lot of runs. So I'm inclined to believe there are no race conditions because this change was made after a lot of tracing to ensure that IPCs are received in the right order.
https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... File headless/lib/embedder_mojo_browsertest.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/lib/embedder_m... 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 16:22:53, Sami wrote: > > On 2017/02/27 15:18:18, jam wrote: > > > On 2017/02/27 11:46:19, Sami wrote: > > > > nit: It might make sense to add a helper method to do this to cut down on > > the > > > > repetition. > > > > > > The issue is that the module that Enable is called on differs (i.e. > > GetNetwork, > > > GetPage), so the code sharing would have to be an object that stores state > and > > > is called a few times, all to save a few lines. > > > > Ah, I was just thinking of two helpers like EnablePageDomainSynchronously() > and > > EnableNetworkDomainSynchronously(). WDYT? > > The issue is we'd need more as other test use other devtools modules. > > IMO I didn't think it's necessary to have this helper; if you disagree and want > it please let me know and I'll add it. Fair enough, it was just a small nit. We'll rethink this if it starts to become annoying. https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), On 2017/02/27 16:29:43, jam wrote: > On 2017/02/27 16:22:53, Sami wrote: > > On 2017/02/27 15:18:18, jam wrote: > > > On 2017/02/27 11:46:19, Sami wrote: > > > > This part makes me a little nervous: if we can't rely on waiting for loads > > to > > > > complete just by looking at DevTools events, does that mean that headless > > > users > > > > (who can only used DevTools) can't do this reliably either? I wonder if > > > there's > > > > a way to make the messages related to this get deterministically delivered > > > > instead? > > > > > > The main difference with PlzNavigate, due to the buffering of devtools IPCs > in > > > RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered > before > > > enabling network module won't generate events (and similarly for other > > devtools > > > messages). The solution for headless consumers is that they have to wait > > > synchronously for module initialization like I did in the other devtools > > tests, > > > if they care about devtools messages. Per Dmitry, this buffering could go > away > > > once the speculative RenderFrameHost in PlzNavigate is removed, but that is > > not > > > happening before PlzNavigate launches, which is why I don't see any other > > > solution. > > > > I see, thanks for the explanation. I wonder if there's still a fundamental > race > > condition here since these tests are mostly trying to catch the initial > > navigation of the WebContents (i.e., not an explicit Page.navigate) which > could > > in theory happen way before we get around to attaching the DevTools client (in > > order to enable the network domain). Is there's any way around that -- or do > we > > need to do something like specifying the list of domains to enable when > creating > > the WebContents? > > I'm not really sure I understand this comment, sorry do you have specific steps? > > > Before the change in this function, there was a lot of flakiness with > PlzNavigate enabled. Afterwards I didn't see any after a lot of runs. So I'm > inclined to believe there are no race conditions because this change was made > after a lot of tracing to ensure that IPCs are received in the right order. Sorry for being vague: I fully trust this specific change works. I'm just wondering what to tell headless folks who can't use content::TestNavigationObserver. Here's the scenario I'm thinking of: 1. Create a headless WebContents, giving it an initial URL to navigate to. 2. Attach a DevTools client to the WebContents. 3. Enable the Page domain. 4. Wait for a load event that never comes, because the navigation already happened before step 3 (or really, step 2)? The chicken/egg-situation being that you can't enable the Page domain before you have a WebContents, which navigates as soon as it's constructed. Please tell me if I'm missing something :)
https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... 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 2017/02/27 16:29:43, jam wrote: > > On 2017/02/27 16:22:53, Sami wrote: > > > On 2017/02/27 15:18:18, jam wrote: > > > > On 2017/02/27 11:46:19, Sami wrote: > > > > > This part makes me a little nervous: if we can't rely on waiting for > loads > > > to > > > > > complete just by looking at DevTools events, does that mean that > headless > > > > users > > > > > (who can only used DevTools) can't do this reliably either? I wonder if > > > > there's > > > > > a way to make the messages related to this get deterministically > delivered > > > > > instead? > > > > > > > > The main difference with PlzNavigate, due to the buffering of devtools > IPCs > > in > > > > RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered > > before > > > > enabling network module won't generate events (and similarly for other > > > devtools > > > > messages). The solution for headless consumers is that they have to wait > > > > synchronously for module initialization like I did in the other devtools > > > tests, > > > > if they care about devtools messages. Per Dmitry, this buffering could go > > away > > > > once the speculative RenderFrameHost in PlzNavigate is removed, but that > is > > > not > > > > happening before PlzNavigate launches, which is why I don't see any other > > > > solution. > > > > > > I see, thanks for the explanation. I wonder if there's still a fundamental > > race > > > condition here since these tests are mostly trying to catch the initial > > > navigation of the WebContents (i.e., not an explicit Page.navigate) which > > could > > > in theory happen way before we get around to attaching the DevTools client > (in > > > order to enable the network domain). Is there's any way around that -- or do > > we > > > need to do something like specifying the list of domains to enable when > > creating > > > the WebContents? > > > > I'm not really sure I understand this comment, sorry do you have specific > steps? > > > > > > Before the change in this function, there was a lot of flakiness with > > PlzNavigate enabled. Afterwards I didn't see any after a lot of runs. So I'm > > inclined to believe there are no race conditions because this change was made > > after a lot of tracing to ensure that IPCs are received in the right order. > > Sorry for being vague: I fully trust this specific change works. I'm just > wondering what to tell headless folks who can't use > content::TestNavigationObserver. Here's the scenario I'm thinking of: > > 1. Create a headless WebContents, giving it an initial URL to navigate to. > 2. Attach a DevTools client to the WebContents. > 3. Enable the Page domain. > 4. Wait for a load event that never comes, because the navigation already > happened before step 3 (or really, step 2)? > > The chicken/egg-situation being that you can't enable the Page domain before you > have a WebContents, which navigates as soon as it's constructed. Please tell me > if I'm missing something :) ah, got it, thanks. An example of this scenario is my changes to embedder_mojo_browsertest.cc in this cl. It's not possible, with the current devtools code, to navigate a page and then wait for devtools messages. The solution is to have a WebContents start with the default about:blank, and then after enabling a devtools module that they're interested in, navigate.
https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... 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 2017/02/27 16:36:39, Sami wrote: > > On 2017/02/27 16:29:43, jam wrote: > > > On 2017/02/27 16:22:53, Sami wrote: > > > > On 2017/02/27 15:18:18, jam wrote: > > > > > On 2017/02/27 11:46:19, Sami wrote: > > > > > > This part makes me a little nervous: if we can't rely on waiting for > > loads > > > > to > > > > > > complete just by looking at DevTools events, does that mean that > > headless > > > > > users > > > > > > (who can only used DevTools) can't do this reliably either? I wonder > if > > > > > there's > > > > > > a way to make the messages related to this get deterministically > > delivered > > > > > > instead? > > > > > > > > > > The main difference with PlzNavigate, due to the buffering of devtools > > IPCs > > > in > > > > > RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered > > > before > > > > > enabling network module won't generate events (and similarly for other > > > > devtools > > > > > messages). The solution for headless consumers is that they have to wait > > > > > synchronously for module initialization like I did in the other devtools > > > > tests, > > > > > if they care about devtools messages. Per Dmitry, this buffering could > go > > > away > > > > > once the speculative RenderFrameHost in PlzNavigate is removed, but that > > is > > > > not > > > > > happening before PlzNavigate launches, which is why I don't see any > other > > > > > solution. > > > > > > > > I see, thanks for the explanation. I wonder if there's still a fundamental > > > race > > > > condition here since these tests are mostly trying to catch the initial > > > > navigation of the WebContents (i.e., not an explicit Page.navigate) which > > > could > > > > in theory happen way before we get around to attaching the DevTools client > > (in > > > > order to enable the network domain). Is there's any way around that -- or > do > > > we > > > > need to do something like specifying the list of domains to enable when > > > creating > > > > the WebContents? > > > > > > I'm not really sure I understand this comment, sorry do you have specific > > steps? > > > > > > > > > Before the change in this function, there was a lot of flakiness with > > > PlzNavigate enabled. Afterwards I didn't see any after a lot of runs. So I'm > > > inclined to believe there are no race conditions because this change was > made > > > after a lot of tracing to ensure that IPCs are received in the right order. > > > > Sorry for being vague: I fully trust this specific change works. I'm just > > wondering what to tell headless folks who can't use > > content::TestNavigationObserver. Here's the scenario I'm thinking of: > > > > 1. Create a headless WebContents, giving it an initial URL to navigate to. > > 2. Attach a DevTools client to the WebContents. > > 3. Enable the Page domain. > > 4. Wait for a load event that never comes, because the navigation already > > happened before step 3 (or really, step 2)? > > > > The chicken/egg-situation being that you can't enable the Page domain before > you > > have a WebContents, which navigates as soon as it's constructed. Please tell > me > > if I'm missing something :) > > ah, got it, thanks. > > An example of this scenario is my changes to embedder_mojo_browsertest.cc in > this cl. It's not possible, with the current devtools code, to navigate a page > and then wait for devtools messages. The solution is to have a WebContents start > with the default about:blank, and then after enabling a devtools module that > they're interested in, navigate. > oh, and if course, if all a user of headless cares about is waiting for navigation to complete, you can always expose a version of WaitForLoad to non-test code.
On 2017/02/27 16:52:57, jam wrote: > https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... > File headless/test/headless_browser_test.cc (right): > > https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... > 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 2017/02/27 16:29:43, jam wrote: > > > On 2017/02/27 16:22:53, Sami wrote: > > > > On 2017/02/27 15:18:18, jam wrote: > > > > > On 2017/02/27 11:46:19, Sami wrote: > > > > > > This part makes me a little nervous: if we can't rely on waiting for > > loads > > > > to > > > > > > complete just by looking at DevTools events, does that mean that > > headless > > > > > users > > > > > > (who can only used DevTools) can't do this reliably either? I wonder > if > > > > > there's > > > > > > a way to make the messages related to this get deterministically > > delivered > > > > > > instead? > > > > > > > > > > The main difference with PlzNavigate, due to the buffering of devtools > > IPCs > > > in > > > > > RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered > > > before > > > > > enabling network module won't generate events (and similarly for other > > > > devtools > > > > > messages). The solution for headless consumers is that they have to wait > > > > > synchronously for module initialization like I did in the other devtools > > > > tests, > > > > > if they care about devtools messages. Per Dmitry, this buffering could > go > > > away > > > > > once the speculative RenderFrameHost in PlzNavigate is removed, but that > > is > > > > not > > > > > happening before PlzNavigate launches, which is why I don't see any > other > > > > > solution. > > > > > > > > I see, thanks for the explanation. I wonder if there's still a fundamental > > > race > > > > condition here since these tests are mostly trying to catch the initial > > > > navigation of the WebContents (i.e., not an explicit Page.navigate) which > > > could > > > > in theory happen way before we get around to attaching the DevTools client > > (in > > > > order to enable the network domain). Is there's any way around that -- or > do > > > we > > > > need to do something like specifying the list of domains to enable when > > > creating > > > > the WebContents? > > > > > > I'm not really sure I understand this comment, sorry do you have specific > > steps? > > > > > > > > > Before the change in this function, there was a lot of flakiness with > > > PlzNavigate enabled. Afterwards I didn't see any after a lot of runs. So I'm > > > inclined to believe there are no race conditions because this change was > made > > > after a lot of tracing to ensure that IPCs are received in the right order. > > > > Sorry for being vague: I fully trust this specific change works. I'm just > > wondering what to tell headless folks who can't use > > content::TestNavigationObserver. Here's the scenario I'm thinking of: > > > > 1. Create a headless WebContents, giving it an initial URL to navigate to. > > 2. Attach a DevTools client to the WebContents. > > 3. Enable the Page domain. > > 4. Wait for a load event that never comes, because the navigation already > > happened before step 3 (or really, step 2)? > > > > The chicken/egg-situation being that you can't enable the Page domain before > you > > have a WebContents, which navigates as soon as it's constructed. Please tell > me > > if I'm missing something :) > > ah, got it, thanks. > > An example of this scenario is my changes to embedder_mojo_browsertest.cc in > this cl. It's not possible, with the current devtools code, to navigate a page > and then wait for devtools messages. The solution is to have a WebContents start > with the default about:blank, and then after enabling a devtools module that > they're interested in, navigate. This is what telemetry and WebDriver do in a similar case.
https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/network_handler.cc:679: frontend_->LoadingFailed( We should send "requestWillBeSent" before "loadingFailed" with the same request id for protocol events to be reasonably paired. https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:745: std::string request_id = base::IntToString(base::GetCurrentProcId()) + "." + I'd move protocol-specific code (like request id generation or error message) to NetworkHandler instead.
lgtm. https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... File headless/test/headless_browser_test.cc (right): https://codereview.chromium.org/2720543002/diff/60001/headless/test/headless_... headless/test/headless_browser_test.cc:164: content::TestNavigationObserver observer(web_contents_impl->web_contents(), On 2017/02/27 16:53:45, jam wrote: > On 2017/02/27 16:52:57, jam wrote: > > On 2017/02/27 16:36:39, Sami wrote: > > > On 2017/02/27 16:29:43, jam wrote: > > > > On 2017/02/27 16:22:53, Sami wrote: > > > > > On 2017/02/27 15:18:18, jam wrote: > > > > > > On 2017/02/27 11:46:19, Sami wrote: > > > > > > > This part makes me a little nervous: if we can't rely on waiting for > > > loads > > > > > to > > > > > > > complete just by looking at DevTools events, does that mean that > > > headless > > > > > > users > > > > > > > (who can only used DevTools) can't do this reliably either? I wonder > > if > > > > > > there's > > > > > > > a way to make the messages related to this get deterministically > > > delivered > > > > > > > instead? > > > > > > > > > > > > The main difference with PlzNavigate, due to the buffering of devtools > > > IPCs > > > > in > > > > > > RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered > > > > before > > > > > > enabling network module won't generate events (and similarly for other > > > > > devtools > > > > > > messages). The solution for headless consumers is that they have to > wait > > > > > > synchronously for module initialization like I did in the other > devtools > > > > > tests, > > > > > > if they care about devtools messages. Per Dmitry, this buffering could > > go > > > > away > > > > > > once the speculative RenderFrameHost in PlzNavigate is removed, but > that > > > is > > > > > not > > > > > > happening before PlzNavigate launches, which is why I don't see any > > other > > > > > > solution. > > > > > > > > > > I see, thanks for the explanation. I wonder if there's still a > fundamental > > > > race > > > > > condition here since these tests are mostly trying to catch the initial > > > > > navigation of the WebContents (i.e., not an explicit Page.navigate) > which > > > > could > > > > > in theory happen way before we get around to attaching the DevTools > client > > > (in > > > > > order to enable the network domain). Is there's any way around that -- > or > > do > > > > we > > > > > need to do something like specifying the list of domains to enable when > > > > creating > > > > > the WebContents? > > > > > > > > I'm not really sure I understand this comment, sorry do you have specific > > > steps? > > > > > > > > > > > > Before the change in this function, there was a lot of flakiness with > > > > PlzNavigate enabled. Afterwards I didn't see any after a lot of runs. So > I'm > > > > inclined to believe there are no race conditions because this change was > > made > > > > after a lot of tracing to ensure that IPCs are received in the right > order. > > > > > > Sorry for being vague: I fully trust this specific change works. I'm just > > > wondering what to tell headless folks who can't use > > > content::TestNavigationObserver. Here's the scenario I'm thinking of: > > > > > > 1. Create a headless WebContents, giving it an initial URL to navigate to. > > > 2. Attach a DevTools client to the WebContents. > > > 3. Enable the Page domain. > > > 4. Wait for a load event that never comes, because the navigation already > > > happened before step 3 (or really, step 2)? > > > > > > The chicken/egg-situation being that you can't enable the Page domain before > > you > > > have a WebContents, which navigates as soon as it's constructed. Please tell > > me > > > if I'm missing something :) > > > > ah, got it, thanks. > > > > An example of this scenario is my changes to embedder_mojo_browsertest.cc in > > this cl. It's not possible, with the current devtools code, to navigate a page > > and then wait for devtools messages. The solution is to have a WebContents > start > > with the default about:blank, and then after enabling a devtools module that > > they're interested in, navigate. > > > > oh, and if course, if all a user of headless cares about is waiting for > navigation to complete, you can always expose a version of WaitForLoad to > non-test code. Alright, thanks for the explanation. The intermediate hop to about:blank is a little unfortunate, but maybe we can't avoid it without exposing something new.
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtool... File content/browser/devtools/protocol/network_handler.cc (right): https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtool... content/browser/devtools/protocol/network_handler.cc:679: frontend_->LoadingFailed( On 2017/02/27 17:31:31, dgozman wrote: > We should send "requestWillBeSent" before "loadingFailed" with the same request > id for protocol events to be reasonably paired. Done. https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtool... File content/browser/devtools/render_frame_devtools_agent_host.cc (right): https://codereview.chromium.org/2720543002/diff/60001/content/browser/devtool... content/browser/devtools/render_frame_devtools_agent_host.cc:745: std::string request_id = base::IntToString(base::GetCurrentProcId()) + "." + On 2017/02/27 17:31:31, dgozman wrote: > I'd move protocol-specific code (like request id generation or error message) to > NetworkHandler instead. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thank you!
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2720543002/#ps100001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488236099443270, "parent_rev": "89990679c0ab95d19bdf182b1d90f2f5d752379b", "commit_rev": "6be7f531cadb6afd9d953ffae9a0572de0ee766f"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/6be7f531cadb6afd9d953ffae9a0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/6be7f531cadb6afd9d953ffae9a0... |