fix(service-worker): properly handle invalid hashes in all scenarios by alxhub · Pull Request #21288 · angular/angular

alxhub · 2018-01-03T19:26:20Z

When the SW fetches URLs listed in a manifest with hashes, it checks
the content hash against the manifest to make sure it has the correct
version of the URL. In the event of a mismatch, the SW is supposed to
consider the manifest invalid, and avoid using it. There are 3 cases
to consider by which this can happen.

Case 1: during the initial SW installation, a manifest is activated
without waiting for every URL to be fully loaded. In the background,
every prefetch URL listed by the manifest is requested and cached.
One such prefetch request could fail the hash test, and cause the
manifest to be treated as invalid. In such a case, the SW should
enter a state of EXISTING_CLIENTS_ONLY, as the latest manifest is
invalid.

This case works today.

Case 2: during the initial SW installation, as in Case 1, a manifest
is activated without waiting for each URL to fully load. However,
it's possible that the application could request a URL with a bad
hash before background initialization tries to load that URL. This
happens if, for example, the application has a broken index.html.

In this case, the SW should enter a state of EXISTING_CLIENTS_ONLY,
and serve the request from the network instead.

What happens today is that the internal error escapes the SW and
is returned as a rejected Promise to respondWith(), causing a
browser-level error that the site cannot be loaded, breaking the
site.

This change allows the SW to detect the error and enter the correct
state, falling back on the network if needed.

Case 3: during checkForUpdate(), the SW will try to fully cache the
new update before making it the latest version. Failure here is
complicated - if the page fails to load due to transient network
conditions (timeouts, 500s, etc), then it makes sense to continue
serving the existing cached version, and attempt to activate the
update on the next cycle.

If the page fails due to non-transient conditions though (400 error,
hash mismatch, etc), then the SW should consider the updated
manifest invalid, and enter a state of EXISTING_CLIENTS_ONLY.

Currently, all errors are treated as transient.

This change causes the SW to treat all errors during updates as
non-transient, which can cause the SW to unnecessarily enter a
safe mode. A future change can allow the SW to remain in normal mode
if the error is provably transient.

mary-poppins · 2018-01-03T19:37:14Z

You can preview 66c89d6 at https://pr21288-66c89d6.ngbuilds.io/.

mary-poppins · 2018-01-03T19:37:27Z

You can preview c18158a at https://pr21288-c18158a.ngbuilds.io/.

gkalpak

Is it intentional that this is an Error and not a SwCriticalError?

gkalpak · 2018-01-04T17:54:08Z

Missing semicolon 😃

alxhub · 2018-01-08T20:28:41Z

And yes, it's intentional that the other one is an error. I actually don't think that condition can be triggered, because a not-ok response would have already been converted to an error further down in the depths of fetching.

When the SW fetches URLs listed in a manifest with hashes, it checks the content hash against the manifest to make sure it has the correct version of the URL. In the event of a mismatch, the SW is supposed to consider the manifest invalid, and avoid using it. There are 3 cases to consider by which this can happen. Case 1: during the initial SW installation, a manifest is activated without waiting for every URL to be fully loaded. In the background, every prefetch URL listed by the manifest is requested and cached. One such prefetch request could fail the hash test, and cause the manifest to be treated as invalid. In such a case, the SW should enter a state of EXISTING_CLIENTS_ONLY, as the latest manifest is invalid. This case works today. Case 2: during the initial SW installation, as in Case 1, a manifest is activated without waiting for each URL to fully load. However, it's possible that the application could request a URL with a bad hash before background initialization tries to load that URL. This happens if, for example, the application has a broken index.html. In this case, the SW should enter a state of EXISTING_CLIENTS_ONLY, and serve the request from the network instead. What happens today is that the internal error escapes the SW and is returned as a rejected Promise to respondWith(), causing a browser-level error that the site cannot be loaded, breaking the site. This change allows the SW to detect the error and enter the correct state, falling back on the network if needed. Case 3: during checkForUpdate(), the SW will try to fully cache the new update before making it the latest version. Failure here is complicated - if the page fails to load due to transient network conditions (timeouts, 500s, etc), then it makes sense to continue serving the existing cached version, and attempt to activate the update on the next cycle. If the page fails due to non-transient conditions though (400 error, hash mismatch, etc), then the SW should consider the updated manifest invalid, and enter a state of EXISTING_CLIENTS_ONLY. Currently, all errors are treated as transient. This change causes the SW to treat all errors during updates as non-transient, which can cause the SW to unnecessarily enter a safe mode. A future change can allow the SW to remain in normal mode if the error is provably transient.

mary-poppins · 2018-01-08T20:40:12Z

You can preview 12de6ca at https://pr21288-12de6ca.ngbuilds.io/.

gkalpak · 2018-01-09T19:00:52Z

LGTM 👍
@alxhub, leaving it to you to apply the appropriate target and action labels when ready 😃

alexeagle · 2018-01-09T22:15:38Z

@kara no targets affected in google3 🍏

IgorMinar · 2018-01-09T23:57:49Z

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+export class SwCriticalError extends Error { readonly isCritical: boolean = true; }


Can you please check with @mhevery that this is ok? in the past we had problems when we tried to extend from the Error class.

In ES5 extending Error is broken and should not be done. In ES6 extending error works fine as long as the browser is executing ES6 class construct.

Since the SW runs entirely in ES6, I think this is safe to do.

IgorMinar · 2018-01-09T23:58:20Z

Otherwise this LGTM

…21288) When the SW fetches URLs listed in a manifest with hashes, it checks the content hash against the manifest to make sure it has the correct version of the URL. In the event of a mismatch, the SW is supposed to consider the manifest invalid, and avoid using it. There are 3 cases to consider by which this can happen. Case 1: during the initial SW installation, a manifest is activated without waiting for every URL to be fully loaded. In the background, every prefetch URL listed by the manifest is requested and cached. One such prefetch request could fail the hash test, and cause the manifest to be treated as invalid. In such a case, the SW should enter a state of EXISTING_CLIENTS_ONLY, as the latest manifest is invalid. This case works today. Case 2: during the initial SW installation, as in Case 1, a manifest is activated without waiting for each URL to fully load. However, it's possible that the application could request a URL with a bad hash before background initialization tries to load that URL. This happens if, for example, the application has a broken index.html. In this case, the SW should enter a state of EXISTING_CLIENTS_ONLY, and serve the request from the network instead. What happens today is that the internal error escapes the SW and is returned as a rejected Promise to respondWith(), causing a browser-level error that the site cannot be loaded, breaking the site. This change allows the SW to detect the error and enter the correct state, falling back on the network if needed. Case 3: during checkForUpdate(), the SW will try to fully cache the new update before making it the latest version. Failure here is complicated - if the page fails to load due to transient network conditions (timeouts, 500s, etc), then it makes sense to continue serving the existing cached version, and attempt to activate the update on the next cycle. If the page fails due to non-transient conditions though (400 error, hash mismatch, etc), then the SW should consider the updated manifest invalid, and enter a state of EXISTING_CLIENTS_ONLY. Currently, all errors are treated as transient. This change causes the SW to treat all errors during updates as non-transient, which can cause the SW to unnecessarily enter a safe mode. A future change can allow the SW to remain in normal mode if the error is provably transient. PR Close #21288

angular-automatic-lock-bot · 2019-09-13T03:52:58Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jan 3, 2018

alxhub force-pushed the sw-mismatch-bug branch from c18158a to 66c89d6 Compare January 3, 2018 19:28

alxhub requested a review from gkalpak January 3, 2018 21:41

mhevery added the area: service-worker Issues related to the @angular/service-worker package label Jan 4, 2018

gkalpak reviewed Jan 4, 2018

View reviewed changes

alxhub force-pushed the sw-mismatch-bug branch from 66c89d6 to 12de6ca Compare January 8, 2018 20:32

gkalpak approved these changes Jan 9, 2018

View reviewed changes

alxhub added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jan 9, 2018

IgorMinar reviewed Jan 9, 2018

View reviewed changes

kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jan 10, 2018

kara assigned alxhub Jan 10, 2018

alxhub added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 10, 2018

alexeagle closed this in 3951098 Jan 10, 2018

manfredsteyer mentioned this pull request Apr 30, 2018

Service Worker: Hash Mismatch #23613

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019

Conversation

alxhub commented Jan 3, 2018

Uh oh!

mary-poppins commented Jan 3, 2018

Uh oh!

mary-poppins commented Jan 3, 2018

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

gkalpak Jan 4, 2018

Choose a reason for hiding this comment

Uh oh!

alxhub Jan 8, 2018

Choose a reason for hiding this comment

Uh oh!

alxhub commented Jan 8, 2018

Uh oh!

mary-poppins commented Jan 8, 2018

Uh oh!

gkalpak commented Jan 9, 2018

Uh oh!

alexeagle commented Jan 9, 2018

Uh oh!

IgorMinar Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

mhevery Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

alxhub Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Jan 9, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 13, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants