Issue metadata
Sign in to add a comment
|
Regression: Sign-in to chrome page becomes unresponsive after giving print command.
Reported by
sanyam.g...@etouch.net,
Sep 4
|
||||||||||||||||||||||
Issue descriptionChrome Version: 71.0.3542.0 (Official Build) Revision 4097c6595e73c799040bc3a99bcfcc6adb178386-refs/branch-heads/3542@{#1} (32/64-bit) OS: Windows(7,8,8.1,10)and Linux(14.04 LTS) What steps will reproduce the problem? 1. Launch chrome, navigate to NTP and give print command by pressing 'ctrl+p' 2. Now click on avatar icon (while print preview overlay is open). 3. Click on 'sign in' from avatar overlay and observe. Actual Result : Sign-in to chrome page becomes unresponsive. Expected Result: Sign-in to chrome page should not become unresponsive This is regression issue broken in ‘M-70’ and will soon update the other info: Good build: 70.0.3536.0(Revision: 587137) Bad build : 70.0.3537.0(Revision: 587302) Note: Issue is not reproducible on Mac (10.12.6, 10.13.1, 10.13.6, 10.14) OS
,
Sep 4
Thank you for the bug report. I was able to reproduce this by cloning from the master branch. However, I cannot reproduce when I check out de05f8f9dc649b306c6ba098c45977ed84894960 (r587249) and build Chromium (with API keys). Based on this, I don't think my patch introduced this regression...
,
Sep 5
nicolaso: Can you try bisecting with tools/bisect-builds.py to help find the cause? You'll need to set the API keys at run time instead of at build time. Let me know if you need help.
,
Sep 5
Re-bisected with API keys to https://chromium.googlesource.com/chromium/src/+log/578be39046de0eb8ad2c103b3a98dda76b5651b2..6aa5a8a398d978aca7cb31ecf40afd764323d8da I'm guessing this is https://crrev.com/c/1194484, which is the fix for https://crbug.com/863582 . This prevented interacting with PDFs behind constrained web dialogs (like Print Preview). Maybe it also disabled interacting with this sign-in input somehow. cc-ing avi@.
,
Sep 5
+avi@ Ran the bisect script to identify the culprit CL. It looks like [1] is fine, but I can reproduce the bug on [2]. avi@, since you are the author of [2], do you think your change could have introduced this bug? [1] https://chromium.googlesource.com/chromium/src/+/09a292e587ef9c97e9bf19e316b47061f62d98be [2] https://chromium.googlesource.com/chromium/src/+/738ea19831a4395bff3066a8ce994482ac8b44d6
,
Sep 5
( I should really refresh the page before hitting 'send' >_> ) Anyways, looks like I arrived at the same results as rbpotter@ did
,
Sep 5
I'm OOO for 2 weeks or so. This sounds related to what I've done, but if widgets in an unrelated WebContents are going unresponsive, this is surprising and makes me think that something deeper is wrong.
,
Sep 24
I cannot reproduce this as the steps don't work for me. When I click the sign-in button, in the repro video that causes the print dialog to close, but on the ToT I'm working on (67025f5a3a66d54d511f8f8a1b97a5b6ad30dd7b) that doesn't cause the print dialog to close, and when I manually close the dialog things work.
,
Sep 24
I accidentally ran into another way to reproduce this the other day, which seems to still work in current Canary (on Win10): (1) Open Print Preview (2) In the omnibox, type chrome://settings/searchEngines + hit enter to navigate (3) Try to interact with the search field for the subpage, located below the blue header next to the "Manage Search Engines" label.
,
Sep 24
Issue 884636 has been merged into this issue.
,
Sep 24
This seems to be some kind of race that I'm not winning on my custom builds. I have a suspicion for a fix based on the code.
,
Sep 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f7e423bf9327e5069d1c462c1da43e487932b606 commit f7e423bf9327e5069d1c462c1da43e487932b606 Author: Avi Drissman <avi@chromium.org> Date: Tue Sep 25 22:51:57 2018 Make sure to unblock a WebContents during a mass dialog closure. BUG= 880210 Change-Id: I44e80bc0866fef5370936db3f2cb4d3eacf620f2 Reviewed-on: https://chromium-review.googlesource.com/1241094 Reviewed-by: Leonard Grey <lgrey@chromium.org> Reviewed-by: Mike Wittman <wittman@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#594135} [modify] https://crrev.com/f7e423bf9327e5069d1c462c1da43e487932b606/components/web_modal/web_contents_modal_dialog_manager.cc [modify] https://crrev.com/f7e423bf9327e5069d1c462c1da43e487932b606/components/web_modal/web_contents_modal_dialog_manager.h
,
Sep 25
This fix is from inspection, as I can't repro. For those who have repro-ed, can you test on a canary that has this patch to verify? Thanks!
,
Sep 26
Update : Rechecked the above issue on Windows (7,8,8.1,10) and Linux (14.04 LTS) using latest Canary build #71.0.3562.0 and the issue is still reproducible. Kindly refer the attached screen cast. Thank you..!!
,
Sep 26
rbpotter: that seems like a focus issue, though the call that should be focusing is the same one that isn't getting called re event routing. I'm still looking at this.
,
Sep 26
OK, I know what's going on, though not entirely what changed.
What's happening when navigating away from the page with the print dialog:
1. The print dialog is closed.
2. WebContentsModalDialogManager::BlockWebContentsInteraction(false) is called.
3. That calls SetWebContentsBlocked(false) on the delegate, which is Browser.
4. Browser::SetWebContentsBlocked() sees that the WebContents is being unblocked and is in front, so it calls WebContents::Focus().
5. WebContentsImpl::Focus() calls WebContentsView::Focus() on the view.
6. Linux and Windows use WebContentsViewAura::Focus(), which (unlike WebContentsViewMac::Focus() and thus the platform difference!) delegates to WebContentsViewDelegate::Focus().
7. The delegate for Chromium is ChromeWebContentsViewDelegateViews, and its Focus() call turns around and calls Focus() on the focus helper.
8. The ChromeWebContentsViewFocusHelper::Focus() call has:
const web_modal::WebContentsModalDialogManager* manager =
web_modal::WebContentsModalDialogManager::FromWebContents(web_contents_);
if (manager && manager->IsDialogActive())
manager->FocusTopmostDialog();
The WebContentsModalDialogManager indeed thinks that there is a dialog active, and "activates" it, causing the page to not properly take focus.
I don't know where in this chain my change made things different, but this is the chain right now. The question I'm going to answer is "after step 1, can we make sure that the dialog manager returns false from IsDialogActive()?"
,
Sep 27
OK, it's not quite as simple as #16 makes it out to be. Commenting out the FocusTopmostDialog() call in ChromeWebContentsViewFocusHelper::Focus() makes the problem go away, but the problem is that it's not called by Browser when the dialog goes away. It's called when *someone else* calls Focus(). I'm still not clear about what's going on.
,
Sep 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a0549215f29c2a0b574f323222a043ecebde6a68 commit a0549215f29c2a0b574f323222a043ecebde6a68 Author: Avi Drissman <avi@chromium.org> Date: Fri Sep 28 18:43:45 2018 Revert "Make sure to unblock a WebContents during a mass dialog closure." This reverts commit f7e423bf9327e5069d1c462c1da43e487932b606. Reason for revert: This was a speculative fix, thinking that perhaps WillClose() wasn't getting called. But it is, and all the correct calls are being made (see https://bugs.chromium.org/p/chromium/issues/detail?id=880210#c16) so this code only complicates things. Original change's description: > Make sure to unblock a WebContents during a mass dialog closure. > > BUG= 880210 > > Change-Id: I44e80bc0866fef5370936db3f2cb4d3eacf620f2 > Reviewed-on: https://chromium-review.googlesource.com/1241094 > Reviewed-by: Leonard Grey <lgrey@chromium.org> > Reviewed-by: Mike Wittman <wittman@chromium.org> > Commit-Queue: Avi Drissman <avi@chromium.org> > Cr-Commit-Position: refs/heads/master@{#594135} TBR=avi@chromium.org,wittman@chromium.org,lgrey@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 880210 Change-Id: Ibbd669649add66e0bc4052eb6b04c845ffb40f38 Reviewed-on: https://chromium-review.googlesource.com/1249389 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Leonard Grey <lgrey@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#595166} [modify] https://crrev.com/a0549215f29c2a0b574f323222a043ecebde6a68/components/web_modal/web_contents_modal_dialog_manager.cc [modify] https://crrev.com/a0549215f29c2a0b574f323222a043ecebde6a68/components/web_modal/web_contents_modal_dialog_manager.h
,
Sep 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d1596214f9f76bbc890cd792076c9d00d54bf8cb commit d1596214f9f76bbc890cd792076c9d00d54bf8cb Author: Avi Drissman <avi@chromium.org> Date: Fri Sep 28 19:05:19 2018 Fix focus issues with web contents modal dialogs. BUG= 880210 TEST=as in bug Change-Id: I9f305084bf9f6b146d0323baeabc4437466857df Reviewed-on: https://chromium-review.googlesource.com/1252241 Reviewed-by: Leonard Grey <lgrey@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#595171} [modify] https://crrev.com/d1596214f9f76bbc890cd792076c9d00d54bf8cb/chrome/browser/ui/views/tab_contents/chrome_web_contents_view_focus_helper.cc
,
Sep 28
That should fix it; please verify.
,
Dec 14
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by sanyam.g...@etouch.net
, Sep 4Owner: nicolaso@chromium.org
Status: Assigned (was: Unconfirmed)