handle bad/invalid chunks as input more defensively#5671
Conversation
|
Can I ask that you expand the PR description a bit more detailing the issue and summarizing the fix. |
Definitely. Just updated the description. |
jasnell
left a comment
There was a problem hiding this comment.
To be clear, the "invalid chunks" here means passing invalid types to input the stream.
| comment: 'Failed V8 assert', | ||
| // external/v8/src/api/api-inl.h:163; message = v8::internal::ValueHelper::IsEmpty(that) || IsJSArrayBufferView(v8::internal::Tagged<v8::internal::Object>( v8::internal::ValueHelper::ValueAsAddress(that))) | ||
| disabledTests: true, | ||
| }, |
There was a problem hiding this comment.
Hmm... I guess there's a question here to ask about whether we want to rely entirely on the WPT's for these cases or extend our workerd tests to account for these. That's a broader question, of course that doesn't necessarily need to be addressed in these individual PRs. It's worth noting, however, that since we do not have control over the WPT's, changes could in theory be introduced in the tests that change the expectations. We might not catch those. It's exceedingly unlikely so probably not worth the effort, but it's something to keep in mind.
There was a problem hiding this comment.
Resolving this to land... Will unresolve once it lands.
There was a problem hiding this comment.
Even though WPT is a moving target, it's highly unlikely for someone to remove a test from WPT - since it would be breaking change and something that's hard to justify.
Fixes #5394
Failing web-platform test can be found at https://github.com/web-platform-tests/wpt/blob/master/encoding/streams/decode-bad-chunks.any.js. It tests for passing invalid chunk into the stream, and prior to this function we didn't throw the correct error, even though we failed. This change ensures that we are WPT compliant.