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

Issue 890848 link

Starred by 4 users
Status: Fixed
Owner:
dominickn@chromium.org
Closed: Oct 8
Cc:
█ mgiuca@chromium.org
dominickn@chromium.org
█ bgalbs@google.com
pcovell@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

userChoice promise doesn't resolve

Project Member Reported by petele@google.com, Oct 1 
Chrome Version       : 70.0.3538.34
OS Version: 11021.28.0
URLs (if applicable) : https://amber-ash-pete.glitch.me/
Other browsers tested:
  This works as expected (userChoice promise is resolved) on Chrome for Android (tested 69,70 & 71)

What steps will reproduce the problem?
1. In a browser tab, go to https://amber-ash-pete.glitch.me/
2. Click Install PWA


What is the expected result?
userChoice promise should resolve and it's output should be added to the textbox: 'userChoice: {"outcome": "accepted"...}', followed by '`appinstalled` fired'


What happens instead of that?
userChoice promise is not resolved and left pending (nothing is added to the text box). Only '`appinstalled` fired' appears.


Please provide any additional information below. Attach a screenshot if
possible.

UserAgentString: Mozilla/5.0 (X11; CrOS x86_64 11021.28.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.34 Safari/537.36

Comment 1 by petele@google.com, Oct 1 

Note - this also fails on Chrome OS m69
cros-ifail.png
22.2 KB View Download

Comment 2 by petele@chromium.org, Oct 1

Components: UI>Browser>WebAppInstalls
Labels: -Pri-3 Pri-1

Comment 3 by petele@chromium.org, Oct 1

Labels: allpublic

Comment 4 by dominickn@chromium.org, Oct 2

Cc: mgiuca@chromium.org
Labels: OS-Linux OS-Mac OS-Windows
Owner: dominickn@chromium.org
Status: Started (was: Unconfirmed)
Ah, I see why this is happening.

1. we complete an installation in BookmarkAppHelper, which calls AppBannerManager::OnInstall to fire the appinstalled event (https://cs.chromium.org/chromium/src/chrome/browser/extensions/bookmark_app_helper.cc?type=cs&q=chrome/browser/extensions/bookmark_app_helper.cc&sq=package:chromium&g=0&l=598)

2. AppBannerManager::OnInstall() calls ResetBindings(), since on an installation we want to ensure the beforeinstallpromptevent can't trigger another installation

3. But breaking the binding in (2) also means that when SendBannerAccepted() is called afterwards in AppBannerManagerDesktop::DidFinishCreatingBookmarkApp, it doesn't work. This is the method that resolves the userChoice object.

I think we can actually not reset bindings in (2) any more. That should fix this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 4 

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

commit 74ac297f3dc4367f94e6ce195b8d1cd0acd3976d
Author: Dominick Ng <dominickn@chromium.org>
Date: Thu Oct 04 08:21:31 2018

Call AppBannerManager::OnInstall in BookmarkAppHelper's finishing callback.

On desktop platforms, calling AppBannerManager::OnInstall in
BookmarkAppHelper::FinishInstallation resulted in a bug where the userChoice
promise on beforeinstalleventprompt was not resolved. This is because
OnInstall() clears Mojo bindings, wiping out the connection between
AppBannerManager and the beforeinstallpromptevent in the renderer.

This CL moves the call to AppBannerManager::OnInstall from
BookmarkAppHelper::FinishInstallation to BookmarkAppHelper::callback_. For
installations from app banners on desktop, this is
AppBannerManagerDesktop::DidFinishCreatingBookmarkApp; for installations
from the menu, it is extensions::TabHelper::FinishCreateBookmarkApp. This
means that for banners, OnInstall can be called after resolving the
userChoice promise, fixing the bug.

Tests are refactored and a new AppBannerManagerDesktopBrowserTest is
introduced to test the events are fired as expected after installation.

BUG= 890848 

Change-Id: I26a122e261ad1104af69443d9230e5458e271d24
Reviewed-on: https://chromium-review.googlesource.com/c/1256392
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596544}
[modify] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/browser/banners/app_banner_manager_browsertest.cc
[add] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/browser/banners/app_banner_manager_browsertest_base.cc
[add] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/browser/banners/app_banner_manager_browsertest_base.h
[modify] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/browser/banners/app_banner_manager_desktop.cc
[modify] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/browser/banners/app_banner_manager_desktop.h
[add] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/browser/banners/app_banner_manager_desktop_browsertest.cc
[modify] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/browser/extensions/bookmark_app_helper.cc
[modify] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/browser/extensions/tab_helper.cc
[modify] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/test/BUILD.gn
[modify] https://crrev.com/74ac297f3dc4367f94e6ce195b8d1cd0acd3976d/chrome/test/data/banners/main.js

Comment 6 by dominickn@chromium.org, Oct 8

Status: Fixed (was: Started)
►

Sign in to add a comment