desktop-pwas: Wait for Service Worker to be registered for default installed windows apps. |
|||
Issue descriptionWe currently perform the following steps when installing a default app: 1. Create a new background WebContents and load the installation URL 2. Wait for the page to load 3. Retrieve metadata 4. Install based on retrieved metadata 5. Close the WebContents Pages don't usually start registering a service worker until the page has loaded[1] so apps have between steps 3. and 5. to register the service worker. There is a chance they can't register the service worker by the time we close the WebContents, in which case the app wouldn't work offline the first time it launches. Proposed solution: After we've retrieved the metadata and installed the app, wait for the service worker to be registered before closing the WebContents. [1] https://developers.google.com/web/fundamentals/primers/service-workers/registration#improving_the_boilerplate
,
Nov 16
That would just involve setting wait_for_worker to true in the invocation of InstallableManager::GetData right? Perhaps with a sensible timeout?
,
Nov 16
Are you proposing we set the wait_for_worker flag when retrieving the manifest? I don't think we want to do that. We want to install the app as soon as possible. If for some reason a default app never register a service worker, it should still be installed.
,
Nov 16
Functionally, both solutions still hold up the installation queue (because we only use one WebContents in PendingBookmarkAppManager?), so as soon as we have n > 1 default installed apps, it feels like waiting for the service worker before install or after install are much the same (in the latter case, you get one app installed, and then wait until SW is registered or a timeout fires before proceeding with the queue; in the former, you wait first, then get your app installed). Applying a timeout on the service worker waiting prior to installation is what Android currently does, so doing that here as well maximises commonality.
,
Nov 16
I think what we should do for default installed apps is: - install immediately - do not fail or otherwise change install if we can't determine pwa'ness - make sure the SW loads, with a reasonable (but long) timeout If this is wait_for_sw will do, great, but I suspect it will at least stop the install from happening immediately.
,
Nov 16
Discussed with dominickn@ offline. There are basically two options: 1. Block installation on service worker registration; if no service worker is registered after a small timeout, e.g. 10 seconds, continue the installation. This would just require us adding the option to wait for a service worker with a timeout to InstallableManager. The benefits of this approach are that the change is small, we don't potentially have WebContents alive forever in cases where the apps misbehave, and matches what we do on Android. The downside is that we could potentially increase the installation time if apps take a long time to register their service worker. 2. Install default apps without waiting for the service worker to be registered, but keep the WebContents open until the Service Worker gets registered. This would require us using separate WebContents for each default app installation, which would increase the complexity of PendingAppManager and could potentially increase CPU and memory usage during startup. The benefits of this approach is that the apps are installed as quickly as possible and they don't delay installation of other apps.
,
Nov 16
Ben, I think you are thinking of solution 2, which has the downside of being a much bigger change and could potentially increase resource usage. What do you think of solution 1?
,
Nov 16
My two concerns with solution 1, which may just be me not understanding it well, are: 1. regular default app installs can be delayed, but i don't want to delay the android messages install as it can happen as a result of user input 2. i want to have a fairly large timeout so that sws are cached even when network is poor etc. I think 10 seconds will result in lots of web apps not working offline the first time they are run.
,
Nov 16
Wouldn't solution 2 also potentially hold up the Android Messages install? For instance, the Chromebook starts up, the users proceeds through OOBE, and then is prompted to set up multi device. While that happens, we've begun installing the current queue of default apps. If the default installed apps queue is occupying all of the background WebContents we have available, and they are all sitting there waiting for 5 minutes for SWs to appear, Messages install is just going to sit there and wait until one of the timers finishes (up to 5 minutes). I think if we *know* a default app is going to be a PWA, and we expect them (or influence them) to ensure their service worker registrations happen ASAP, solution 1 with a 10/20/30 second timeout or so is quite serviceable, because it puts an upper bound on the worst case scenario for Messages install waiting time. WDYT?
,
Nov 16
I am not saying to do option 2, I am just giving requirements: - android sms should not be delayed when installed as a result of user action - setting up a chromebook on a flaky / slow network should not result in icons that don't work offline.
,
Nov 19
These are good points. I also worry about resource consumption: WebContents can consume hundreds of MB of RAM pretty easily. Can we leave a background tab open and consuming a large chunk of RAM for ~minutes when many Chromebooks are already quite RAM limited? And can we do that during first run (a fairly critical time)?
,
Nov 19
Yep, we should make sure we don't blow out resource usage. I don't think this is a hard problem given that - usually the caching will be fast - there is no real time limit on how long it takes. But I think if you're trying to fit it into the existing code paths / processes (i.e. into the install flow) the requirements don't add up. If this is some kind of post install process I think it gets a lot easier.
,
Nov 20
Bumping priority and setting a milestone to make sure this gets fixed soon. This is an important bug to fix.
,
Nov 20
+loyso (relevant to refactoring) |
|||
►
Sign in to add a comment |
|||
Comment 1 by ortuno@chromium.org
, Nov 16