Verbose state animation triggered twice (http bad) |
||||||||
Issue descriptionVersion: 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!
,
Nov 16 2016
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...
,
Nov 16 2016
Ah, I think it might be the css. Maybe loading and applying the stylesheet recreates the layout object...
,
Nov 16 2016
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!
,
Nov 23 2016
,
Nov 24 2016
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.
,
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.
,
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
,
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
,
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
,
Nov 29 2016
This should be fixed in 57.0.2936.0; maxwalker, can you verify? If so I'll request a merge to M56.
,
Nov 30 2016
LGTM - thank you!
,
Nov 30 2016
Thanks Max! Requesting a merge for the commit in comment 10
,
Nov 30 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Nov 30 2016
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 |
||||||||
Comment 1 by est...@chromium.org
, Nov 12 2016Owner: est...@chromium.org
Status: Assigned (was: Untriaged)