Launching the SMS PWA from settings seems to have regressed |
|||||||||||||||
Issue descriptionOn Chrome HEAD right now, clicking "set up" for Android Messages in Better Together settings seems to fail to launch the PWA, but there are some weird caveats here. 1) It works fine when overriding the app url to an alternative URL. 2) The error state seems weird. It says that the app isn't installed, so it starts trying to install it (even though it *is* installed). Then, it gets an error code 1 installing, which is kAlreadyInstalled. How can it be both installed and not installed at the same time? Let's find out!
,
Sep 26
,
Sep 27
Did some more debugging and it seems that the installed app has no url handlers somehow. I'm hitting this early return when checking if the app matches the url: https://cs.chromium.org/chromium/src/chrome/common/extensions/api/url_handlers/url_handlers_parser.cc?rcl=459099e1137fdf6ac8362ffdc5901799070fcadc&l=42 For whatever reason, when installing the PWA from my localhost, this works correctly, but the sandbox url doesn't. Gio, do you have any idea why there might not be any url handlers for the PWA?
,
Sep 27
Talked with Jeremy offline. GetInstalledPwaForUrl is not a reliable way to know if an app installed through PendingAppManager is installed because the app might not have been installed as a PWA. To check if an app is installed we need a method that queries the WebAppExtensionIds map and then tries to retrieve the Extension for the app. I think eventually such a method should be in the AppRegistry; in the short term I think it would make sense to add such a function to WebAppProvider. Nigel, Ben, could either of you take this?
,
Sep 27
I think we already have it. Jeremy, can you try using BookmarkOrHostedAppInstalled? https://cs.chromium.org/chromium/src/chrome/browser/web_applications/extensions/bookmark_app_util.h?rcl=c38c4ec0dd3136222fffd6f20820cc919c8820f7&l=39
,
Sep 27
For that to work, the URL used during installation has to be the same as the manifest's start_url.
,
Oct 1
I think the bigger question with comment #4 is why the app wasn't being installed as a PWA in the first place. Under what circumstances might that happen? I also want to note that for some reason, this just started working correctly again for me after reboot. Azeem also mentioned that he was seeing some inconsistent behavior here. Was there any sort of migration issue for some of the recent PWA installation changes? I want to make sure we understand why this might have broken temporarily.
,
Oct 1
,
Oct 1
If you want to find out why a site isn't being recognized as a PWA you can go to inspect / Application tab, then click the manifest tab and there should be a "Add to homescreen" link. If you click that any errors with the site's PWA check will be listed in the console.
,
Oct 1
Thanks, Ben. I'll give that a shot the next time I see this. What's weird to me is that it started working without my changing anything at all about the PWA.
,
Oct 1
re #7: it's possible that the service worker took too long to load which caused us to detect the site as a non-PWA. Future attempts to install might succeed because the service worker is already loaded and installed. I think there is a significant chance that the app will not be installed as a PWA, so we should use BookmarkOrHostedAppInstalled instead of GetInstalledPwaForUrl.
,
Oct 1
Are there any other functional differences if the app isn't installed as a PWA? I just want to make sure we understand all the implications here. Also - what's the timeout for the serviceworker loading? In the case of this serviceworker, unfortunately, wiz generates a ton of JS and it can be pretty slow to load at times. Is bumping up that timeout an option at all?
,
Oct 1
The differences are really subtle (IMO, I shouldn't matter if we install it as a PWA or not). Here is a list of the differences: (Shortcut refers to non-PWAs). - Web Push notifications for installed PWAs open an Desktop PWA window; for shortcuts, they open a tab - When navigating to a URL covered by a Desktop PWA, we will show an intent picker to open the URL in the app. Shortcuts don’t have this option. - Context menu for links to sites covered by a Desktop PWA includes an option to open the link in a new Desktop PWA window - There is an option to open the current tab in a Desktop PWA if there is one installed for the site. Shortcuts don’t have this option. - Out of scope navigations in Desktop PWAs are redirected to a new tab; shortcuts bring down the address bar when navigating out of scope - Desktop PWAs are blocked from loading insecure content. Shortcuts can still load insecure content. On top of the differences being so subtle, we plan to implement an update system to convert Shortcuts to Desktop PWAs. So if we install a Shortcut today, it will eventually become a Desktop PWA. I don't think we have a target release for this system though.
,
Oct 2
Thanks for the details, Gio. I think all of these points aren't that big of a deal except that the first one seems pretty unfortunate for our SMS background notifications. Do you happen to know what the timeout value on installation is to be considered a PWA?
,
Oct 3
I experienced this a few minutes ago. When I followed the instructions in #9 and clicked on "Add to homescreen" nothing happened: nothing was printed in the console and it doesn't add the app. I patched in a couple additional log lines[1] that was suggested by ortuno@ and everything just started working again :/ [1]https://chromium-review.googlesource.com/c/chromium/src/+/1258663
,
Oct 3
#15: Confusingly, if nothing happens when you click that button, then the site is considered a PWA. #14: Talked to Dom about this. We actually have no timeout for this. We run the PWA check as soon as the page loads and don't wait for the Service Worker to be registered. Ben, Nigel: do you think you could add an option to BookmarkAppHelper to wait for a service worker as part of the installability check?
,
Oct 3
> add an option to BookmarkAppHelper Are you asking whether it's possible (and the 'right' place to modify the code) technically, or whether it's something we have spare time to look at? If it's about possibility, I'd say that you know BAH better than I do. If it's about spare time, Ben and I could discuss this tomorrow. Do we even know if racing with the service worker loading is the fundamental issue, or are we just guessing?
,
Oct 3
As for BookmarkOrHostedAppInstalled vs GetInstalledPwaForUrl, perhaps the answer is neither? Combining web_app::ExtensionIdsMap::LookupExtensionId with extensions::ExtensionRegistry::GetExtensionById might do it. The first half of that, web_app::ExtensionIdsMap::LookupExtensionId, is pretty closely associated with the PendingAppManager implementation.
Note that the map keys (URLs, or really, strings) must match exactly. IIUC android_sms_app_helper_delegate_impl.cc calls chromeos::android_sms::GetAndroidMessagesURL{,WithExperiments}(), with and without "WithExperiments". If those two functions return different URLs, then installing under one URL will not get picked up by passing the second URL to web_app::ExtensionIdsMap::LookupExtensionId.
,
Oct 3
Funny enough, it actually didn't work at all when I *was* trying to launch using the URL with the experiment URL params. Basically (for now) we need to install the PWA using the url that includes experiment params which actually serve the app manifest, etc. If we do that, though, when launching the app, it only works (sometimes) if we do *not* include those url params. In my debugging with Gio on this, basically it seemed like when things broke down, all of the checks in GetInstalledPwaForUrl were ok except that the Extension that should have matched had no url handlers (see comment #3). It seems like there are 2 issues here: The first is that the app sometimes isn't getting installed as a PWA for some reason. For that, Gio's asked in comment #16 about waiting for the serviceworker as part of the installability check. I'd still like to get to the bottom of this, but Jon's comment in #15 gives me some doubt that this is what's wrong here. The second issue is whether we should be working around the first problem by using some other method of launching the app besides retrieving it via GetInstalledPwaForUrl. IIUC, if we solve the first problem, this one will go away, right? I'm happy to use some other method here, but I want to make sure we're not papering over some deeper issue.
,
Oct 3
I haven't read everything on this bug, but a couple of points: * this does sound like it could be the waiting for SW installation. Note that this would only be a problem only the first time you load the page, after this the SW is installed. * can we just skip the whole installability check for this? We know it is a PWA. For built in things like this the behavior should be 100% reproducible.
,
Oct 3
Yeah, Ben, it does seem like it could just be happening the first time you load the page. The kicker is that this installation attempt will almost always be the first time you load the page! It's invisible to users and is done as part of OOBE or Better Together setup. There's no visible page load here. +1 to skipping the installability check if possible here. How might we force that?
,
Oct 3
Yup, that would be totally doable. The question is who has the spare cycles to do it :) (implementation wise: we retrieve the manifest as part of the installability check, so we still have to perform the installability check. We'll have to add an option to just ignore the result of the installability check. Similar to what we did for always installing a non-PWA. See [1] and [2] for an example of the change we need.) [1] https://chromium-review.googlesource.com/c/chromium/src/+/1226481/14/chrome/browser/extensions/bookmark_app_helper.h [2] https://chromium-review.googlesource.com/c/chromium/src/+/1226481/14/chrome/browser/extensions/bookmark_app_helper.cc
,
Oct 4
,
Oct 4
I filed a new bug for the proposed solution to this issue (since this bug has a lot of discussion in it). Issue 892013 . Assigning to Alan.
,
Oct 5
,
Oct 12
Alan's fix seemed to increase reliability in some cases, but I can still reproduce errors opening/installing the app by clearing storage for the domain and then trying to install the app. Azeem is working on getting new logs.
,
Oct 16
I'm also able to reproduce this issue after clearing storage and trying to re-install. Installation consistently fails with a kFailedUnknownReason status code. But strangely, the issue disappears after trying for a little while. I traced the issue down a little bit and from what I've investigated so far, it seems that it fails during icon download. When the installation fails, the WebContents::DownloadImage call here https://cs.chromium.org/chromium/src/chrome/browser/web_applications/components/web_app_icon_downloader.cc?rcl=0cecb6ce10b4c752f053ecbe9742c2e058b35a6f&l=52 does not call the DidDownloadFavicon callback here https://cs.chromium.org/chromium/src/chrome/browser/web_applications/components/web_app_icon_downloader.cc?rcl=0cecb6ce10b4c752f053ecbe9742c2e058b35a6f&l=100 instead only the DidFinishNavigation observer method here https://cs.chromium.org/chromium/src/chrome/browser/web_applications/components/web_app_icon_downloader.cc?rcl=0cecb6ce10b4c752f053ecbe9742c2e058b35a6f&l=125 is run. When the installation does succeed, the DidDownloadFavicon callback is indeed called and returns a 200 status code. I am not sure why this happen, but i'll try to trace it down a bit more tomorrow.
,
Oct 16
I wonder if there should be retry logic involved here to handle the inevitable scenario of network failure.
,
Oct 16
I think there are two possibilities: 1. There is a redirect which causes us to navigate while downloading the icon. Or 2. DidFinishNavigation is getting called after we've loaded the page in PendingBookmarkAppManager. You could add some logging to PendingBookmarkAppManager::DidFinishLoad and override DidFinishNavigation also in PBAM to check if DidFinishNavigation is called after the DidFinishLoad or if there is a redirect.
,
Oct 16
I captured the following logs for a case that failed. Please see my WIP CL https://crrev.com/c/1284436 with the log statements for reference. [19731:19731:1016/122744.536625:INFO:pending_bookmark_app_manager.cc(303)] PWATest:PBAM>> DidFinishNavigation netErrorCode = 0 wasServerRedirect = 0 url=https://android-messages.sandbox.google.com/?DefaultToPersistent=true ptr=0x15591d475700 [19731:19731:1016/122744.768395:INFO:pending_bookmark_app_manager.cc(263)] PWATest:PBAM>> DidFinishLoad [19731:19731:1016/122744.817398:INFO:bookmark_app_installation_task.cc(99)] PWATest:BAIT>> OnGetWebApplicationInfo web_app_info = 0x15591dfae580 [19731:19731:1016/122744.817510:INFO:bookmark_app_helper.cc(368)] PWATest:BAH>> Doing Installability check [19731:19731:1016/122744.839630:INFO:bookmark_app_helper.cc(397)] PWATest:BAH>> OnDidPerformaInstallableCheck for_installable_site=0 [19731:19731:1016/122744.839721:INFO:web_app_icon_downloader.cc(52)] PWATest:WAID>> Downloading iconhttps://ssl.gstatic.com/images/branding/product/1x/messages_96dp.png [19731:19731:1016/122744.949088:INFO:web_app_icon_downloader.cc(130)] PWATest:WAID>> DidFinishNavigation netErrorCode = 0 wasServerRedirect = 0 url=https://android-messages.sandbox.google.com/?DefaultToPersistent=true ptr=0x15591d475700 [19731:19731:1016/122744.949162:INFO:bookmark_app_helper.cc(455)] PWATest:BAH>> OnIconsDownloaded: Failed downloading app icons [19731:19731:1016/122744.949190:INFO:pending_bookmark_app_manager.cc(224)] PWATest:PBAM>> OnInstalled: result_code=2 [19731:19731:1016/122744.949213:INFO:pending_bookmark_app_manager.cc(247)] PWATest:PBAM>> Installation Finished: app_id.has_value=0 #28 I'm not sure if it's a network failure in this case. I printed out the navigation_handle netErrorCode and it seems to be 0=OK, so the page load in PBAM is working fine. I don't know if just the icon download is facing network issues, but trying to download just the image url outside of the installation works fine while the installation fails. #29 (1) I can't confirm whether there are any redirects. But if I just make a direct request for the image url, I don't see any redirects in the response. I also printed out navigation_handle->WasServerRedirect() value inside DidFinishNavigation method which seems to be 0=false. However, i don't think the DidFinishNavigation calls inside WebAppIconDownloader correspond to the image download. The url that's logged seems to be that of the page load. (2) I believe this is what's happening, but in a more confusing way. For the page load inside PBAM, DidFinishNavigation is indeed called before the DidFinishLoad. But for some reason it looks like DidFinishNavigation inside WebAppIconDownloader gets called again later with the same navigation_handle (you can see that the the ptr values are the same). If I comment out the last three lines in WebAppIconDownloader::DidFinishNavigation that clears the favicon_map and calls the callback with a success=false, the installation continues successfully.
,
Oct 16
Thanks for digging into this, Azeem! Over to Gio for next steps here.
,
Oct 17
Thanks for the logs. I was able to reproduce. Seems like the page does a couple of same document navigations after it finishes loading. I'll working on a patch to ignore same-document navigations in WebAppIconDownloader which should fix the bug.
,
Oct 18
Awesome, thanks Gio!
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a8226af635e2356a6b6048848e5f308e094c5f34 commit a8226af635e2356a6b6048848e5f308e094c5f34 Author: Giovanni Ortuño Urquidi <ortuno@chromium.org> Date: Thu Oct 18 03:44:11 2018 desktop-pwas: Ignore same-document navigations when retrieving icons Some pages could perform same-document navigations e.g. push/popState while we download icon. Since these navigations are still in the same document it should be safe to ignore them and continue with the installation. Changes WebAppIconDownloader to ignore same-document navigations and continue downloading the icons. Bug: 889660 Change-Id: I120ba6f38a5aa0a803fa82615cd8bd73412bbb67 Reviewed-on: https://chromium-review.googlesource.com/c/1287330 Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org> Reviewed-by: calamity <calamity@chromium.org> Cr-Commit-Position: refs/heads/master@{#600652} [modify] https://crrev.com/a8226af635e2356a6b6048848e5f308e094c5f34/chrome/browser/web_applications/components/web_app_icon_downloader.cc [modify] https://crrev.com/a8226af635e2356a6b6048848e5f308e094c5f34/chrome/browser/web_applications/components/web_app_icon_downloader_unittest.cc
,
Oct 18
Should be fixed now. The change is very small, should we ask to merge to 71?
,
Oct 18
Yep, thanks Gio!
,
Oct 19
Your change meets the bar and is auto-approved for M71. Please go ahead and merge the CL to branch 3578 manually. Please contact milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb6941aec2a2abfc26bc0597435a10e4ab7f4ec5 commit eb6941aec2a2abfc26bc0597435a10e4ab7f4ec5 Author: Giovanni Ortuño Urquidi <ortuno@chromium.org> Date: Fri Oct 19 18:19:33 2018 desktop-pwas: Ignore same-document navigations when retrieving icons Some pages could perform same-document navigations e.g. push/popState while we download icon. Since these navigations are still in the same document it should be safe to ignore them and continue with the installation. Changes WebAppIconDownloader to ignore same-document navigations and continue downloading the icons. Bug: 889660 Change-Id: I120ba6f38a5aa0a803fa82615cd8bd73412bbb67 Reviewed-on: https://chromium-review.googlesource.com/c/1287330 Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org> Reviewed-by: calamity <calamity@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#600652}(cherry picked from commit a8226af635e2356a6b6048848e5f308e094c5f34) Reviewed-on: https://chromium-review.googlesource.com/c/1292000 Reviewed-by: Jeremy Klein <jlklein@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#162} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/eb6941aec2a2abfc26bc0597435a10e4ab7f4ec5/chrome/browser/web_applications/components/web_app_icon_downloader.cc [modify] https://crrev.com/eb6941aec2a2abfc26bc0597435a10e4ab7f4ec5/chrome/browser/web_applications/components/web_app_icon_downloader_unittest.cc
,
Oct 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb6941aec2a2abfc26bc0597435a10e4ab7f4ec5 Commit: eb6941aec2a2abfc26bc0597435a10e4ab7f4ec5 Author: ortuno@chromium.org Commiter: jlklein@chromium.org Date: 2018-10-19 18:19:33 +0000 UTC desktop-pwas: Ignore same-document navigations when retrieving icons Some pages could perform same-document navigations e.g. push/popState while we download icon. Since these navigations are still in the same document it should be safe to ignore them and continue with the installation. Changes WebAppIconDownloader to ignore same-document navigations and continue downloading the icons. Bug: 889660 Change-Id: I120ba6f38a5aa0a803fa82615cd8bd73412bbb67 Reviewed-on: https://chromium-review.googlesource.com/c/1287330 Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org> Reviewed-by: calamity <calamity@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#600652}(cherry picked from commit a8226af635e2356a6b6048848e5f308e094c5f34) Reviewed-on: https://chromium-review.googlesource.com/c/1292000 Reviewed-by: Jeremy Klein <jlklein@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#162} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by ortuno@chromium.org
, Sep 26