Bug 31446 – [chromium] crash in chromium when loading multipart/x-mixed-replace data

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

[Tony Chang]

Here's what's happening:

1) We get some multipart/x-mixed-replace data from the server, parse the header out of it, then call WebCore::ResourceLoader::didReceiveResponse.
2) This calls through to FrameLoader::setupForReplace(), which nulls out m_documentLoader.
3) User stops the load and tries to navigate to a different page.
4) When navigating to a different page, we call through to ResourceLoadNotifier::dispatchWillSendRequest which then 
tries to access the document loader, which is null, so we crash.

This doesn't crash in Safari because CFNetwork seems to ensures that during multipart data, 
calls to WebCore::ResourceLoader::didReceiveResponse are immediately followed by 
calls to WebCore::ResourceLoader::didReceiveData.  Since it's not possible to cancel a load between these two calls, we don't end up with the null document loader.

In Chromium, we could match CFNetwork and postpone the call to didReceiveResponse until we have some data to send with it.  This works around the crash, but WebCore seems to buffer on didReceiveData up to 1024 bytes (maybe in the document loader?  I'm not sure).  This means that you never see content if each multipart chunk is less than 1024 bytes.  This doesn't impact Safari because CFNetwork just doesn't handle multipart chunks less than 1024 bytes (it merges chunks until you get at least 1024 bytes), so maybe that's expected behavior for WebCore.

I'm not sure what the right fix for this is, but I'll upload a patch for discussion.

[Tony Chang]

Created attachment 43114 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 43114 [details]
Patch

Needs a ChangeLog which explains what you're doing here and why.  The code might also need a comment here.

[Eric Seidel (no email)]

CCing loader folks.

[Tony Chang]

Created attachment 43211 [details]
Patch

[Tony Chang]

(In reply to comment #4)
> Created an attachment (id=43211) [details]
> Patch

I'll write an http layout test once I get some feedback as to which direction to go.

[Adam Barth]

This seems reasonable to me, but I don't understand multi-part loading in detail.  In general, WebCore shouldn't depend on details of CFNetwork.

[Darin Fisher (:fishd, Google)]

Comment on attachment 43211 [details]
Patch

> +++ b/WebCore/loader/FrameLoader.cpp
...
> +                if (isReplacing())
> +                    setDocumentLoader(m_provisionalDocumentLoader.get());

In WebFrameImpl::loadData, we also call setReplacing.  Could that interact
in an interesting way with this code?

[Tony Chang]

(In reply to comment #7)
> (From update of attachment 43211 [details])
> > +++ b/WebCore/loader/FrameLoader.cpp
> ...
> > +                if (isReplacing())
> > +                    setDocumentLoader(m_provisionalDocumentLoader.get());
> 
> In WebFrameImpl::loadData, we also call setReplacing.  Could that interact
> in an interesting way with this code?

As far as I can tell, WebFrameImpl::loadData is only used by WebFrameImpl::loadData (error pages), so it doesn't seem to interact with this code.

In general, lots of stuff happens when you stop/cancel a load, so it's not clear to me where the right place to put this code is.

[Tony Chang]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 43211 [details] [details])
> > > +++ b/WebCore/loader/FrameLoader.cpp
> > ...
> > > +                if (isReplacing())
> > > +                    setDocumentLoader(m_provisionalDocumentLoader.get());
> > 
> > In WebFrameImpl::loadData, we also call setReplacing.  Could that interact
> > in an interesting way with this code?
> 
> As far as I can tell, WebFrameImpl::loadData is only used by
> WebFrameImpl::loadData (error pages), so it doesn't seem to interact with this
> code.

Err, WebFrameImpl::loadData is only used by WebFrameImpl::loadHTMLString.

[Adam Barth]

Attachment 43211 [details] passed the style-queue

[Darin Fisher (:fishd, Google)]

Tony, I think trying to match the behavior of CFNetwork is probably a safer route to go.  Less differences between Safari and Chrome is generally less problematic.  Instead of delaying until 1024 bytes are received, perhaps we could just delay the didReceiveResponse until we have some data to pass in a didReceiveData call?

[Darin Fisher (:fishd, Google)]

Actually, doesn't the FrameLoaderClient get notified during the call to didReceiveResponse?  In that case, would the solution I just proposed really work?

[Tony Chang]

(In reply to comment #12)
> Actually, doesn't the FrameLoaderClient get notified during the call to
> didReceiveResponse?  In that case, would the solution I just proposed really
> work?

It works in that it doesn't crash, but it doesn't render anything if the content is less than 1024 bytes.  Using the suggested change (wait for data before sending the header), here's a video when the server sends 512 bytes of data:
http://ponderer.org/tests/data512.ogg
http://ponderer.org/tests/multipart-512.py

Here's a video when the server sends > 1024 bytes of data:
http://ponderer.org/tests/data1024.ogg
http://ponderer.org/tests/multipart-1024.py

The ToT Chrome handles the above two cases correctly, but the renderer crashes when navigating away (not really noticeable unless you try to navigate to the same domain).

I think sending the headers right away tells the doc loader than it can render the data it has so far.  This allows data less than 1024 bytes to render immediately.  One option would be to have Chrome match CFNetwork and try to make the document render partial data.

Firefox handles both cases properly and Safari doesn't handle the 512 case because of CFNetwork merging two parts into a single document.

[Maciej Stachowiak]

Comment on attachment 43211 [details]
Patch

Looks good to me. Needs a layout test (or if this is covered by some existing test, please say so).

[Tony Chang]

Created attachment 46335 [details]
with test

[Maciej Stachowiak]

Comment on attachment 46335 [details]
with test

r+

[Tony Chang]

Created attachment 46348 [details]
Patch

[Tony Chang]

(In reply to comment #17)
> Created an attachment (id=46348) [details]
> Patch

This version should work on OSX.  This works around the CF bug mentioned above where each mulitpart piece has to be at least 512 bytes for CF to work.

[WebKit Commit Bot]

Comment on attachment 46348 [details]
Patch

Clearing flags on attachment: 46348

Committed r53143: <http://trac.webkit.org/changeset/53143>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Gregg]

This patch appears to cause chromium browser test failures:

[ RUN      ] ErrorPageTest.FLAKY_DNSError_GoBack2
ASSERTION FAILED: loader != m_documentLoader
(/b/slave/linux/build/src/third_party/WebKit/WebCore/loader/FrameLoader.cpp:2339 void WebCore::FrameLoader::setDocumentLoader(WebCore::DocumentLoader*))

[Tony Chang]

(In reply to comment #21)
> This patch appears to cause chromium browser test failures:
> 
> [ RUN      ] ErrorPageTest.FLAKY_DNSError_GoBack2
> ASSERTION FAILED: loader != m_documentLoader
> (/b/slave/linux/build/src/third_party/WebKit/WebCore/loader/FrameLoader.cpp:2339
> void WebCore::FrameLoader::setDocumentLoader(WebCore::DocumentLoader*))

Crash analysis and fix in bug 33560.