1.82 KB, patch
|Details | Diff | Splinter Review|
dom/workers/test/serviceworkers/test_fetch_event.html has a bug, which is actually found in fetch_event_worker.js. The handler for "nonexistent_image.gif" uses atob() to convert a base64-encoded string into a string containing binary data which can be passed to the Response constructor. Unfortunately, this doesn't work correctly, because a conversion from UTF-16 to UTF-8 occurs which ends up inserting garbage bytes into the image data. We got away with this previous because the garbage bytes occur just after the image header's width and height fields, which used to be all we cared about when deciding whether to fire the load event. However, starting in bug 1191114 we need the entire header, which exposes the fact that this image is fatally corrupted by the garbage bytes. So bug 1191114 makes us start firing onerror instead of onload for this image, and we fail the test. We can fix the test by avoiding the encoding conversion.
Created attachment 8646486 [details] [diff] [review] Avoid corrupting image data in test_fetch_event.html Here's the patch. We create a blob and pass that to the Response() constructor instead of passing a string, avoiding any encoding-related issues.
Attachment #8646486 - Flags: review?(bkelly)
(There's a try job in bug 1191114 which contains this patch and is 100% green.)
Comment on attachment 8646486 [details] [diff] [review] Avoid corrupting image data in test_fetch_event.html Review of attachment 8646486 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/workers/test/serviceworkers/fetch_event_worker.js @@ +92,5 @@ > + // If we just pass |imageAsBinaryString| to the Response constructor, an > + // encoding conversion occurs that corrupts the image. Instead, we need to > + // convert it to a blob, which requires going through the intermediary of a > + // typed array. > + var imageAsArray = new Uint8Array(imageLength); The Response constructor actually takes BufferSource as an argument. That means you can pass Uint8Array directly to new Response(). (That *should* work, but who knows if we have another bug there...)
Attachment #8646486 - Flags: review?(bkelly) → review+
Created attachment 8646773 [details] [diff] [review] Avoid corrupting image data in test_fetch_event.html Thanks for the review! I've confirmed that the blob step is indeed unnecessary, and updated the patch accordingly.
Attachment #8646486 - Attachment is obsolete: true
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.