Reland of Post tasks for sensitive input visibility notifications

Reland of Post tasks for sensitive input visibility notifications (patchset #1 id:1 of https://codereview.chromium.org/2531683002/ )

Reason for revert:
Fixing SecuritySTateTabHelperTests

Original issue's description:
> Revert of Post tasks for sensitive input visibility notifications (patchset #5 id:80001 of https://codereview.chromium.org/2515373003/ )
>
> Reason for revert:
> Suspected for flaky failures on browser_tests for Mac 10.9 asan, Mac 10.10
>
> E.g.
> SecurityStateTabHelperTestWithPasswordCcSwitch/SecurityStateTabHelperTestWithPasswordCcSwitch.PasswordSecurityLevelDowngradedFromIframe/0
> SecurityStateTabHelperTest.PasswordSecurityLevelNotDowngradedWithoutSwitch
>
> https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/24682
> https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.10%20Tests/builds/9716
>
> Errors like
>
> ../../chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:966: Failure
> Value of: security_info.security_level
>   Actual: 0
> Expected: security_state::HTTP_SHOW_WARNING
> Which is: 1
> ../../chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:971: Failure
> Value of: entry->GetSSL().content_status & content::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP
>   Actual: false
> Expected: true
>
> Original issue's description:
> > Post tasks for sensitive input visibility notifications
> >
> > If password inputs are hidden and shown multiple times in the same task
> > (as apparently happens when processing a stylesheet), we don't want to
> > the omnibox warning to flicker in and out. Thus, this CL posts sensitive
> > input visibility notifications to the end of the task queue, so that the
> > omnibox warning updates get coalesced.
> >
> > BUG=664194
> > TEST=Visit http://http-password.badssl.com and observe that the omnibox
> > warning does not flicker in and out twice.
> >
> > Committed: https://crrev.com/7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1
> > Cr-Commit-Position: refs/heads/master@{#434340}
>
> TBR=dcheng@chromium.org,haraken@chromium.org,estark@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=664194
>
> Committed: https://crrev.com/e0c7eb5a36b45db9a17c75715a99b3a547c45974
> Cr-Commit-Position: refs/heads/master@{#434400}

TBR=dcheng@chromium.org,haraken@chromium.org,tapted@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=664194
Committed: https://crrev.com/d262a0ed6bd977cfec18514171790fcaee02a0a1
Cr-Commit-Position: refs/heads/master@{#434820}

[estark, 2016-11-28 17:33:14]

Created Reland of Post tasks for sensitive input visibility notifications

[estark, 2016-11-28 17:33:52]

Note to reviewers: there's nothing to review at the moment as I haven't fixed
the tests; will send mail when the patch is fixed

[estark, 2016-11-28 20:31:29]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 20:33:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-11-28 22:10:11]

The CQ bit was unchecked by estark@chromium.org

[estark, 2016-11-28 23:18:37]

estark@chromium.org changed reviewers:
+ meacer@chromium.org

[estark, 2016-11-28 23:18:37]

meacer, Lord of Fixing Flaky Browser Tests: could you take a look at
security_state_tab_helper_browser_tests.cc and let me know what you think?

It used to be that Blink sent a message to the browser process whenever a
password field was created. When a password field was created in the initial
page load, the browser process would receive that message before NavigateToURL()
returned. In this CL that I'm trying to reland, password field notifications are
queued at the end of the Blink task queue, so the browser process is not
guaranteed to hear about password fields before NavigateToURL() returns. To work
around this, I'm having the browser tests inject a script after NavigateToURL()
returns, to ensure that the event loop has run past the password field
notification. Feels kinda hacky though. Do you have any other ideas? Thanks!

[meacer, 2016-11-28 23:41:08]

On 2016/11/28 23:18:37, estark wrote:
> meacer, Lord of Fixing Flaky Browser Tests: could you take a look at
> security_state_tab_helper_browser_tests.cc and let me know what you think?
> 
> It used to be that Blink sent a message to the browser process whenever a
> password field was created. When a password field was created in the initial
> page load, the browser process would receive that message before
NavigateToURL()
> returned. In this CL that I'm trying to reland, password field notifications
are
> queued at the end of the Blink task queue, so the browser process is not
> guaranteed to hear about password fields before NavigateToURL() returns. To
work
> around this, I'm having the browser tests inject a script after
NavigateToURL()
> returns, to ensure that the event loop has run past the password field
> notification. Feels kinda hacky though. Do you have any other ideas? Thanks!

Looks reasonable to me, but fair warning that I'm not very familar with blinks
event loops.
Would using blink::testing::runPendingTasks() help instead of injecting scripts?

[estark, 2016-11-28 23:56:11]

On 2016/11/28 23:41:08, Mustafa Emre Acer wrote:
> On 2016/11/28 23:18:37, estark wrote:
> > meacer, Lord of Fixing Flaky Browser Tests: could you take a look at
> > security_state_tab_helper_browser_tests.cc and let me know what you think?
> > 
> > It used to be that Blink sent a message to the browser process whenever a
> > password field was created. When a password field was created in the initial
> > page load, the browser process would receive that message before
> NavigateToURL()
> > returned. In this CL that I'm trying to reland, password field notifications
> are
> > queued at the end of the Blink task queue, so the browser process is not
> > guaranteed to hear about password fields before NavigateToURL() returns. To
> work
> > around this, I'm having the browser tests inject a script after
> NavigateToURL()
> > returns, to ensure that the event loop has run past the password field
> > notification. Feels kinda hacky though. Do you have any other ideas? Thanks!
> 
> Looks reasonable to me, but fair warning that I'm not very familar with blinks
> event loops.
> Would using blink::testing::runPendingTasks() help instead of injecting
scripts?

I don't think there's a way to call that from the browser process in a browser
test, is there?

[dcheng, 2016-11-29 00:06:12]

FWIW, I think we've used a similar hack to deflake site isolation tests (for the
reasons mentioned; it's the only way to spin the renderer's message loop)

[meacer, 2016-11-29 00:36:31]

On 2016/11/28 23:56:11, estark wrote:
> On 2016/11/28 23:41:08, Mustafa Emre Acer wrote:
> > On 2016/11/28 23:18:37, estark wrote:
> > > meacer, Lord of Fixing Flaky Browser Tests: could you take a look at
> > > security_state_tab_helper_browser_tests.cc and let me know what you think?
> > > 
> > > It used to be that Blink sent a message to the browser process whenever a
> > > password field was created. When a password field was created in the
initial
> > > page load, the browser process would receive that message before
> > NavigateToURL()
> > > returned. In this CL that I'm trying to reland, password field
notifications
> > are
> > > queued at the end of the Blink task queue, so the browser process is not
> > > guaranteed to hear about password fields before NavigateToURL() returns.
To
> > work
> > > around this, I'm having the browser tests inject a script after
> > NavigateToURL()
> > > returns, to ensure that the event loop has run past the password field
> > > notification. Feels kinda hacky though. Do you have any other ideas?
Thanks!
> > 
> > Looks reasonable to me, but fair warning that I'm not very familar with
blinks
> > event loops.
> > Would using blink::testing::runPendingTasks() help instead of injecting
> scripts?
> 
> I don't think there's a way to call that from the browser process in a browser
> test, is there?

Ah, of course. Ignore that.

[estark, 2016-11-29 00:38:23]

On 2016/11/29 00:06:12, dcheng wrote:
> FWIW, I think we've used a similar hack to deflake site isolation tests (for
the
> reasons mentioned; it's the only way to spin the renderer's message loop)

Ok, yay, that makes me feel a little better. I'm going to go ahead and reland
this then (the red bot looks like an unrelated flake).

[estark, 2016-11-29 00:40:11]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-11-29 00:41:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-11-29 01:40:20]

LGTM

[commit-bot: I haz the power, 2016-11-29 01:46:06]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480380011834790,
"parent_rev": "7266dea9f7efd387478c9a27605ec9d408176777", "commit_rev":
"a5e88b5b78cf9eacb6216c36aeee6352f2802b6b"}

[commit-bot: I haz the power, 2016-11-29 01:47:05]

Committed patchset #2 (id:120001)

[commit-bot: I haz the power, 2016-11-29 01:50:42]

Description was changed from

==========
Reland of Post tasks for sensitive input visibility notifications (patchset #1
id:1 of https://codereview.chromium.org/2531683002/ )

Reason for revert:
Fixing SecuritySTateTabHelperTests

Original issue's description:
> Revert of Post tasks for sensitive input visibility notifications (patchset #5
id:80001 of https://codereview.chromium.org/2515373003/ )
> 
> Reason for revert:
> Suspected for flaky failures on browser_tests for Mac 10.9 asan, Mac 10.10
> 
> E.g.
>
SecurityStateTabHelperTestWithPasswordCcSwitch/SecurityStateTabHelperTestWithPasswordCcSwitch.PasswordSecurityLevelDowngradedFromIframe/0
> SecurityStateTabHelperTest.PasswordSecurityLevelNotDowngradedWithoutSwitch
> 
>
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%20...
>
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.10%20Tests...
> 
> Errors like
> 
> ../../chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:966:
Failure
> Value of: security_info.security_level
>   Actual: 0
> Expected: security_state::HTTP_SHOW_WARNING
> Which is: 1
> ../../chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:971:
Failure
> Value of: entry->GetSSL().content_status &
content::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP
>   Actual: false
> Expected: true
> 
> Original issue's description:
> > Post tasks for sensitive input visibility notifications
> >
> > If password inputs are hidden and shown multiple times in the same task
> > (as apparently happens when processing a stylesheet), we don't want to
> > the omnibox warning to flicker in and out. Thus, this CL posts sensitive
> > input visibility notifications to the end of the task queue, so that the
> > omnibox warning updates get coalesced.
> >
> > BUG=664194
> > TEST=Visit http://http-password.badssl.com and observe that the omnibox
> > warning does not flicker in and out twice.
> >
> > Committed: https://crrev.com/7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1
> > Cr-Commit-Position: refs/heads/master@{#434340}
> 
> TBR=dcheng@chromium.org,haraken@chromium.org,estark@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=664194
> 
> Committed: https://crrev.com/e0c7eb5a36b45db9a17c75715a99b3a547c45974
> Cr-Commit-Position: refs/heads/master@{#434400}

TBR=dcheng@chromium.org,haraken@chromium.org,tapted@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=664194
==========

to

==========
Reland of Post tasks for sensitive input visibility notifications (patchset #1
id:1 of https://codereview.chromium.org/2531683002/ )

Reason for revert:
Fixing SecuritySTateTabHelperTests

Original issue's description:
> Revert of Post tasks for sensitive input visibility notifications (patchset #5
id:80001 of https://codereview.chromium.org/2515373003/ )
>
> Reason for revert:
> Suspected for flaky failures on browser_tests for Mac 10.9 asan, Mac 10.10
>
> E.g.
>
SecurityStateTabHelperTestWithPasswordCcSwitch/SecurityStateTabHelperTestWithPasswordCcSwitch.PasswordSecurityLevelDowngradedFromIframe/0
> SecurityStateTabHelperTest.PasswordSecurityLevelNotDowngradedWithoutSwitch
>
>
https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%20...
>
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.10%20Tests...
>
> Errors like
>
> ../../chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:966:
Failure
> Value of: security_info.security_level
>   Actual: 0
> Expected: security_state::HTTP_SHOW_WARNING
> Which is: 1
> ../../chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:971:
Failure
> Value of: entry->GetSSL().content_status &
content::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP
>   Actual: false
> Expected: true
>
> Original issue's description:
> > Post tasks for sensitive input visibility notifications
> >
> > If password inputs are hidden and shown multiple times in the same task
> > (as apparently happens when processing a stylesheet), we don't want to
> > the omnibox warning to flicker in and out. Thus, this CL posts sensitive
> > input visibility notifications to the end of the task queue, so that the
> > omnibox warning updates get coalesced.
> >
> > BUG=664194
> > TEST=Visit http://http-password.badssl.com and observe that the omnibox
> > warning does not flicker in and out twice.
> >
> > Committed: https://crrev.com/7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1
> > Cr-Commit-Position: refs/heads/master@{#434340}
>
> TBR=dcheng@chromium.org,haraken@chromium.org,estark@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=664194
>
> Committed: https://crrev.com/e0c7eb5a36b45db9a17c75715a99b3a547c45974
> Cr-Commit-Position: refs/heads/master@{#434400}

TBR=dcheng@chromium.org,haraken@chromium.org,tapted@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=664194

Committed: https://crrev.com/d262a0ed6bd977cfec18514171790fcaee02a0a1
Cr-Commit-Position: refs/heads/master@{#434820}
==========

[commit-bot: I haz the power, 2016-11-29 01:50:43]

Patchset 2 (id:??) landed as
https://crrev.com/d262a0ed6bd977cfec18514171790fcaee02a0a1
Cr-Commit-Position: refs/heads/master@{#434820}