REGRESSION (r20182-r20184): Incorrect rendering of multipart images

• Component: Images
• Status: RESOLVED FIXED

[This is possibly a duplicate of bug 13642]

To reproduce this bug, apply the attached patch to LayoutTests and execute 'run-webkit-httpd'. Then in WebKit go to <http://127.0.0.1:8000/multipart/image-decoding.html>.

Expected result: The image should cycle between a blue square, a green square and Abe.

Actual result: The image cycles between a white square, a green square, and a grey line.

Notes: Firefox and shipping Safari render the images as expected. So far this doesn't seem to be a loader bug, as I have observed that ImageSource::setData() is called with the correct data.

Created attachment 14589 [details]
HTTP test in patch form

Regressed in <http://trac.webkit.org/projects/webkit/changeset/20182> or in one of the follow-up patches <http://trac.webkit.org/projects/webkit/changeset/20183> and <http://trac.webkit.org/projects/webkit/changeset/20184>.

The problem is that ImageSource::setData() does not copy the data, and in the case of multipart images, the buffer becomes invalid by the time createFrameAtIndex() is called.

Created attachment 14593 [details]
Band-aid fix

The way the loader currently works, only when a new part arrives, the previous part is delivered to the client. This means that at time t, Safari always shows you part t-1. This is in fact the behavior of shipping Safari as well. You can see it by changing the timeouts in the test case to make them uneven: change the sleep(1) after abe to sleep(4) and run the test. Notice how it is the green square, rather than abe, that is displayed for 4 seconds in each cycle. In contrast, in Firefox it is indeed abe that is displayed for 4 seconds.

This patch doesn't fix the above behavior, but accounting for it, it ensures that the client gets its own copy of the data so that it can use it after the resource data changes to contain (parts of) the next part.

I think it's worth applying this fix for now, until the aforementioned bug is addressed, so I will try to make a test to go with this patch.

Created attachment 14595 [details]
Copy the current part's data before it is overwritten by the next part

Includes a pixel test. Also moves the last multipart test together with the new one into a new 'multipart' subdirectory where they share the multipart.php script.

[Brady Eidson]

Comment on attachment 14595 [details]
Copy the current part's data before it is overwritten by the next part

I agree the larger bug here is we need to be displaying frame T, not T-1.

But I think this is a solid fix to land now.  r=me

[Brady Eidson]

<rdar://problem/5210662>

[Brady Eidson]

Landed in r21536

[Brady Eidson]

*** Bug 13642 has been marked as a duplicate of this bug. ***

[Peter]

Webkit does not crash anymore, webcam renders smoothly. Thanks for the effort! The address bar keeps showing the blue "loading" bar though. Is there a separate bug yet for this?

(In reply to comment #10)
> The address bar keeps showing the blue "loading" bar though. Is there a
> separate bug yet for this?

Bug 13763.