Issue metadata
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 descriptionUserAgent: 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
,
Feb 10 2017
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.
,
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.
,
Feb 10 2017
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?
,
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.
,
Feb 10 2017
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).
,
Feb 10 2017
,
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
,
Feb 10 2017
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
,
Feb 10 2017
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?
,
Feb 10 2017
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?
,
Feb 10 2017
> 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.
,
Feb 10 2017
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."
,
Feb 13 2017
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?
,
Feb 13 2017
SGTM
,
Feb 13 2017
SGTM as well.
,
Feb 14 2017
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.
,
Feb 14 2017
Yep, should be a relatively easy change.
,
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
,
Feb 22 2017
,
Feb 22 2017
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
,
Feb 23 2017
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 .
,
Feb 23 2017
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
,
Feb 28 2017
,
Mar 1 2017
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.
,
Mar 24 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by brajkumar@chromium.org
, Feb 10 2017Labels: -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)