Bug 23806 - Night builds crash when visiting http://www.spawn.com/toys/media.aspx
Summary: Night builds crash when visiting http://www.spawn.com/toys/media.aspx
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL: http://www.spawn.com/toys/media.aspx
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2009-02-06 16:03 PST by David Levin
Modified: 2009-02-09 13:11 PST (History)
2 users (show)

See Also:


Attachments
crash stack traces (32.37 KB, text/plain)
2009-02-07 22:38 PST, David Levin
no flags Details
Crash reduction. (92 bytes, text/html)
2009-02-08 10:20 PST, Dimitri Glazkov (Google)
no flags Details
fix embed with empty src attribute crash, v1 (3.28 KB, patch)
2009-02-09 11:39 PST, Dimitri Glazkov (Google)
hyatt: review+
Details | Formatted Diff | Diff
Another stack trace. (37.17 KB, text/plain)
2009-02-09 11:58 PST, Dimitri Glazkov (Google)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-02-06 16:03:26 PST
This is related to the loader and started occurring due to some change in the range 40539:40668.
Comment 1 Alexey Proskuryakov 2009-02-07 04:13:40 PST
Is this a duplicate of bug 23736? Are stack traces the same?
Comment 2 David Levin 2009-02-07 22:38:41 PST
Created attachment 27459 [details]
crash stack traces

It looks like different stack traces to me.
Comment 3 David Levin 2009-02-07 22:45:01 PST
fwiw, rahulk@chromium.org emailed me on Friday that he thinks he knows where the fix needs to be.

Comment 4 Dimitri Glazkov (Google) 2009-02-08 08:39:52 PST
The issue occurs because there's a malformed embed tag on the page: it has an empty src attribute and a non-empty type attrubite. I'll look into this on Monday.
Comment 5 Dimitri Glazkov (Google) 2009-02-08 10:20:29 PST
Created attachment 27467 [details]
Crash reduction.

Here's the reduction. I have a simple fix, but really would dig deeper to figure out where the regression occurred -- and possibly have a more accurate fix.
Comment 6 Dimitri Glazkov (Google) 2009-02-09 11:39:20 PST
Created attachment 27489 [details]
fix embed with empty src attribute crash, v1

 LayoutTests/ChangeLog                              |   11 +++++++++++
 .../loader/empty-embed-src-attribute-expected.txt  |    7 +++++++
 .../fast/loader/empty-embed-src-attribute.html     |   14 ++++++++++++++
 WebCore/ChangeLog                                  |   14 ++++++++++++++
 WebCore/loader/FrameLoader.cpp                     |    2 +-
 5 files changed, 47 insertions(+), 1 deletions(-)
Comment 7 Dave Hyatt 2009-02-09 11:56:07 PST
Comment on attachment 27489 [details]
fix embed with empty src attribute crash, v1

I don't think this is right, since an empty URL is not in the HTTP/S family and so should not add in HTTP fields. Why is the early return causing a crash?  What does the stack look like?
Comment 8 Dimitri Glazkov (Google) 2009-02-09 11:58:22 PST
Created attachment 27490 [details]
Another stack trace.

Here is another stack trace, from the reduction.
Comment 9 Dave Hyatt 2009-02-09 12:32:06 PST
It seems to me that requestObject just shouldn't even be called on an empty URL, but I'm not a networking expert.  Somehow having the mainDocumentURL set on the ResourceRequest caused the eventual ResourceRequest to have a non-null URL, but that doesn't seem right to me.

I think patching RenderPartObject and then adding an assert !url.isEmpty() to requestObject may be the way to go here.


Comment 10 Dave Hyatt 2009-02-09 12:32:25 PST
(Or even just making requestObject bail if the URL is empty.)

Comment 11 Dave Hyatt 2009-02-09 12:36:29 PST
It looks like loads are deliberately allowed for empty URLs if the MIME type is set.  Maybe we're wanting to load about:blank for HTML MIME types?

Comment 12 Dave Hyatt 2009-02-09 12:54:46 PST
Comment on attachment 27489 [details]
fix embed with empty src attribute crash, v1

Just  going to + this for now, since I can't see any better way to fix it.  I think URL just gets coincidentally set to non-null from updateResourceRequest.  That's private in platform though so FrameLoader can't be made a friend.  We basically need some random operation to be done on the resource request like setting the main document URL to force an update to happen.  This all seems really fragile and wrong, but oh well.

r=me
Comment 13 Dimitri Glazkov (Google) 2009-02-09 13:11:08 PST
Landed as http://trac.webkit.org/changeset/40792.