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

Issue 690523 link

Starred by 8 users

Issue metadata

Status: Fixed
Merged: issue 488143
Owner:
Last visit 28 days ago
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Cannot In-line Install Extension in User initiated Full Screen

Reported by teddy@proctorio.com, Feb 9 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce the problem:
1. Go to page that allows for inline install (e.g.: http://noelarlante.com/setup-chrome-extension-for-inline-installation/#, the link on the right hand side JSFiddle.Net Default to JQuery Library)
2. Maximize the browser to full screen
3. Attempt to install an extension, it will not work. This does not seem to happen from within the Chrome Web Store

What is the expected behavior?
The extension should install regardless of window state

What went wrong?
The extension does not install

Did this work before? N/A 

Chrome version: 56.0.2924.87  Channel: stable
OS Version: 6.3
Flash Version: Shockwave Flash 24.0 r0

 
Cc: brajkumar@chromium.org
Labels: -Type-Bug -Pri-2 hasbisect-per-revision M-57 OS-Linux OS-Mac Pri-1 Type-Bug-Regression
Owner: ackermanb@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce this issue on Mac OS 10.12, Ubuntu 14.04 and Windows-10 using chrome latest stable 
M56-56.0.2924.87.

Using the per-revision bisect providing the bisect results,

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

CHANGE-LOG URL:
---------------------------------------
https://chromium.googlesource.com/chromium/src/+log/e8a19cea39a641c9e35ab84bb621c4246883de7b..6ca20846938a643f34ea9ead734c81c4c6ebd74b

From the CL above, assigning the issue to the concern owner
Review-Url: https://codereview.chromium.org/2391353003

ackermanb@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Thanks!
Cc: rdevlin....@chromium.org mea...@chromium.org
Status: WontFix (was: Assigned)
This change is working as intended see crbug/488143 for more details. Unfortunately we have been seeing a lot of abuse with inline install by extensions using fullscreen mode and disabled it for security reasons.

Comment 3 by teddy@proctorio.com, Feb 10 2017

This is causing issues for our new users and we cannot be alone in this concern. Can you explain the security reasons? It seems like it is trivial to press F11 and then install, but there is no way of displaying this to users on a legitimate inline install site. 

Comment 4 by mea...@chromium.org, Feb 10 2017

Mergedinto: 488143
Status: Duplicate (was: WontFix)
Comment #3: The security concerns are explained in  bug 488143 . Can you please explain your use case? Is there a reason you cannot exit fullscreen mode before initiating inline install?

Comment 5 by teddy@proctorio.com, Feb 10 2017

I understand not installing from an iframe, but what is the difference between full screen and not full screen? What is the security concern? If you can ask the user to exit full screen then how is any security concern being fixed?

I've read through the other thread and I'm not sure I see where you address security concerns: "I forgot that I filed this bug. Not sure how big of a concern this is on the drive by web nowadays..."

I cannot remove fullscreen through JS running on the page. I understand that I can add a message that requests the user to remove their full screen, but these are users and the more messages there are the more confusing it gets. 

document.webkitExitFullscreen() and document.webkitIsFullScreen do not work as expected if the user has initiated full screen without requesting fullscreen through a gesture on the page.

Comment 6 by mea...@chromium.org, Feb 10 2017

Status: Available (was: Duplicate)
Summary: Cannot In-line Install Extension in User initiated Full Screen (was: Cannot In-line Install Extension in Full Screen)
The difference between fullscreen and not fullscreen is that the omnibox, which is the main security indicator, is visible in the non-fullscreen mode. The security problem is mentioned in the very first comment of that bug: That pages are tricking users to inline install extensions while in fullscreen mode. This works because the omnibox is not visible, so there is no way for the user to make a meaningful security decision.

However, it looks like I missed the part in your original report where you said it's the user initiating fullscreen mode, as opposed to the site entering it via webkitRequestfullscreen. In that case, yes, the concerns in  bug 488143  don't apply. If it's user-initiated fullscreen mode, we can exit fullscreen when the inline install dialog is displayed. 

This also aligns with  bug 670135  (modal dialogs should exit fullscreen).

Comment 7 by mea...@chromium.org, Feb 10 2017

Status: Assigned (was: Available)

Comment 8 by teddy@proctorio.com, Feb 10 2017

I hadn't thought about that, you're right that's a concern and that this situation is different than the one you present. I believe the user initiated full screen is occurring mostly on Mac where the green maximize window option will open Chrome in full screen. 

Thank you for adding this as a case in the other bug, we'd be happy to have this functionality back as it had been
Blockedon: 670135
The change for this prompt in particular is pretty easy - just modifying the line here [1] to only check IsTabFullscreen().  But I don't know if we'd want to do this before  bug 670135  is fixed?  Tentatively adding that as a blocker for this one.

[1] https://chromium.googlesource.com/chromium/src/+/0ddb4374a9221dba28e9ea8a1edaacb32dcc0861/chrome/browser/extensions/webstore_inline_installer.cc#187
I think mgiuca is only going to fix alert() dialogs in  Bug 670135 , so it shouldn't affect or block this bug.

I'd be okay with fixing the immediate issue (1) and then implementing the exit from fullscreen (2) as a follow up, but it sounds like doing (2) is straightforward enough to just go ahead do?
Blockedon: -670135
Cc: mgiuca@chromium.org
Ah, sorry, I was thinking of  issue 642568 , which is about showing UI for all modal dialogs/infobars/etc.  Unfortunately, it doesn't look like there's an ETA on that one.

My worry about implementing the exit from fullscreen is that there are a number of potential UX/edge case pitfalls - do we then re-enter fullscreen after?  What if the site tries to enter fullscreen once we exit?  etc.  I'd rather these types of questions be answered for the general case, rather than us having a one-off solution for extensions code.

mgiuca@, is there any timeline for  issue 642568 ?  meacer@, how confident are we that only blocking on tab-initiated fullscreen is enough to prevent abuse cases?
> do we then re-enter fullscreen after

IMO, no. Otherwise this is going to be effectively the same as implementing a different version of  issue 642568 .

> What if the site tries to enter fullscreen once we exit?

Entering fullscreen requires a user gesture, and it shouldn't be an issue if the inline install dialog is up since the user can't interact with the page. But in case the page somehow still manages to get a user gesture, perhaps we can ignore the enter fullscreen call? (which will bring its own set of edge cases and problems)

Also, mgiuca@ can correct me, but  issue 642568  was an alternative to  issue 670135 , and only one of them will be implemented (most likely  issue 670135 ).  

> meacer@, how confident are we that only blocking on tab-initiated fullscreen is enough to prevent abuse cases?

The abuse almost always consisted of the page initiating the inline install, but more people might be using fullscreen mode on Mac so this a tough call. Given that  bug 488143  has been around for quite some time, we might be okay allowing browser fullscreen for some more time.

All that said, it seems like Mac fullscreen is a good argument for implementing  issue 642568  in addition to  issue 642568 , both for inline install and alert()s.
Correction: "All that said, it seems like Mac fullscreen is a good argument for implementing  issue 642568  in addition to * issue 670135 *, both for inline install and alert()s."

Cool.  With all the discussion here, I think a safe decision is:
- Allow user-initiated fullscreen for installs.
- Solve the mac fullscreen use case (and user-initiated fullscreen in general) with whatever we come up with for  issue 642568 .

Does that sound good to everyone?
SGTM
SGTM as well.
ackermanb@, think this is something you'll be able to get to this week?  If we're going to make the merge back to 57, sooner is better than later.
Yep, should be a relatively easy change.
Project Member

Comment 19 by bugdroid1@chromium.org, Feb 15 2017

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

commit e6d00f176148e0aa1067b237a2352e99ceff751c
Author: ackermanb <ackermanb@chromium.org>
Date: Wed Feb 15 19:02:16 2017

Allow inline install for user initiated fullscreen mode since this mode requires that the user initiate it and due to a nuance with mac windows where the green maximize button enters fullscreen.

BUG= 690523 

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

[modify] https://crrev.com/e6d00f176148e0aa1067b237a2352e99ceff751c/chrome/browser/extensions/webstore_inline_installer.cc
[modify] https://crrev.com/e6d00f176148e0aa1067b237a2352e99ceff751c/chrome/browser/extensions/webstore_inline_installer_browsertest.cc
[modify] https://crrev.com/e6d00f176148e0aa1067b237a2352e99ceff751c/chrome/test/data/extensions/api_test/webstore_inline_install/install_from_fullscreen.html

Labels: Merge-Request-57
Status: Fixed (was: Assigned)
Project Member

Comment 21 by sheriffbot@chromium.org, Feb 22 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
If possible, could you please merge your change to M57 branch 2987 by 5:00 PM PT tomorrow, Thursday (02/23) so we can pick it up for this next week beta release.
Thank you .
Project Member

Comment 23 by bugdroid1@chromium.org, Feb 23 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/029de77350f15b7d2b946c3dea196cb81d672bbd

commit 029de77350f15b7d2b946c3dea196cb81d672bbd
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Feb 23 19:42:37 2017

Allow inline install for user initiated fullscreen mode

Allow inline install for user initiated fullscreen mode since this
mode requires that the user initiate it and due to a nuance with mac
windows where the green maximize button enters fullscreen.

BUG= 690523 

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

Review-Url: https://codereview.chromium.org/2716693002 .
Cr-Commit-Position: refs/branch-heads/2987@{#665}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/029de77350f15b7d2b946c3dea196cb81d672bbd/chrome/browser/extensions/webstore_inline_installer.cc
[modify] https://crrev.com/029de77350f15b7d2b946c3dea196cb81d672bbd/chrome/browser/extensions/webstore_inline_installer_browsertest.cc
[modify] https://crrev.com/029de77350f15b7d2b946c3dea196cb81d672bbd/chrome/test/data/extensions/api_test/webstore_inline_install/install_from_fullscreen.html

Cc: ackermanb@chromium.org hdodda@chromium.org
 Issue 696368  has been merged into this issue.
Labels: TE-Verified-57.0.2987.88 TE-Verified-M57
Verified the issue on Mac 10.12.3, Win 10 and Ubuntu 14.04 using 57.0.2987.88 and its working fine now.Hence added respective TE-verified labels.
690523_Mar_1.mp4
2.0 MB View Download
Cc: kkaluri@chromium.org
 Issue 689767  has been merged into this issue.

Sign in to add a comment