Fix image corruption in test_fetch_event.html

RESOLVED FIXED in Firefox 43



DOM: Service Workers
3 years ago
3 years ago


(Reporter: seth, Assigned: seth)



Firefox Tracking Flags

(firefox43 fixed)



(1 attachment, 1 obsolete attachment)



3 years ago
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.

Comment 1

3 years ago
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)

Comment 2

3 years ago
(There's a try job in bug 1191114 which contains this patch and is 100% green.)

Comment 3

3 years ago
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+

Comment 4

3 years ago
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.


3 years ago
Attachment #8646486 - Attachment is obsolete: true
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.