Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

should update on navigation or the subsequent updatefound event be delayed until document DOM is loaded? #788

Closed
wanderview opened this issue Nov 21, 2015 · 12 comments
Milestone

Comments

@wanderview
Copy link
Member

Recently we got a bug report for the following demo:

https://adriftwith.me/sandbox/service_worker_reload/go.html

When you refresh in chrome you always get the "new ServiceWorker installed" text. Usually it seems after some delay.

In firefox, however, you usually don't get the text, but sometimes you do.

I believe this is happening because we trigger the update immediately after the fetch event resolves. This means the update can complete before the document script has a chance to attach to the registration objects. So it never sees the updatefound events.

I agree with the developers that it would be nice to reliably get updatefound events if you are updating on navigation.

Should the spec be changed to delay navigation updates or the navigation updatefound event until the target document has completed loading?

@wanderview wanderview added this to the Version 1 milestone Nov 21, 2015
@wanderview
Copy link
Member Author

In this gecko bug I am modifying our code to delay update until a controlled document reaches DOMContentLoaded.

https://bugzilla.mozilla.org/show_bug.cgi?id=1226443

@wanderview
Copy link
Member Author

Presumably chrome is delaying for such a long time so that it the update does not regression load times.

@mfalken
Copy link
Member

mfalken commented Nov 24, 2015

Yes Chrome triggers update after a one second delay after load completes, so it doesn't contend with the network for loading the page.

@jungkees
Copy link
Collaborator

@annevk what'd be a good way to spec this requirement? Setting a fixed amount of delay in the spec doesn't seem like a good idea. Also, the target resource can be a worker, a shared worker as well as a document, so no unified concept can be referenced to get the right timing. Any idea?

@annevk
Copy link
Member

annevk commented Nov 30, 2015

Depends on the semantics you want I suppose. (Note that workers cannot be navigated so I'm not sure how the same problem applies to them.)

@jungkees
Copy link
Collaborator

jungkees commented Dec 1, 2015

Soft Update is triggered for all the non-subresource requests (whose destination is not "serviceworker" of course).

@wanderview
Copy link
Member Author

@mattto What does chrome do if the client has been closed when the update timer triggers? Do you go ahead with the update or cancel the update?

@mfalken
Copy link
Member

mfalken commented Dec 2, 2015

We continue with the update, it's scheduled on the SW entity itself rather than attached to the client.

For that matter, opening lots of tabs really quickly will only trigger one update in Chrome.

@jakearchibald
Copy link
Contributor

I'm not against delaying the update a bit for performance reasons (or at least suggest it can be low priority), but I don't think it fixes the original issue.

Imagine:

  • Functional event lands in SW
  • More than 24hrs since update, so update triggered, updated SW found, becomes installing
  • User navigates to in-scope page
  • "updatefound" listener added
  • Update triggered, SW is byte identical so it's ignored
  • SW moves from "installing" to "installed"

Even with the delay, the listener misses the new SW. The only safe way know when a SW becomes installed is to listen for "updatefound" but also track any current reg.installing. Eg https://github.com/jakearchibald/wittr/blob/cache-avatars/public/js/main/IndexController.js#L45.

@wanderview
Copy link
Member Author

I think that case is a race between two different operations. I'm not sure devs would expect to duplicate those conditions often.

The update on navigate, though, is a very repeatable use case.

FWIW, in gecko we have implemented a 1 second delay after document load event. For non-navigation events that trigger an update we fire them 1 second after the event. One an update check is scheduled, any further update checks are coalesced into that currently scheduled timer. We don't push back the timer, though, so that we are guaranteed to fire the update regardless of how many events the SW is receiving.

I think this is close to what chrome is doing, but not 100% sure.

@mfalken
Copy link
Member

mfalken commented Jan 22, 2016

Chrome's implementation tries to schedule the updates after the worker stops:

  • Schedule an update 1 second after page load for a navigation in a SW's scope.
  • Whenever the active worker stops, if it hasn't attempted an update in 24 hours, and no update is scheduled, schedule an update.
  • If the active worker has been running without stopping for 5 minutes, if hasn't attempted an update in 24 hours, and no update is scheduled, schedule an update.

Scheduling an update pushes back the timer.

I implemented this before the spec required Soft Updates at the end of each event. I wanted to wait for the worker to stop to avoid situations where the worker triggers an update and skipWaiting() kills it while it's in middle of an event (Chrome doesn't wait for the active worker to be finish in-progress requests... that's a bug). But as it's approximately the same as the spec (the rough result is a Soft Update approx every 24 hours), I probably wouldn't expend much effort to get it to exactly match unless there's a real benefit for devs.

@jakearchibald
Copy link
Contributor

F2F resolution: Add a note to the spec to suggest delaying triggering an update until after "DOMContentLoaded" of the associated page. If it's a worker, don't update until the after the worker script has run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants