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

Fix refresh loop on Firefox #5964

Closed
wants to merge 5 commits into from
Closed

Conversation

p5nbTgip0r
Copy link
Contributor

@p5nbTgip0r p5nbTgip0r commented Sep 8, 2020

I want to preface this with a warning that I have no experience with service workers, so I don't fully understand whether this fix is a bad implementation or if it breaks something. I'm open to suggestions if there is another way I should be implementing this.
This PR should fix #5943.

Explanation of changes
Chromium based browsers don't seem to trigger `updatefound` or `statechange` events when the first service worker is registered/installed. Firefox does trigger these events in that circumstance. My guess for why this happens is due to Firefox not installing the service worker right away (which I've observed Chromium to do, hence the lack of update and state events). When it gets around to installing and activating the service worker, we have an event listener for `updatefound` (which is called when it starts installing a worker), which then registers a listener for `statechange` events. The `statechange` events are emitted as the state of our service worker goes from `installed`, to `activating`, and finally `activated`. In each of these events, we trigger a tab refresh in an attempt to update our service worker to the latest available instance, causing the refresh loop.

To fix this issue, I added a check for the current state the worker is on. If the state is installed, then I proceed onto checking whether navigator.serviceWorker.controller is not null. If there is an existing worker which is currently active, then this property will not be null. Using this, I'm able to check whether our service worker is the first one to be activated or if this is going to replace another service worker which is active currently. If there's a worker active already, I copied the original design of forcing a page refresh (along with a check to fix refresh loops when using the Update on reload checkbox in Chromium's DevTools). And if there is no active worker, I just log a simple message indicating the service worker was cached.


With this fix in place I uncovered another issue which I've managed to reproduce on 13.0.1 (I'm not certain on how far back this issue goes). I realised shortly after that Firefox was not succesfully activating the service worker at all. As shown in this screenshot, the worker enters a redundant state after it is registered. Per the W3C implementation recommendation,, this is a result of any of the following:

I believe the installation was failing on step 10.2.1 under the W3C recommendation for the installation algorithm.

I managed to trace where it was failing down to the precache() function in service-worker.js, but it's actually caused by this bit of code in the index.html:

<div class="audio alarms">
  <audio src="audio/alarm.mp3" preload="auto" loop="true" class="alarm mp3" type="audio/mp3"></audio>
  <audio src="audio/alarm2.mp3" preload="auto" loop="true" class="urgent alarm2 mp3" type="audio/mp3"></audio>
</div>

The preload="auto" attributes allow these audio files to be fetched and cached by the browser while it's loading the page. This would not be an issue typically, but the service-worker.js has /audio/alarm.mp3 and /audio/alarm2.mp3 in the list of assets to cache interally for the service worker to serve. It does this by using the addAll() function in its Cache instance (docs) which takes an array of URLs and stores the responses for all of them in itself. This function will attempt to use the browser's cache wherever possible, and is where the issue starts to surface. The add() and addAll() functions in Cache do not accept 'Partial Content' responses for storage and the browser fetches the alarm mp3s upon page load with the Range header set, causing the webserver to give 'Partial Content' responses.
In essence, if the browser loads these alarm files before our service worker, then our precache() function will fail to run.

To fix this, I reimplemented the addAll() and created Requests for each asset URL with no-store as the cache setting in order to avoid the browser cache from having any effect on the service worker cache. In addition, I also added some code to strip Range headers from the fetch function to prevent any future requests which might trigger a 'Partial Content' response from failing.

In short: I added checks to ensure the update events regarding the state of the service worker is worthy of a page refresh and fixed a bug regarding running the service worker on Firefox which involved the browser caching 'Partial Content' responses and being given to the service worker when it tried to cache assets inside its own cache. The cache which service workers use do not support caching 'Partial Content' responses directly, so this would result in an error and the service worker crashing immediately upon installation.

I've only tested this on Firefox (versions 81.0b7 and 79.0) and Chromium (v87.0.4243.0), so more testing may be necessary.

@sulkaharo
Copy link
Member

Ping @pazaan who's also looking at this

@sulkaharo
Copy link
Member

Merging through #5970

@sulkaharo sulkaharo closed this Sep 9, 2020
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

Successfully merging this pull request may close these issues.

2 participants