Bug 38543 - [Chromium] Crash in WebPageSerializerImpl::serialize() on downloaded iframe
Summary: [Chromium] Crash in WebPageSerializerImpl::serialize() on downloaded iframe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-04 13:46 PDT by Nate Chapin
Modified: 2010-05-12 12:15 PDT (History)
4 users (show)

See Also:


Attachments
patch (1.42 KB, patch)
2010-05-04 13:54 PDT, Nate Chapin
eric: review-
Details | Formatted Diff | Diff
patch + updated changelog (1.61 KB, patch)
2010-05-07 09:41 PDT, Nate Chapin
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-05-04 13:46:08 PDT
Original report at http://crbug.com/42212

If an iframe is set to a url that gets downloaded, the frame loader never commits to the load and never sets m_URL (since it never actually performed a navigation).  If we try to save the page, we get to WebPageSerializerImpl::serialize() and try to do operations on each of the urls in turn, reach the invalid url of the downloaded iframe, and crash.

There are two ways to fix this:
1. Check the url in WebPageSerializerImpl before using it
2. Ensure FrameLoader::m_url is always valid.

I'm inclined to go with (1), since this crash appears to be occurring in Chromium only.
Comment 1 Nate Chapin 2010-05-04 13:54:54 PDT
Created attachment 55044 [details]
patch
Comment 2 Adam Barth 2010-05-04 14:04:06 PDT
Comment on attachment 55044 [details]
patch

Please add a test.  I think we have the ability to unit test the Chromium WebKit API.
Comment 3 Nate Chapin 2010-05-04 15:44:31 PDT
A test that covers this functionality would need to bring up a full WebFrame and be able to exercise it.  I don't believe that is possible with the testing tools currently available for the Chromium API in webkit.org (I think it requires the tools chromium has in test_shell_tests).  If you would like me to write a unit test, I'm happy to do so, but I think it would need to be in src.chromium.org for now.
Comment 4 Nate Chapin 2010-05-05 08:37:18 PDT
Comment on attachment 55044 [details]
patch

Resetting r? for the reason outlined in my previous post (a test that covers this functionality would need to live in the chromium repo at the moment).
Comment 5 Nate Chapin 2010-05-05 09:23:06 PDT
(In reply to comment #4)
> (From update of attachment 55044 [details])
> Resetting r? for the reason outlined in my previous post (a test that covers
> this functionality would need to live in the chromium repo at the moment).

FYI, the test for this patch is up for review at http://codereview.chromium.org/1917007
Comment 6 Eric Seidel (no email) 2010-05-05 21:56:23 PDT
Comment on attachment 55044 [details]
patch

Needs tests or explanation of why testing is impossible.
Comment 7 Dimitri Glazkov (Google) 2010-05-06 08:17:29 PDT
(In reply to comment #6)
> (From update of attachment 55044 [details])
> Needs tests or explanation of why testing is impossible.

In the changelog, right? Because I think Nate provided a pretty good explanation in the bug.
Comment 8 Eric Seidel (no email) 2010-05-07 09:35:18 PDT
Yes, in the ChangeLog.  I don't (and I guess Adam doesn't either) read bugs during most reviews (except to validate that no one else has objected).
Comment 9 Nate Chapin 2010-05-07 09:41:03 PDT
Created attachment 55388 [details]
patch + updated changelog
Comment 10 Adam Barth 2010-05-11 13:05:55 PDT
Comment on attachment 55388 [details]
patch + updated changelog

ok
Comment 12 Nate Chapin 2010-05-12 12:15:26 PDT
http://trac.webkit.org/changeset/59169