|Summary:||[chromium] crash in chromium when loading multipart/x-mixed-replace data|
|Product:||WebKit||Reporter:||Tony Chang <tony>|
|Component:||Page Loading||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||abarth, beidson, commit-queue, eric, fishd, johnnyg, mjs|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Tony Chang 2009-11-12 16:34:24 PST
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.
Comment 2 Eric Seidel (no email) 2009-11-13 13:18:55 PST
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.
Comment 3 Eric Seidel (no email) 2009-11-13 13:19:12 PST
CCing loader folks.
Comment 5 Tony Chang 2009-11-13 16:13:17 PST
(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.
Comment 6 Adam Barth 2009-11-13 17:52:27 PST
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 7 Darin Fisher (:fishd, Google) 2009-11-16 10:11:34 PST
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?
Comment 8 Tony Chang 2009-11-16 12:09:35 PST
(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.
Comment 9 Tony Chang 2009-11-16 12:10:17 PST
(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.
Comment 11 Darin Fisher (:fishd, Google) 2009-12-02 10:11:22 PST
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?
Comment 12 Darin Fisher (:fishd, Google) 2009-12-02 10:13:41 PST
Actually, doesn't the FrameLoaderClient get notified during the call to didReceiveResponse? In that case, would the solution I just proposed really work?
Comment 13 Tony Chang 2009-12-02 11:06:51 PST
(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 14 Maciej Stachowiak 2009-12-28 17:45:36 PST
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).
Comment 16 Maciej Stachowiak 2010-01-11 22:41:02 PST
Comment on attachment 46335 [details] with test r+
Comment 18 Tony Chang 2010-01-12 01:09:16 PST
(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 19 WebKit Commit Bot 2010-01-12 10:10:28 PST
Comment on attachment 46348 [details] Patch Clearing flags on attachment: 46348 Committed r53143: <http://trac.webkit.org/changeset/53143>
Comment 20 WebKit Commit Bot 2010-01-12 10:10:41 PST
All reviewed patches have been landed. Closing bug.
Comment 21 John Gregg 2010-01-12 14:37:33 PST
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*))
Comment 22 Tony Chang 2010-01-12 17:07:00 PST
(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.