Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2288203002:
Redo PasswordManagerBrowserTestBase.InternalsPage (Closed)
|
Created:
4 years, 3 months ago by vabr (Chromium) Modified:
4 years, 3 months ago Reviewers:
vasilii CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRedo PasswordManagerBrowserTestBase.InternalsPage
The old version of the test was flaky after
https://codereview.chromium.org/2270333002/ switched some messages to Mojo,
because the signal that "internals page is ready for logging" propagated into
the renderer after the renderer tried to log.
The issue is likely Mojo-specific. It did not occur without the switch to
Mojo. After the switch to Mojo, signals about navigation events no longer go
through the same channel as the autofill messages (including the
logging-related). Therefore the test code could have observed the internals
page finish loading and consequently start loading the page with forms
without all Mojo messages being properly transferred.
The issue was test-specific, because the test was opening the page with forms
much quicker after opening the internals page, than a human trying to capture
the logs could do.
The issue was Mac-specific, but not sure why. It does not seem important,
though.
Therefore, this CL fixes the test in adding an additional reload of the form
page. That is, instead of:
1. Loading the internals page
2. Loading the page with forms
the test now does
1'. Loading the internals page
2'. Loading the page with forms
3'. Reloading the page with forms
The reloading increases the chance of the Mojo messages about logging
activation to arrive to the renderer, simply because reloading takes longer.
IIUC, there is no good event to wait for to be sure that the Mojo messages
were received (without modifying the production code).
The CL also splits the test into two, because it checked for two different\
things in the logs (browser vs. renderer messages).
BUG=640737
R=vasilii@chromium.org
Committed: https://crrev.com/e78084414756fa83c67cb9a50f5693fb3bcef68d
Cr-Commit-Position: refs/heads/master@{#416540}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Typo #Patch Set 3 : Reload page with forms #Patch Set 4 : Fix compilation #
Total comments: 2
Patch Set 5 : Just rebased #Patch Set 6 : Just rebased #Patch Set 7 : Inline the helper method, update comments #
Total comments: 1
Messages
Total messages: 41 (21 generated)
The CQ bit was checked by vabr@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.
vabr@chromium.org changed reviewers: + vasilii@chromium.org
Hi Vasilii, PTAL. Thanks! Vaclav
Can you clarify which events from the renderer trigger the text (both browser and renderer) you are waiting for? https://codereview.chromium.org/2288203002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2288203002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_manager_browsertest.cc:2794: // Check that the internals page contains logs both from the browser. logs from the browser.
(I'll address the code comment once I'm at a computer with a checkout.) On 2016/08/29 13:56:07, vasilii wrote: > Can you clarify which events from the renderer trigger the text (both browser > and renderer) you are waiting for? The tests want to hear a log saying it comes from PasswordAutofillAgent (for renderer) or PasswordManager (for browser). The first events satisfying it currently are PasswordAutofillAgent::SendPasswordForms and the related PasswordManager::CreatePendingLoginManagers. In other words currently the test waits for page parsing. Do you want me to state that in the browsertest itself? Cheers, Vaclav
But page parsing happens before NavigateToURL returns. How can the test be flaky in its current implementation? I wish to learn more about Mojo.
On 2016/08/29 15:19:43, vasilii wrote: > But page parsing happens before NavigateToURL returns. How can the test be flaky > in its current implementation? The logs make a longer trip: source renderer -> browser -> renderer with the internals page. The last step might be just what makes the difference. In other words, my previous answer was not precise. The test should not wait just until the browser hears about the page parsing, but also until the renderer with the internals page gets the message from the browser and manages to run its JavaScript to append it to its own HTML. > I wish to learn more about Mojo. Just to clarify -- the test has been flaky before the corresponding AutofillHostMsg_RecordSavePasswordProgress has been converted to Mojo (it has not been converted yet).
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
https://codereview.chromium.org/2288203002/diff/1/chrome/browser/password_man... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2288203002/diff/1/chrome/browser/password_man... chrome/browser/password_manager/password_manager_browsertest.cc:2794: // Check that the internals page contains logs both from the browser. On 2016/08/29 13:56:07, vasilii wrote: > logs from the browser. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/29 15:37:48, vabr (Chromium) wrote: > On 2016/08/29 15:19:43, vasilii wrote: > > But page parsing happens before NavigateToURL returns. How can the test be > flaky > > in its current implementation? > > The logs make a longer trip: source renderer -> browser -> renderer with the > internals page. The last step might be just what makes the difference. > In other words, my previous answer was not precise. The test should not wait > just until the browser hears about the page parsing, but also until the renderer > with the internals page gets the message from the browser and manages to run its > JavaScript to append it to its own HTML. > > > I wish to learn more about Mojo. > > Just to clarify -- the test has been flaky before the corresponding > AutofillHostMsg_RecordSavePasswordProgress has been converted to Mojo (it has > not been converted yet). Still unclear. The current implementation uses ExecuteScriptAndExtractBool for searching. The script should be executed after the append script for logs. Are you sure that the delay isn't caused by TestPasswordStore which lives on the UI thread but executes everything asynchronously?
On 2016/08/29 15:49:28, vasilii wrote: > On 2016/08/29 15:37:48, vabr (Chromium) wrote: > > On 2016/08/29 15:19:43, vasilii wrote: > > > But page parsing happens before NavigateToURL returns. How can the test be > > flaky > > > in its current implementation? > > > > The logs make a longer trip: source renderer -> browser -> renderer with the > > internals page. The last step might be just what makes the difference. > > In other words, my previous answer was not precise. The test should not wait > > just until the browser hears about the page parsing, but also until the > renderer > > with the internals page gets the message from the browser and manages to run > its > > JavaScript to append it to its own HTML. > > > > > I wish to learn more about Mojo. > > > > Just to clarify -- the test has been flaky before the corresponding > > AutofillHostMsg_RecordSavePasswordProgress has been converted to Mojo (it has > > not been converted yet). > > Still unclear. The current implementation uses ExecuteScriptAndExtractBool for > searching. The script should be executed after the append script for logs. > Are you sure that the delay isn't caused by TestPasswordStore which lives on the > UI thread but executes everything asynchronously? Thanks for helping me figure this out, Vasilii. I am not sure that I understand the delay, and you have a good point about ExecuteScriptAndExtractBool providing enough additional delay to see the message on the internals page already. With TestPasswordStore, I don't know. Logging from the renderer should not depend on it. It could delay filling passwords but unlikely reporting the parsed forms. Having said that, there is likely something test-specific, which breaks. For more context: The test flakes much more with logging passed through Mojo. While the Mojo change https://codereview.chromium.org/2270333002/ has not landed, the trybots show that. Also local running of the test is close to 100% failure rate (it still passes sometimes). When I observed this today, I realised that while in normal Chrome, the renderer messages show up with no problem, in browser tests they fail to show up completely (unlike browser logging, which works). Again, that's with Mojo, not with IPC, so might be an issue with the Mojo patch as well. I will look more into this tomorrow. I'm fine with postponing this CL until I learn more. Cheers, Vaclav
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/29 16:12:42, vabr (Chromium) wrote: > On 2016/08/29 15:49:28, vasilii wrote: > > On 2016/08/29 15:37:48, vabr (Chromium) wrote: > > > On 2016/08/29 15:19:43, vasilii wrote: > > > > But page parsing happens before NavigateToURL returns. How can the test be > > > flaky > > > > in its current implementation? > > > > > > The logs make a longer trip: source renderer -> browser -> renderer with the > > > internals page. The last step might be just what makes the difference. > > > In other words, my previous answer was not precise. The test should not wait > > > just until the browser hears about the page parsing, but also until the > > renderer > > > with the internals page gets the message from the browser and manages to run > > its > > > JavaScript to append it to its own HTML. > > > > > > > I wish to learn more about Mojo. > > > > > > Just to clarify -- the test has been flaky before the corresponding > > > AutofillHostMsg_RecordSavePasswordProgress has been converted to Mojo (it > has > > > not been converted yet). > > > > Still unclear. The current implementation uses ExecuteScriptAndExtractBool for > > searching. The script should be executed after the append script for logs. > > Are you sure that the delay isn't caused by TestPasswordStore which lives on > the > > UI thread but executes everything asynchronously? > > Thanks for helping me figure this out, Vasilii. I am not sure that I understand > the delay, and you have a good point about ExecuteScriptAndExtractBool providing > enough additional delay to see the message on the internals page already. > > With TestPasswordStore, I don't know. Logging from the renderer should not > depend on it. It could delay filling passwords but unlikely reporting the parsed > forms. > > Having said that, there is likely something test-specific, which breaks. For > more context: > The test flakes much more with logging passed through Mojo. While the Mojo > change https://codereview.chromium.org/2270333002/ has not landed, the trybots > show that. Also local running of the test is close to 100% failure rate (it > still passes sometimes). When I observed this today, I realised that while in > normal Chrome, the renderer messages show up with no problem, in browser tests > they fail to show up completely (unlike browser logging, which works). Again, > that's with Mojo, not with IPC, so might be an issue with the Mojo patch as > well. > > I will look more into this tomorrow. I'm fine with postponing this CL until I > learn more. > > Cheers, > Vaclav Curious to see what you will find :-)
The CQ bit was checked by vabr@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.
Description was changed from ========== Redo PasswordManagerBrowserTestBase.InternalsPage The old version of the test was flaky because of not waiting for the right event. The test wanted to check some output on the internals page, and waited only for finished navigation. There seems to be no well observable event to wait for execpt for the output indeed appearing on the page, so this CLs changes the test to wait for that. It also splits the test into two, because it checked for two different things in the logs. BUG=640737 ========== to ========== Redo PasswordManagerBrowserTestBase.InternalsPage The old version of the test was flaky after https://codereview.chromium.org/2270333002/ switched some messages to Mojo, because the signal that "internals page is ready for logging" propagated into the renderer after the renderer tried to log. The issue is likely Mojo-specific. It did not occur without the switch to Mojo. After the switch to Mojo, signals about navigation events no longer go through the same channel as the autofill messages (including the logging-related). Therefore the test code could have observed the internals page finish loading and consequently start loading the page with forms without all Mojo messages being properly transferred. The issue was test-specific, because the test was opening the page with forms much quicker after opening the internals page, than a human trying to capture the logs could do. The issue was Mac-specific, but not sure why. It does not seem important, though. Therefore, this CL fixes the test in adding an additional reload of the form page. That is, instead of: 1. Loading the internals page 2. Loading the page with forms the test now does 1'. Loading the internals page 2'. Loading the page with forms 3'. Reloading the page with forms The reloading increases the chance of the Mojo messages about logging activation to arrive to the renderer, simply because reloading takes longer. IIUC, there is no good event to wait for to be sure that the Mojo messages were received (without modifying the production code). The CL also splits the test into two, because it checked for two different\ things in the logs (browser vs. renderer messages). BUG=640737 R=vasilii@chromium.org ==========
Hi Vasilii, PTAL. I updated the CL description with my findings. Cheers, Vaclav
I looked at the code. PasswordManagerInternalsUI receives the DidStopLoading
event and registers itself in the LogRouter. This results in all the renderers
being sent AutofillMsg_SetLoggingState. Consequences:
- After NavigateToURLWithDisposition("chrome://password-manager-internals")
returns, the browser process already knows that logging is happening. Thus, no
workarounds are needed for InternalsPage_Browser.
- The renderer for password_form.html is notified in
ChromePasswordManagerClient::ChromePasswordManagerClient:161. If the message
comes too late to log anything useful then it's a real bug in the production
code. It can be probably reproduced by calling "chrome.exe
some_password_form_page.html" while chrome is already open and logging. If this
is the case then we shouldn't land a test with a hack tailored to hide real
problem.
https://codereview.chromium.org/2288203002/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):
https://codereview.chromium.org/2288203002/diff/60001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:168: // The
check is done by a repeatedly called JavaScript function, which navigates
obsolete
Thanks, Vasilii.
On 2016/08/30 14:09:48, vasilii wrote:
> I looked at the code. PasswordManagerInternalsUI receives the DidStopLoading
> event and registers itself in the LogRouter. This results in all the renderers
> being sent AutofillMsg_SetLoggingState. Consequences:
> - After NavigateToURLWithDisposition("chrome://password-manager-internals")
> returns, the browser process already knows that logging is happening. Thus, no
> workarounds are needed for InternalsPage_Browser.
Yes, the browser messages were never a problem. The workaround was not targeted
on
them, it was just a side effect of pulling out the otherwise common code.
> - The renderer for password_form.html is notified in
> ChromePasswordManagerClient::ChromePasswordManagerClient:161.
This notification only reaches frames which exist before the client is
constructed.
It is neither the case in the test I observed, nor in launching Chrome manually
in
a similar scenario. It could be the case when the owning WebContents is changed
for
an existing RenderFrameHost (e.g., pre-rendering or undocking a DevTools
window).
In the test, the chain is:
1. The renderer's PasswordAutofillAgent requests the logging state on its own
construction.
2. The ContentPasswordManagerDriver responds.
> If the message
> comes too late to log anything useful then it's a real bug in the production
> code. It can be probably reproduced by calling "chrome.exe
> some_password_form_page.html" while chrome is already open and logging. If
this
> is the case then we shouldn't land a test with a hack tailored to hide real
> problem.
Interestingly, launching chromium by hand as you suggest, the roundtrip of 1.
and 2. as described
above is fast enough that it arrives before the PasswordAutofillAgent tries to
log anything.
So the production code works fine.
In the test the response 2. is always too late, i.e., first the
PasswordAutofillAgent does the
request 1. on construction, then it tries to log and first then the response 2.
arrives.
I'm curious why there is such a reliable difference between the test and the
production code.
Will look further, but probably not today any more.
Once I know more, I'll update this.
Cheers,
Vaclav
>
>
https://codereview.chromium.org/2288203002/diff/60001/chrome/browser/password...
> File chrome/browser/password_manager/password_manager_browsertest.cc (right):
>
>
https://codereview.chromium.org/2288203002/diff/60001/chrome/browser/password...
> chrome/browser/password_manager/password_manager_browsertest.cc:168: // The
> check is done by a repeatedly called JavaScript function, which navigates
> obsolete
The CQ bit was checked by vabr@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...
Hi Vasilii, I looked into the issue today. TL;DR -- I don't think the production code is buggy. In addition, the test is best-effort, but unlikely to flake. Details: Observing the test on GNU/Linux and Mac, I saw that Mojo can take different amounts of time (relative to the test execution speed) to do the round-trip from the renderer: "Is logging active?" -> "Yes, it is." There is no contract with Mojo about the speed of such round-trips, so there is no error in Mojo. Password manager code also does not have a contract capping the delay between opening the internals page and logs coming in. Achieving that would only be possible by blocking the renderer code until the response is received. However, that would mean blocking the main purpose of the code (form filling) for a helper feature (logging), which does not sound like the right trade-off. So the absence of a cap on the logging delay is a conscious decision. To capture that, the test needs to be permissive: it should verify that the logging is available eventually, not necessarily in time for the first page load. To do this precisely, I should write the test that it keeps reloading the form page until it succeeds (or times out). If you prefer that, I'll do it, otherwise I favour going with the slightly easier current solution (patch set 7). Please let me know what you think about this. Cheers, Vaclav
I forgot to also publish the comment. The main thing is in the previous e-mail, though. https://codereview.chromium.org/2288203002/diff/60001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2288203002/diff/60001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:168: // The check is done by a repeatedly called JavaScript function, which navigates On 2016/08/30 14:09:48, vasilii wrote: > obsolete Thanks for spotting. I inlined the helper method, because, as you noted before, the reloading is not needed for the browser-logging test. While inlining, I also updated the comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Vasilii, Answering your offline question below. The reloaded tab does benefit from the logging state information sent to it before the reloading. Cheers, Vaclav https://codereview.chromium.org/2288203002/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2288203002/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:2765: forms_web_contents->ReloadFocusedFrame(false); This does not recreate the PasswordAutofillAgent (PAA) instance. The PAA instance is created once in RenderFrameImpl::Initialize. Calling ReloadFocusedFrame sends a message to the renderer which ends up handled by RenderFrameImpl::OnReload. In particular, the RenderFrameImpl instance is not destroyed, and hence no new Initialize is called.
On 2016/09/02 16:16:29, vabr (Chromium) wrote: > Hi Vasilii, > > I looked into the issue today. TL;DR -- I don't think the production code is > buggy. In addition, the test is best-effort, but unlikely to flake. > > Details: > > Observing the test on GNU/Linux and Mac, I saw that Mojo can take different > amounts of time (relative to the test execution speed) to do the round-trip from > the renderer: "Is logging active?" -> "Yes, it is." > > There is no contract with Mojo about the speed of such round-trips, so there is > no error in Mojo. > > Password manager code also does not have a contract capping the delay between > opening the internals page and logs coming in. Achieving that would only be > possible by blocking the renderer code until the response is received. However, > that would mean blocking the main purpose of the code (form filling) for a > helper feature (logging), which does not sound like the right trade-off. > > So the absence of a cap on the logging delay is a conscious decision. > > To capture that, the test needs to be permissive: it should verify that the > logging is available eventually, not necessarily in time for the first page > load. To do this precisely, I should write the test that it keeps reloading the > form page until it succeeds (or times out). If you prefer that, I'll do it, > otherwise I favour going with the slightly easier current solution (patch set > 7). > > Please let me know what you think about this. > > Cheers, > Vaclav LGTM if the test stays flaky then I vote for the second approach.
On 2016/09/05 12:56:02, vasilii wrote: > On 2016/09/02 16:16:29, vabr (Chromium) wrote: > > Hi Vasilii, > > > > I looked into the issue today. TL;DR -- I don't think the production code is > > buggy. In addition, the test is best-effort, but unlikely to flake. > > > > Details: > > > > Observing the test on GNU/Linux and Mac, I saw that Mojo can take different > > amounts of time (relative to the test execution speed) to do the round-trip > from > > the renderer: "Is logging active?" -> "Yes, it is." > > > > There is no contract with Mojo about the speed of such round-trips, so there > is > > no error in Mojo. > > > > Password manager code also does not have a contract capping the delay between > > opening the internals page and logs coming in. Achieving that would only be > > possible by blocking the renderer code until the response is received. > However, > > that would mean blocking the main purpose of the code (form filling) for a > > helper feature (logging), which does not sound like the right trade-off. > > > > So the absence of a cap on the logging delay is a conscious decision. > > > > To capture that, the test needs to be permissive: it should verify that the > > logging is available eventually, not necessarily in time for the first page > > load. To do this precisely, I should write the test that it keeps reloading > the > > form page until it succeeds (or times out). If you prefer that, I'll do it, > > otherwise I favour going with the slightly easier current solution (patch set > > 7). > > > > Please let me know what you think about this. > > > > Cheers, > > Vaclav > > LGTM > if the test stays flaky then I vote for the second approach. Thanks, Vasilii. I acknowledge that if the test stays flaky, I will do the second approach (keeping reloading until it works or times out). Cheers, Vaclav
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Redo PasswordManagerBrowserTestBase.InternalsPage The old version of the test was flaky after https://codereview.chromium.org/2270333002/ switched some messages to Mojo, because the signal that "internals page is ready for logging" propagated into the renderer after the renderer tried to log. The issue is likely Mojo-specific. It did not occur without the switch to Mojo. After the switch to Mojo, signals about navigation events no longer go through the same channel as the autofill messages (including the logging-related). Therefore the test code could have observed the internals page finish loading and consequently start loading the page with forms without all Mojo messages being properly transferred. The issue was test-specific, because the test was opening the page with forms much quicker after opening the internals page, than a human trying to capture the logs could do. The issue was Mac-specific, but not sure why. It does not seem important, though. Therefore, this CL fixes the test in adding an additional reload of the form page. That is, instead of: 1. Loading the internals page 2. Loading the page with forms the test now does 1'. Loading the internals page 2'. Loading the page with forms 3'. Reloading the page with forms The reloading increases the chance of the Mojo messages about logging activation to arrive to the renderer, simply because reloading takes longer. IIUC, there is no good event to wait for to be sure that the Mojo messages were received (without modifying the production code). The CL also splits the test into two, because it checked for two different\ things in the logs (browser vs. renderer messages). BUG=640737 R=vasilii@chromium.org ========== to ========== Redo PasswordManagerBrowserTestBase.InternalsPage The old version of the test was flaky after https://codereview.chromium.org/2270333002/ switched some messages to Mojo, because the signal that "internals page is ready for logging" propagated into the renderer after the renderer tried to log. The issue is likely Mojo-specific. It did not occur without the switch to Mojo. After the switch to Mojo, signals about navigation events no longer go through the same channel as the autofill messages (including the logging-related). Therefore the test code could have observed the internals page finish loading and consequently start loading the page with forms without all Mojo messages being properly transferred. The issue was test-specific, because the test was opening the page with forms much quicker after opening the internals page, than a human trying to capture the logs could do. The issue was Mac-specific, but not sure why. It does not seem important, though. Therefore, this CL fixes the test in adding an additional reload of the form page. That is, instead of: 1. Loading the internals page 2. Loading the page with forms the test now does 1'. Loading the internals page 2'. Loading the page with forms 3'. Reloading the page with forms The reloading increases the chance of the Mojo messages about logging activation to arrive to the renderer, simply because reloading takes longer. IIUC, there is no good event to wait for to be sure that the Mojo messages were received (without modifying the production code). The CL also splits the test into two, because it checked for two different\ things in the logs (browser vs. renderer messages). BUG=640737 R=vasilii@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Redo PasswordManagerBrowserTestBase.InternalsPage The old version of the test was flaky after https://codereview.chromium.org/2270333002/ switched some messages to Mojo, because the signal that "internals page is ready for logging" propagated into the renderer after the renderer tried to log. The issue is likely Mojo-specific. It did not occur without the switch to Mojo. After the switch to Mojo, signals about navigation events no longer go through the same channel as the autofill messages (including the logging-related). Therefore the test code could have observed the internals page finish loading and consequently start loading the page with forms without all Mojo messages being properly transferred. The issue was test-specific, because the test was opening the page with forms much quicker after opening the internals page, than a human trying to capture the logs could do. The issue was Mac-specific, but not sure why. It does not seem important, though. Therefore, this CL fixes the test in adding an additional reload of the form page. That is, instead of: 1. Loading the internals page 2. Loading the page with forms the test now does 1'. Loading the internals page 2'. Loading the page with forms 3'. Reloading the page with forms The reloading increases the chance of the Mojo messages about logging activation to arrive to the renderer, simply because reloading takes longer. IIUC, there is no good event to wait for to be sure that the Mojo messages were received (without modifying the production code). The CL also splits the test into two, because it checked for two different\ things in the logs (browser vs. renderer messages). BUG=640737 R=vasilii@chromium.org ========== to ========== Redo PasswordManagerBrowserTestBase.InternalsPage The old version of the test was flaky after https://codereview.chromium.org/2270333002/ switched some messages to Mojo, because the signal that "internals page is ready for logging" propagated into the renderer after the renderer tried to log. The issue is likely Mojo-specific. It did not occur without the switch to Mojo. After the switch to Mojo, signals about navigation events no longer go through the same channel as the autofill messages (including the logging-related). Therefore the test code could have observed the internals page finish loading and consequently start loading the page with forms without all Mojo messages being properly transferred. The issue was test-specific, because the test was opening the page with forms much quicker after opening the internals page, than a human trying to capture the logs could do. The issue was Mac-specific, but not sure why. It does not seem important, though. Therefore, this CL fixes the test in adding an additional reload of the form page. That is, instead of: 1. Loading the internals page 2. Loading the page with forms the test now does 1'. Loading the internals page 2'. Loading the page with forms 3'. Reloading the page with forms The reloading increases the chance of the Mojo messages about logging activation to arrive to the renderer, simply because reloading takes longer. IIUC, there is no good event to wait for to be sure that the Mojo messages were received (without modifying the production code). The CL also splits the test into two, because it checked for two different\ things in the logs (browser vs. renderer messages). BUG=640737 R=vasilii@chromium.org Committed: https://crrev.com/e78084414756fa83c67cb9a50f5693fb3bcef68d Cr-Commit-Position: refs/heads/master@{#416540} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e78084414756fa83c67cb9a50f5693fb3bcef68d Cr-Commit-Position: refs/heads/master@{#416540} |
