Bug 13767 - REGRESSION: Multipart image documents are garbled
Summary: REGRESSION: Multipart image documents are garbled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-05-17 15:58 PDT by mitz
Modified: 2007-05-25 10:38 PDT (History)
0 users

See Also:


Attachments
Patch (2.57 KB, patch)
2007-05-17 16:00 PDT, mitz
no flags Details | Formatted Diff | Diff
Patch including http test (34.55 KB, patch)
2007-05-21 08:42 PDT, mitz
no flags Details | Formatted Diff | Diff
Patch including http test (34.12 KB, patch)
2007-05-25 03:02 PDT, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-05-17 15:58:44 PDT
This is the standalone image document of bug 13759. The symptoms and the root cause are the same: the image uses the resource data, which is then overwritten by the next part.
Comment 1 mitz 2007-05-17 16:00:57 PDT
Created attachment 14599 [details]
Patch

I tried to make a test, for which I included the standalone image document in an iframe. The problem is that it asserts in FrameLoader::addData(). I'll need to investigate that before I can complete this patch.
Comment 2 Brady Eidson 2007-05-17 16:02:30 PDT
<rdar://problem/5211789>
Comment 3 Brady Eidson 2007-05-17 16:09:10 PDT
Patch is a great start (I know the layout test is going to be more work)

One comment - generally using a PassRefPtr as a local variable is a bad idea and can lead to unpredictable problems later.  While this specific case would be fine, no harm in using a RefPtr instead
Comment 4 mitz 2007-05-21 08:42:43 PDT
Created attachment 14642 [details]
Patch including http test

This patch includes a version of the test that does not trigger the assertion failure (which I am going to file a separate bug on).
Comment 5 Darin Adler 2007-05-22 19:37:12 PDT
Comment on attachment 14642 [details]
Patch including http test

+        cachedImage->data(data, true);

Should be data.release() instead of just data to avoid reference count churn.

Need to remove extra ChangeLog entry in the LayoutTests directory.

r=me, because neither of those is a real problem
Comment 6 mitz 2007-05-25 02:28:13 PDT
Comment on attachment 14642 [details]
Patch including http test

ImageDocument change, so this patch no longer applies cleanly. I'm going to update it.
Comment 7 mitz 2007-05-25 03:02:32 PDT
Created attachment 14716 [details]
Patch including http test

Updated for TOT and addressed Darin's comments. Expected test results have also changed because standalone image documents are in quirks mode now.
Comment 8 Darin Adler 2007-05-25 08:05:52 PDT
Comment on attachment 14716 [details]
Patch including http test

r=me
Comment 9 Sam Weinig 2007-05-25 10:38:40 PDT
Landed in r21760.