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> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, beidson, commit-queue, eric, fishd, johnnyg, mjs | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Tony Chang
2009-11-12 16:34:24 PST
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. |