angular / angular

Deliver web apps with confidence πŸš€
https://angular.dev
MIT License
95.97k stars 25.36k forks source link

Video does not always load its source when using service worker #44420

Closed faileon closed 2 years ago

faileon commented 2 years ago

Which @angular/* package(s) are the source of the bug?

service-worker

Is this a regression?

No

Description

Hello everyone, I've been chasing a bug last few days that I believe is related to using service worker. I believe its related to service worker because:

  1. On localhost (ng serve) I do not have these issues.
  2. If I chose to Bypass for network in the DevTools>Application panel in production, the issue also disappears

When videos are loading their source via service worker, they will result in source not found error and the entire media player will fail to work.

My video component is extremely simple

<video preload="metadata"
       [poster]="poster"
       muted
       controls
       #videoElement
>
  <source #sourceElement (error)="onSourceError($event)" [src]="src" type="video/mp4">
</video>

The event from onSourceError callback does not provide much valuable information about the problem.

My ngsw-config.json

{
  "$schema": "./node_modules/@angular/service-worker/config/schema.json",
  "index": "/index.html",
  "assetGroups": [
    {
      "name": "app",
      "installMode": "prefetch",
      "resources": {
        "files": [
          "/favicon.ico",
          "/index.html",
          "/manifest.webmanifest",
          "/*.css",
          "/*.js"
        ]
      }
    },
    {
      "name": "assets",
      "installMode": "lazy",
      "updateMode": "prefetch",
      "resources": {
        "files": [
          "/assets/**",
          "/*.(eot|svg|cur|jpg|png|webp|gif|otf|ttf|woff|woff2|ani|mp4)"
        ]
      }
    }
  ],
  "dataGroups": [
    {
      "name": "firebase-storage",
      "urls": [
        "https://firebasestorage.googleapis.com/**",
        "https://fonts.googleapis.com/**",
        "https://fonts.gstatic.com/**"
      ],
      "cacheConfig": {
        "maxSize": 1000,
        "maxAge": "15d",
        "strategy": "freshness",
        "timeout": "0s"
      }
    }
  ]
}

Example:

  1. Go to my site: https://tekken.guru (select bunch of different characters)
  2. Stick around long enough for service worker to do its magic
  3. Observe the media player being hopeless in playing the videos and crashing
  4. If you clear the service workers cache (ctrl+f5, Bypass for network or other means) the videos will work again briefly.

On this picture you can see the result of the error having state-no-source alt text

I have tested this on multiple devices and have multiple people reporting me the same issue happening on my site.

Please provide a link to a minimal reproduction of the bug

Difficult to provide when service worker runs only on https... You can see it live on my production site: https://tekken.guru/characters

Please provide the environment you discovered this bug in (run ng version)

Angular CLI: 13.0.4
Node: 14.17.1
Package Manager: npm 6.14.13
OS: win32 x64

Angular: 13.0.3
... animations, cdk, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router
... service-worker

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1300.4
@angular-devkit/build-angular   13.0.4
@angular-devkit/core            13.0.4
@angular-devkit/schematics      13.0.4
@angular/cli                    13.0.4
@angular/fire                   6.1.5
@angular/flex-layout            13.0.0-beta.36
@schematics/angular             13.0.4
rxjs                            6.5.5
typescript                      4.4.4

Anything else?

No response

faileon commented 2 years ago

Could this be related? https://jakearchibald.com/2018/i-discovered-a-browser-bug/#media-via-a-service-worker-didnt-quite-work

gkalpak commented 2 years ago

Sorry for the late response, @faileon πŸ˜‡ Somehow, this slipped through the cracks. That's a very cool website btw 🀘

That blogpost you shared is about a security vulnerability, which is not directly related with the issue here, but one thing that is mentioned in it is related I believe: Range request.

Currently, the Angular ServiceWorker does not support handling range requests, which I believe is what is causing your issue here. There is some discussion about it in #25865. (There is a potential fix that we could try to incorporate, as mentioned in https://github.com/angular/angular/issues/25865#issuecomment-421925411. If anyone feels strongly about it, I would be happy to help with/look at a pull request :wink:)

In the meantime, one work-around would be to bypass the SW for these specific requests. See Bypassing the SW. This would prevent them from working offline, but the rest of the app would and this is still better than not working at all :smiley:

I am going to close this as a duplicate of #25865, but feel free to continue the discussion either there or here.

faileon commented 2 years ago

@gkalpak Thank you for the response, I appreciate it.

Yes the blogpost guided me to a right path about the range request, a pity that ngsw currently does not handle it. I would swear it was not halting the video player before, perhaps it chose to use the network resource instead of cached one previously?

If I find some time during the holidays or after, I'd love to make a PR and resolve this. In the meantime I chose to ignore video files from the service worker and that did the trick, thanks for the suggestion.

faileon commented 2 years ago

I was looking at the implementation from workbox and sw code from @philnash as found in the mentioned issue and I would like to implement a solution for ngsw to properly handle single (and possible multi) part range requests.

However I am a bit lost in the ngsw source code. From what I understood the driver is the main entry point, which then asks AppVersion or network directly for the resource. AppVersion forwards it to either data group or asset group to handle the request and takes in consideration the chosen strategies (performance, freshness,) etc.. I assume the correct place to implement the range requests while keeping the ngsw strategies intact is both in data and assets classes?

When it comes to caching the request should one alter the retrieving/saving logic for range requests? Does it still make sense to cache per request, when there can be multiple requests containing various byte ranges? Especially when it comes to seeking in videos, this could quickly pollute the cache with multiple requests for single file. Would it make more sense to cache the response per resource (omit the range header) and update/insert the bytes accordingly in the cache?

Can you please help me clarify @gkalpak ? Thanks :]

gkalpak commented 2 years ago

Thx, @faileon! (Sorry, I missed this message, because it is on a closed issue.)

Everything you said about the source code is correct. Regarding the implementation:

  1. I would implement this as one or more helper functions that will be used by both asset- and data-groups (to avoid code/logic duplication).
  2. From what I can tell by looking at the Workbox and SW code implementations, it seems like we can get away with a simplified implementation that works as follows:
    • When we receive a range request, we check the cache for a response.
    • If we find a cached response, it will be a full response (no partial content) and we can generate an appropriate 206 (partial content) response from that.
    • If there is no cached response, we can do one of the two (whatever we decide - both options have pros and cons):
      • Option 1: Make a regular request to the server (no range request), cache the full response and then generate a 206 (partial content) response from it.
      • Option 2: Make two simultaneous requests to the server, a range request (which will be served to the client) and a regular request (which will be saved to the cache for future use).

Does that answer your questions? WDYT about my suggested implementation?

angular-automatic-lock-bot[bot] commented 2 years ago

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.