Bugfix: TypeError: Cannot read properties of null… payload.byteLength (timeline-controller) #4902

mtoczko · 2022-09-13T00:56:22Z

This PR will...

Adding error handling from webCryptoDecrypt to SubtitleStreamController.
onWebCryptoError: fixes reading data from softwareDecrypt

Why is this Pull Request needed?

Prevents errors:
Uncaught (in promise) TypeError: Cannot read properties of null (reading 'byteLength')
Uncaught (in promise) TypeError: payload is null

Are there any points in the code the reviewer needs to double check?

No

Resolves issues:

#4898

Checklist

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
API or design changes are documented in API.md

… (reading 'byteLength')``

robwalch · 2022-09-13T01:34:23Z

Hi @mtoczko,

Thanks for the fixes. I missed this one before cutting v1.2.3. However, I closed #4898 as I was not able to reproduce the payload exceptions using the stream or steps in the AES-128 VTT loop loading issue. Is this intermittent or are there additional steps needed to repro?

robwalch · 2022-09-13T01:38:20Z

src/controller/timeline-controller.ts

@@ -473,7 +473,7 @@ export class TimelineController implements ComponentAPI {
 const { initPTS, unparsedVttFrags } = this;
 if (frag.type === PlaylistLevelType.SUBTITLE) {
 // If fragment is subtitle type, parse as WebVTT.
- if (payload.byteLength) {
+ if (payload?.byteLength) {


I would argue that if payload is undefined (or null) then onFragLoaded should not be called. My guess is that this is not an issue with FRAG_LOADED but with the FRAG_DECRYPTED or unparsedVttFrags path(s)? Or is the progressive config option enabled?

In either case, same logic applies: both event types expect payload to be defined so I'd like to know the root cause for this.

Hi @robwalch
The error does not always occur, to me it was able to appear immediately after starting the stream (#4898).
You're right, it's an error with WebCrypto.

By the way, do you know the reason why subtitle-stream-controller does not use ENDED state?
Because I don't see any reason to keep trying to load the last fragment. (VOD stream)

Show all details

base-stream-controller.ts:1395 [log] > [subtitle-stream-controller]: IDLE->KEY_LOADING base-stream-controller.ts:1395 [log] > [subtitle-stream-controller]: KEY_LOADING->IDLE base-stream-controller.ts:612 [log] > [subtitle-stream-controller]: Loading fragment 1 cc: 0 of [0-1] track: 0, target: 60 base-stream-controller.ts:1395 [log] > [subtitle-stream-controller]: IDLE->FRAG_LOADING base-stream-controller.ts:373 [log] > [subtitle-stream-controller]: Loaded fragment 1 of level 0 decrypter.ts:171 [warn] > [decrypter.ts]: WebCrypto Error, disable WebCrypto API: Error onWebCryptoError @ decrypter.ts:171 (anonymous) @ decrypter.ts:166 Promise.catch (async) webCryptoDecrypt @ decrypter.ts:154 _handleFragmentLoadComplete @ subtitle-stream-controller.ts:314 (anonymous) @ base-stream-controller.ts:378 Promise.then (async) _loadFragForPlayback @ base-stream-controller.ts:354 loadFragment @ base-stream-controller.ts:329 loadFragment @ subtitle-stream-controller.ts:420 onKeyLoaded @ base-stream-controller.ts:276 emit @ index.js:203 emit @ hls.ts:253 trigger @ hls.ts:261 onKeyLoading @ key-loader.ts:122 emit @ index.js:182 emit @ hls.ts:253 trigger @ hls.ts:261 loadKey @ base-stream-controller.ts:321 doTick @ subtitle-stream-controller.ts:401 tick @ task-loop.ts:110 setInterval (async) setInterval @ task-loop.ts:72 startLoad @ subtitle-stream-controller.ts:85 (anonymous) @ hls.ts:366 startLoad @ hls.ts:365 onManifestLoaded @ level-controller.ts:196 emit @ index.js:203 emit @ hls.ts:253 trigger @ hls.ts:261 handleMasterPlaylist @ playlist-loader.ts:451 loadsuccess @ playlist-loader.ts:348 readystatechange @ xhr-loader.ts:197 XMLHttpRequest.send (async) (anonymous) @ VM237:1 loadInternal @ xhr-loader.ts:134 load @ xhr-loader.ts:72 load @ playlist-loader.ts:310 onManifestLoading @ playlist-loader.ts:155 emit @ index.js:203 emit @ hls.ts:253 trigger @ hls.ts:261 loadSource @ hls.ts:353 loadSelectedStream @ main.js:374 (anonymous) @ main.js:227 j @ jquery.min.js:2 fireWith @ jquery.min.js:2 ready @ jquery.min.js:2 I @ jquery.min.js:2 decrypter.ts:191 [log] > [decrypter.ts]: JS AES decrypt timeline-controller.ts:476 Uncaught (in promise) TypeError: Cannot read properties of null (reading 'byteLength') at TimelineController.onFragLoaded (timeline-controller.ts:476:19) at TimelineController.onFragDecrypted (timeline-controller.ts:639:12) at EventEmitter.emit (index.js:203:1) at Hls.emit (hls.ts:253:26) at Hls.trigger (hls.ts:261:19) at subtitle-stream-controller.ts:321:15

No reason other than the state is related to SourceBuffer states and this controller does not have one so some flow is missing.

So implementing ENDED state for this controller would have been another approach to stopping loop loading of the last fragment?

Now we have the situation that after loading all the fragments. In the next tick it searches for a new fragment anyway. This would be more of a resource optimization.

If you want to simulate this error add throw new Error('test decrypter'); decrypter.ts#L163.
I see that softwareDecrypt as a result returns null. After that, there is no payload verification. Are you able to verify the functionality of softwareDecrypt?

I still cannot reproduce. I can see how we might end up there. How about this?

- private onWebCryptoError(err, data, key, iv): ArrayBuffer | null { + private onWebCryptoError(err, data, key, iv): ArrayBuffer | never { logger.warn('[decrypter.ts]: WebCrypto Error, disable WebCrypto API:', err); this.config.enableSoftwareAES = true; this.logEnabled = true; - return this.softwareDecrypt(data, key, iv); + const result = this.softwareDecrypt(data, key, iv); + if (result === null) { + throw err; + } + return result; }

and then

- this.webCryptoDecrypt(new Uint8Array(data), key, iv).then(callback); + this.webCryptoDecrypt(new Uint8Array(data), key, iv) + .then(callback) + .catch(logger.error);

Thanks for your suggestions.
For now, I would hold off on merging until I test on a longer stream.

I noticed that the error is easily reproduced as we set in the configuration "enableSoftwareAES": true .

I was able to repro. Thanks for all the changes. It's getting close. I think we can remove this payload?.byteLength safety check now.

phloxic · 2022-09-13T10:15:12Z

Thanks for the fixes. I missed this one before cutting v1.2.3. However, I closed #4898 as I was not able to reproduce the payload exceptions using the stream or steps in the AES-128 VTT loop loading issue. Is this intermittent or are there additional steps needed to repro?

@robwalch - I can still repro the payload exceptions. But by now I believe they were/are caused by hard reloads of the demo page. In 'normal' use I cannot repro (famous last words).

mtoczko · 2022-09-14T14:38:35Z

Hi @phloxic
Do you have a longer test stream? (I mean more vtt segemnts)

mtoczko · 2022-09-14T16:00:07Z

@robwalch
There may also be some error with the key, because now it tries to download the fragment again if the payload was 'null'. And the error goes away after reloading the key.

phloxic · 2022-09-14T21:20:24Z

Hi @phloxic Do you have a longer test stream? (I mean more vtt segemnts)

@mtoczko here you go: https://d12zt1n3pd4xhr.cloudfront.net/dev/aesx-s-cc-en.m3u8

src/crypt/decrypter.ts

robwalch · 2022-09-14T21:31:19Z

src/controller/subtitle-stream-controller.ts

+ // remove the fragment without payload and try downloading again
+ this.fragmentTracker.removeFragment(frag);


I'm a little worried this could lead to loop loading. We probably want to trigger a frag load error (or define a new frag decryption error event) that would go through the properly retry logic rather than leaving it up to the idle tick cycle.

There may also be some error with the key, because now it tries to download the fragment again if the payload was 'null'. And the error goes away after reloading the key.

Makes sense. What concerns me is cases where the key is always bad. A frag load error (when the key fails like this) will keep the retries within the bounds of FragmentLoaderConfig max retry and retry delay params.

I am a little concerned about one thing, why entering the same data. We get different results. As if something modified the loaded key, because after reloading the same key the result is correct.

I'm a little worried this could lead to loop loading. We probably want to trigger a frag load error (or define a new frag >decryption error event) that would go through the properly retry logic rather than leaving it up to the idle tick cycle.

There is such a risk, only we do not have a method to skip such a fragment, without this we will have
a fragment with unencrypted data and a gap in the subtitles.

When the fragment retry logic exhausts all attempts, a fatal error is triggered that ends playback - So the player considers this stream broken rather than leaving a gap in subs. That is better than letting it retry indefinitely without repercussions or delay between retries.

I am a little concerned about one thing, why entering the same data. We get different results.

Is the initialization vector the same each time?

I will take some time and look for the error in decrypter.ts.
I'll see how it works with #4861. For now, I would remove PR from milestone 1.2.4

May be related to #4666

Co-authored-by: Rob Walch <robwalch@users.noreply.github.com>

mtoczko · 2022-09-17T15:10:04Z

@phloxic What AES mode are you using?

phloxic · 2022-09-17T22:45:32Z

@phloxic What AES mode are you using?

Oh, in that version AES-128 for audio and subtitles, and Sample AES for the video. The original symptoms were the same when using AES-128 for everything. Do you need another one like that?

phloxic · 2022-09-17T23:57:37Z

@phloxic What AES mode are you using?

@mtoczko - here's a pure AES-128 version. HTH.

mtoczko · 2022-10-04T15:18:02Z

Hi @robwalch
Your PR #4861 solves the problem with webCryptoDecrypt, there must have been some conflict/race.
I removed the attempt to reload the fragment, only a warning will be returned.

robwalch · 2022-10-05T01:18:09Z

src/controller/subtitle-stream-controller.ts

@@ -326,6 +326,10 @@ export class SubtitleStreamController
 tdecrypt: endTime,
 },
 });
+ })
+ .catch((e) => {
+ this.warn(e.message);


Recently, in #4933 the exception I encountered had no message, only an e.name of "OperationError". Should we account for that here?

Suggested change

this.warn(e.message);

this.warn(e.name || e.message);

You are talking about a WebCrypto error which is handled by

.catch((err) => { logger.warn( '[decrypter.ts]: WebCrypto Error, disable WebCrypto API:', err );

src/crypt/decrypter.ts

robwalch

Thanks @mtoczko,

Can you resolve the merge conflicts or would you like me to?

mtoczko · 2022-10-05T17:58:09Z

@robwalch If you could resolve this conflict, that would be great.

fix: `Uncaught (in promise) TypeError: Cannot read properties of null…

649b91f

… (reading 'byteLength')``

robwalch approved these changes Sep 13, 2022

View reviewed changes

robwalch added the cannot reproduce label Sep 13, 2022

robwalch requested changes Sep 13, 2022

View reviewed changes

robwalch added the Bug label Sep 13, 2022

Marcin Toczko added 2 commits September 14, 2022 17:00

version 2

b91696e

restart after error

0e4785a

robwalch requested changes Sep 14, 2022

View reviewed changes

mtoczko and others added 2 commits September 14, 2022 23:59

Update src/crypt/decrypter.ts

aeb245e

Co-authored-by: Rob Walch <robwalch@users.noreply.github.com>

version 3

a51eef1

robwalch added this to the 1.2.4 milestone Sep 14, 2022

robwalch added Confirmed and removed cannot reproduce labels Sep 14, 2022

robwalch removed this from the 1.2.4 milestone Sep 15, 2022

robwalch mentioned this pull request Sep 28, 2022

Optimal Buffer Settings on a hybrid android app. #4931

Closed

Marcin Toczko and others added 2 commits October 4, 2022 17:12

no attempt to reload

a6fe18e

Merge branch 'master' into bugfix/4898-part-2

be4498e

robwalch requested changes Oct 5, 2022

View reviewed changes

Marcin Toczko added 2 commits October 5, 2022 13:38

version 4

862cdd4

version 4.1

ca13e5e

robwalch previously approved these changes Oct 5, 2022

View reviewed changes

Merge branch 'master' into bugfix/4898-part-2

c24c67d

robwalch dismissed their stale review via c24c67d October 5, 2022 18:01

robwalch approved these changes Oct 5, 2022

View reviewed changes

robwalch merged commit ca19059 into video-dev:master Oct 5, 2022

robwalch added this to the 1.3.0 milestone Oct 5, 2022

mtoczko deleted the bugfix/4898-part-2 branch October 5, 2022 21:32

robwalch mentioned this pull request Oct 6, 2022

Decrypter cleanup #4948

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: TypeError: Cannot read properties of null… payload.byteLength (timeline-controller) #4902

Bugfix: TypeError: Cannot read properties of null… payload.byteLength (timeline-controller) #4902

mtoczko commented Sep 13, 2022 •

edited

Loading

robwalch commented Sep 13, 2022 •

edited

Loading

robwalch Sep 13, 2022 •

edited

Loading

mtoczko Sep 13, 2022

robwalch Sep 13, 2022

mtoczko Sep 13, 2022

mtoczko Sep 13, 2022

robwalch Sep 14, 2022 •

edited

Loading

mtoczko Sep 14, 2022

robwalch Sep 14, 2022

phloxic commented Sep 13, 2022

mtoczko commented Sep 14, 2022

mtoczko commented Sep 14, 2022 •

edited

Loading

phloxic commented Sep 14, 2022

robwalch Sep 14, 2022

robwalch Sep 14, 2022 •

edited

Loading

mtoczko Sep 14, 2022

robwalch Sep 14, 2022 •

edited

Loading

robwalch Sep 14, 2022

mtoczko Sep 15, 2022

robwalch Sep 16, 2022

mtoczko commented Sep 17, 2022

phloxic commented Sep 17, 2022

phloxic commented Sep 17, 2022

mtoczko commented Oct 4, 2022

robwalch Oct 5, 2022

mtoczko Oct 5, 2022

robwalch left a comment

mtoczko commented Oct 5, 2022

		// remove the fragment without payload and try downloading again
		this.fragmentTracker.removeFragment(frag);

Bugfix: TypeError: Cannot read properties of null… payload.byteLength (timeline-controller) #4902

Bugfix: TypeError: Cannot read properties of null… payload.byteLength (timeline-controller) #4902

Conversation

mtoczko commented Sep 13, 2022 • edited Loading

This PR will...

Why is this Pull Request needed?

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

robwalch commented Sep 13, 2022 • edited Loading

robwalch Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robwalch Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phloxic commented Sep 13, 2022

mtoczko commented Sep 14, 2022

mtoczko commented Sep 14, 2022 • edited Loading

phloxic commented Sep 14, 2022

Choose a reason for hiding this comment

robwalch Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robwalch Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtoczko commented Sep 17, 2022

phloxic commented Sep 17, 2022

phloxic commented Sep 17, 2022

mtoczko commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robwalch left a comment

Choose a reason for hiding this comment

mtoczko commented Oct 5, 2022

mtoczko commented Sep 13, 2022 •

edited

Loading

robwalch commented Sep 13, 2022 •

edited

Loading

robwalch Sep 13, 2022 •

edited

Loading

robwalch Sep 14, 2022 •

edited

Loading

mtoczko commented Sep 14, 2022 •

edited

Loading

robwalch Sep 14, 2022 •

edited

Loading

robwalch Sep 14, 2022 •

edited

Loading