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

Issue 802156 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression

Blocking:
issue 739418



Sign in to add a comment

Regression : Unable to type anything in Google doc even if defaut text caret is present.

Reported by avsha...@etouch.net, Jan 16 2018

Issue description

Chrome Version : 65.0.3322.3 (Official Build) 8758ca55b13d4f2082b2ed9269fce8f37f37c577-refs/branch-heads/3322@{#6} 32/64-bit
OS : Mac(10.12.6, 10.13.1, 10.13.3), Windows(7,8,8.1,10), Linux(14.04 LTS)

Test URL : https://www.google.com/intl/en/drive/

What steps will reproduce the problem?
1. Launch chrome, navigate to above URL and sign in to Google drive with valid credentials.
2. In google drive, click on ‘NEW’ button and select ‘Google Docs’ option (A fresh document opens in a new tab).
3. Let the document load completely (do not click anywhere on the document) and try to type something in it.
4. Observe. 

Actual Results : 
1. Default text caret in google doc stops blinking after few seconds.
2. Unable to type anything in Google doc.

Expected Results : 
1. Default text caret in google docs should keep blinking so that it acknowledges user about the current focus position on a doc.
2. Should be able to type in Google doc.

This is a regression issue broken in ‘M-65’ and using the per-revision bisect providing the bisect results,
Good Build : 65.0.3295.0 (Revision : 524285)
Bad Build : 65.0.3296.0 (Revision : 524554)

You are probably looking for a change made after  (known good), but no later than  (first known bad).

CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/24abf78274e01a17c4f0a4c8cc18e55720f3b3b9..3b60933b099bba43823b2c5cbdbe7c3f2e90ae68

Suspect : https://chromium.googlesource.com/chromium/src/+/3b60933b099bba43823b2c5cbdbe7c3f2e90ae68

@alexmos : Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Note : 
1. User needs to click twice on a google doc in order to make default text caret blink again.
2. Able to type in google doc, once the text caret start to blink again.
3. This issue is also observed on M-65 Dev #65.0.3315.3 build.
 
Actual_Result.mov
3.9 MB Download
Expected_Result.mov
5.0 MB Download
Labels: RegressedIn-65 ReleaseBlock-Beta Target-65 FoundIn-65
issue already existing from current Dev 65.0.3315.3, marking with RBB blocker .. please change if required.
Cc: aval...@chromium.org
Components: -Platform>Apps Internals>Sandbox>SiteIsolation
Labels: -FoundIn-65 -RegressedIn-65 OS-Chrome
Confirmed that this is due to sign-in isolation.  I've verified this also affects the current stable release (63.0.3239.132 on Mac).  It also occurs when disabling --site-per-process or --isolate-origins policies, but it does not occur when I run Chrome with --disable-features=sign-in-process-isolation.

If you open the task manager, it looks like the caret freezes when the accounts.google.com subframe first appears in task manager.  So I suspect it has something to do with the accounts subframe stealing focus from the Docs frame.  I'll investigate.

Clicking on the doc, or focusing another window and going back, unfreezes the caret and lets the user type.  So there's an easy workaround, but it's still not a great user experience for sure.

Note that sign-in isolation is currently enabled for 100% of all channels right now, so it's not a recent regression.

Comment 3 by creis@chromium.org, Jan 16 2018

Cc: ew...@chromium.org
Cc: rbyers@chromium.org
 Issue 799578  has been merged into this issue.
Status: Started (was: Assigned)
I think I understand what's going on here.  Fix in progress.

This is a generic site isolation bug that just happens to manifest itself with sign-in isolation in Docs.  

Docs apparently puts the blinking caret in another iframe and focuses that frame with window.focus().  Then, it adds an accounts.google.com subframe (oauth-related, since it's named similar to oauth2relay317124780), which goes into an OOPIF when sign-in isolation is enabled.  When that subframe commits, it triggers this code in RFHM::CommitPending():

      // The main frame's view is already focused, but we need to set
      // page-level focus in the subframe's renderer.
      frame_tree_node_->frame_tree()->SetPageFocus(
          render_frame_host_->GetSiteInstance(), true);

which sends an IPC to set page focus to true in the accounts.google.com renderer.  However, at this point, that renderer doesn't know what the focused frame is and that it has already been changed from the main frame to the "blinking caret" iframe (both RemoteFrames in its process).  It hits this code as part of setting page focus in FocusController::SetFocused():

  if (!focused_frame_ && is_focused_)
    SetFocusedFrame(page_->MainFrame());

That sets the main frame as the focused frame and sends the FrameHostMsg_FrameFocused IPC up to the browser process, which resets the focused frame to the main frame and steals focus away from the blinking caret frame.

A solution would be to let new OOPIF renderer processes know which RemoteFrame has focus in such cases, before setting page focus.
I was testing this yesterday as part of another accessibility bug b/71605481, but I can verify that it is not an accessibility specific bug. This repros intermittently in many versions of Chrome (Canary, 65,64,63) with accessibility mode disabled and no screen readers. I tried to bisect across different ranges but the issue was too intermittent to tie to a specific Chrome release. 
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 18 2018

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

commit f1d9af002af9478cb193b775b9b5889455c67e51
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Jan 18 00:54:24 2018

Prevent an OOPIF commit from stealing focus from other subframes.

When a cross-process subframe navigation commits, the browser
process sends an IPC to the OOPIF to tell it whether it currently
has page focus (based on the page's overall page focus).
However, it never tells the OOPIF about frame focus, in the case
that some frame had become focused prior to that navigation.
This leads to a bug, where as part of setting page focus, Blink's
FocusController::SetFocused() will set the focused frame to the
main frame if the focused frame isn't set yet, which will steal
focus from subframes and give it to the main frame.

This CL fixes this by telling OOPIF processes about both frame
and page focus at commit time.

Bug:  802156 
Change-Id: I14a3cdc248681178b9a5eaf570310224aec2c887
Reviewed-on: https://chromium-review.googlesource.com/871523
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529989}
[modify] https://crrev.com/f1d9af002af9478cb193b775b9b5889455c67e51/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/f1d9af002af9478cb193b775b9b5889455c67e51/content/browser/site_per_process_browsertest.cc

Labels: M-64 Target-64
Labels: M-63

Comment 10 by kirtia@google.com, Jan 18 2018

Thanks Alex for the quick fix! Please follow b/71605481 to get the latest updates from Docs side on the fix. 

Comment 11 by josa...@google.com, Jan 18 2018

Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
moving to RBS for M64 since user able to type after clicking in the doc, please add merge-request once CL is verified in ToT 
Cc: abdulsyed@chromium.org
Labels: Merge-Request-64
Status: Fixed (was: Started)
Commit f1d9af00... initially landed in 65.0.3324.0.  People impacted by this bug have confirmed in b/71605481#comment18 that this bug no longer repros in today's Canary - I think we can mark the bug as fixed and request a merge to M64.
Project Member

Comment 13 by sheriffbot@chromium.org, Jan 18 2018

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: We are only 4 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
I've also confirmed that the fix is working as intended on 65.0.3324.0, and I also checked that so far there aren't any related crashes being reported.  The fix itself is small and well-contained (just a few lines in one function), so I think it's safe for merge.
Labels: -Merge-Review-64 Merge-Approved-64
Approving merge for M64. Branch:3282
Project Member

Comment 16 by bugdroid1@chromium.org, Jan 19 2018

Labels: -merge-approved-64 merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59d85e7d4bb4d4142a3bf9dae5e2611bb8400fc8

commit 59d85e7d4bb4d4142a3bf9dae5e2611bb8400fc8
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Jan 19 00:01:44 2018

Prevent an OOPIF commit from stealing focus from other subframes (Merge to M64)

When a cross-process subframe navigation commits, the browser
process sends an IPC to the OOPIF to tell it whether it currently
has page focus (based on the page's overall page focus).
However, it never tells the OOPIF about frame focus, in the case
that some frame had become focused prior to that navigation.
This leads to a bug, where as part of setting page focus, Blink's
FocusController::SetFocused() will set the focused frame to the
main frame if the focused frame isn't set yet, which will steal
focus from subframes and give it to the main frame.

This CL fixes this by telling OOPIF processes about both frame
and page focus at commit time.

TBR=alexmos@chromium.org

(cherry picked from commit f1d9af002af9478cb193b775b9b5889455c67e51)

Bug:  802156 
Change-Id: I14a3cdc248681178b9a5eaf570310224aec2c887
Reviewed-on: https://chromium-review.googlesource.com/871523
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#529989}
Reviewed-on: https://chromium-review.googlesource.com/875313
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#543}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/59d85e7d4bb4d4142a3bf9dae5e2611bb8400fc8/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/59d85e7d4bb4d4142a3bf9dae5e2611bb8400fc8/content/browser/site_per_process_browsertest.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 19 2018

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

commit 665a1fc46d26c39dc613544c9b8a7c3b0fe76b6e
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Jan 19 01:45:37 2018

Fix focus-related comment in RFHM::CommitPending().

This is a followup to address
https://chromium-review.googlesource.com/c/chromium/src/+/871523/3/content/browser/frame_host/render_frame_host_manager.cc#2036

Bug:  802156 
Change-Id: I1f261f4e0ca26e8c700a13ad15a76027d681d39c
Reviewed-on: https://chromium-review.googlesource.com/875243
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530383}
[modify] https://crrev.com/665a1fc46d26c39dc613544c9b8a7c3b0fe76b6e/content/browser/frame_host/render_frame_host_manager.cc

I've verified the fix on 64.0.3282.113 Win beta, which just came out and includes the merge from #16.
Verified the fix on Chrome 65.0.3325.9/10323.1.0(Peppy,Paine), 64.0.3282.114/10176.59.0(Peppy, Caroline)    
Blocking: 739418
Labels: ET-MUM-Reported

Sign in to add a comment