New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 664194 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

Verbose state animation triggered twice (http bad)

Project Member Reported by maxwalker@chromium.org, Nov 10 2016

Issue description

Version: 56.0.2915.0 
OS: Windows, Mac, Linux, CrOS

What steps will reproduce the problem?
(1) Visit http://http-password.badssl.com/

What is the expected result? What happens instead?
The animation showing the "Not secure" label is triggered twice, see attachment.
Could you take a look? Thanks!
 
Animation.gif
1.9 MB View Download

Comment 1 by est...@chromium.org, Nov 12 2016

Labels: -Pri-2 Hotlist-HttpBad M-56 Pri-1
Owner: est...@chromium.org
Status: Assigned (was: Untriaged)
I suspect this might be related to  issue 664674 . I think the way we're tracking password fields going away is getting confused during cross-process navigations or something.

Comment 2 by est...@chromium.org, Nov 16 2016

Cc: lgar...@chromium.org
Weirdly, I can only reproduce this on http-password.badssl.com, not other http password pages like http://webdbg.com/sandbox/FileForm.asp.

The bug, as far as I can tell from investigating remotely, is that the page or element seems to be getting laid out twice: the element gets attached to the layout tree, then detached, then attached again.

Lucas, do you know of anything weird about http-password.badssl.com that might be causing this? It looks pretty innocuous...

Comment 3 by est...@chromium.org, Nov 16 2016

Ah, I think it might be the css. Maybe loading and applying the stylesheet recreates the layout object...

Comment 4 by est...@chromium.org, Nov 16 2016

Cc: e...@chromium.org dcheng@chromium.org
Labels: OS-All
Ok, we need to fix this but I have no idea how. Adding some Blink experts (dcheng, eae) to see if they have any ideas.

Background for eae and dcheng: we show a "Not secure" warning in the omnibox when there's a password field on the page, and remove the warning when the password field is removed.

attachLayoutTree and detachLayoutTree trigger the message that shows/hides the "Not secure" warning. But it seems that loading a stylesheet causes the element to get detached and attached again, flickering the warning in and out of the omnibox.

Any ideas for how to avoid this flickering? At the moment, the only thing I can think of is doing something really hacky like delaying a few hundred ms before removing the warning. Will continue to think on it but thought I'd check if either of you have any ideas or thoughts. Thanks!

Comment 5 by est...@chromium.org, Nov 23 2016

Status: Started (was: Assigned)
If it helps, we have similar concerns for changes to other things in the UI (e.g. page title/favicon, whether the page is loading, etc.) and in at least some places we end up coalescing these.  I'm vague on the details of this, but it's possible you could use some of that machinery here.

Comment 7 by est...@chromium.org, Nov 24 2016

Thanks Peter! I ended up figuring out something that we can do in Blink for this particular problem (basically deferring the update to an async task that gets de-duped for multiple updates -- under review at https://codereview.chromium.org/2515373003/). But if we continue to run into similar problems then I'll look at what we might be able to reuse in the UI layer.
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1

commit 7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1
Author: estark <estark@chromium.org>
Date: Thu Nov 24 16:30:28 2016

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.

Review-Url: https://codereview.chromium.org/2515373003
Cr-Commit-Position: refs/heads/master@{#434340}

[modify] https://crrev.com/7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1/third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp
[modify] https://crrev.com/7bdbe352cdbd7cc1dec2b71653e60b47688bd9e1/third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0c7eb5a36b45db9a17c75715a99b3a547c45974

commit e0c7eb5a36b45db9a17c75715a99b3a547c45974
Author: tapted <tapted@chromium.org>
Date: Thu Nov 24 22:26:03 2016

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 

Review-Url: https://codereview.chromium.org/2531683002
Cr-Commit-Position: refs/heads/master@{#434400}

[modify] https://crrev.com/e0c7eb5a36b45db9a17c75715a99b3a547c45974/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/e0c7eb5a36b45db9a17c75715a99b3a547c45974/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/e0c7eb5a36b45db9a17c75715a99b3a547c45974/third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp
[modify] https://crrev.com/e0c7eb5a36b45db9a17c75715a99b3a547c45974/third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d262a0ed6bd977cfec18514171790fcaee02a0a1

commit d262a0ed6bd977cfec18514171790fcaee02a0a1
Author: estark <estark@chromium.org>
Date: Tue Nov 29 01:46:02 2016

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 

Review-Url: https://codereview.chromium.org/2538473002
Cr-Commit-Position: refs/heads/master@{#434820}

[modify] https://crrev.com/d262a0ed6bd977cfec18514171790fcaee02a0a1/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
[modify] https://crrev.com/d262a0ed6bd977cfec18514171790fcaee02a0a1/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/d262a0ed6bd977cfec18514171790fcaee02a0a1/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/d262a0ed6bd977cfec18514171790fcaee02a0a1/third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp
[modify] https://crrev.com/d262a0ed6bd977cfec18514171790fcaee02a0a1/third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp

Status: Fixed (was: Started)
This should be fixed in 57.0.2936.0; maxwalker, can you verify?

If so I'll request a merge to M56.
LGTM - thank you!
Labels: Merge-Request-56
Thanks Max! Requesting a merge for the commit in comment 10

Comment 14 by dimu@chromium.org, Nov 30 2016

Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 30 2016

Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2d532253572bc00d903431d10070a01defa9a3d

commit f2d532253572bc00d903431d10070a01defa9a3d
Author: Emily Stark <estark@google.com>
Date: Wed Nov 30 17:53:10 2016

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
BUG= 664194 

Review-Url: https://codereview.chromium.org/2538473002
Cr-Commit-Position: refs/heads/master@{#434820}
(cherry picked from commit d262a0ed6bd977cfec18514171790fcaee02a0a1)

Review URL: https://codereview.chromium.org/2536203004 .

Cr-Commit-Position: refs/branch-heads/2924@{#195}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/f2d532253572bc00d903431d10070a01defa9a3d/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
[modify] https://crrev.com/f2d532253572bc00d903431d10070a01defa9a3d/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/f2d532253572bc00d903431d10070a01defa9a3d/third_party/WebKit/Source/core/dom/Document.h
[modify] https://crrev.com/f2d532253572bc00d903431d10070a01defa9a3d/third_party/WebKit/Source/core/html/forms/PasswordInputType.cpp
[modify] https://crrev.com/f2d532253572bc00d903431d10070a01defa9a3d/third_party/WebKit/Source/core/html/forms/PasswordInputTypeTest.cpp

Sign in to add a comment