Bug 38543 – [Chromium] Crash in WebPageSerializerImpl::serialize() on downloaded iframe

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Nate Chapin]

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.

[Nate Chapin]

Created attachment 55044 [details]
patch

[Adam Barth]

Comment on attachment 55044 [details]
patch

Please add a test.  I think we have the ability to unit test the Chromium WebKit API.

[Nate Chapin]

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.

[Nate Chapin]

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).

[Nate Chapin]

(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

[Eric Seidel (no email)]

Comment on attachment 55044 [details]
patch

Needs tests or explanation of why testing is impossible.

[Dimitri Glazkov (Google)]

(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.

[Eric Seidel (no email)]

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).

[Nate Chapin]

Created attachment 55388 [details]
patch + updated changelog

[Adam Barth]

Comment on attachment 55388 [details]
patch + updated changelog

ok

[WebKit Review Bot]

http://trac.webkit.org/changeset/59169 might have broken Chromium Win Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/59168
http://trac.webkit.org/changeset/59169
http://trac.webkit.org/changeset/59170
http://trac.webkit.org/changeset/59165
http://trac.webkit.org/changeset/59166
http://trac.webkit.org/changeset/59167

[Nate Chapin]

http://trac.webkit.org/changeset/59169