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.
Created attachment 43114 [details] Patch
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.
CCing loader folks.
Created attachment 43211 [details] Patch
(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.
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.
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?
(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.
(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.
Attachment 43211 [details] passed the style-queue
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?
Actually, doesn't the FrameLoaderClient get notified during the call to didReceiveResponse? In that case, would the solution I just proposed really work?
(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.
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).
Created attachment 46335 [details] with test
Comment on attachment 46335 [details] with test r+
Created attachment 46348 [details] Patch
(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.
Comment on attachment 46348 [details] Patch Clearing flags on attachment: 46348 Committed r53143: <http://trac.webkit.org/changeset/53143>
All reviewed patches have been landed. Closing bug.
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*))
(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.