Bug 23806 – Night builds crash when visiting http://www.spawn.com/toys/media.aspx

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[David Levin]

This is related to the loader and started occurring due to some change in the range 40539:40668.

[Alexey Proskuryakov]

Is this a duplicate of bug 23736? Are stack traces the same?

[David Levin]

Created attachment 27459 [details]
crash stack traces

It looks like different stack traces to me.

[David Levin]

fwiw, [e-mail] emailed me on Friday that he thinks he knows where the fix needs to be.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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

[Dave Hyatt]

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?

[Dimitri Glazkov (Google)]

Created attachment 27490 [details]
Another stack trace.

Here is another stack trace, from the reduction.

[Dave Hyatt]

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.

[Dave Hyatt]

(Or even just making requestObject bail if the URL is empty.)

[Dave Hyatt]

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?

[Dave Hyatt]

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

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/40792.