WebKit bug to track the crash in http://crbug.com/29437 . A similar crash in Safari on OS X turned out to be due to out-of-order delivery from the Network Layer: https://bugs.webkit.org/show_bug.cgi?id=23643#c10
Created attachment 51426 [details] Sanity Checks Code to try to track down the crash.
Comment on attachment 51426 [details] Sanity Checks Please don't let this code linger very long. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index 5248861..29091f6 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,16 @@ > +2010-03-21 Jeremy Moskovich <jeremy@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Add some diagnostics to try to track down cause of crash in ArchiveFactory::isArchiveMimeType(). > + > + https://bugs.webkit.org/show_bug.cgi?id=36426 > + > + No new tests as there is no new functionality. > + > + * loader/FrameLoader.cpp: > + (WebCore::FrameLoader::finishedLoadingDocument): Make copy of mimeType string to isolate crash. > + > 2010-03-23 Gustavo Noronha Silva <gustavo.noronha@collabora.co.uk> > > Reviewed by Holger Freyther. > diff --git a/WebCore/loader/FrameLoader.cpp b/WebCore/loader/FrameLoader.cpp > index 0323e97..51277f9 100644 > --- a/WebCore/loader/FrameLoader.cpp > +++ b/WebCore/loader/FrameLoader.cpp > @@ -2812,7 +2812,16 @@ void FrameLoader::finishedLoadingDocument(DocumentLoader* loader) > #endif > > // If loading a webarchive, run through webarchive machinery > +#if PLATFORM(CHROMIUM) Sigh...I guess this is OK since it's only temporary. > + // https://bugs.webkit.org/show_bug.cgi?id=36426 > + // FIXME(jeremy@chromium.org): For debugging purposes, should be removed nit: FIXME() is not WebKit style....just do a FIXME and mention your name in the comment if you wish. > + // before closing the bug. > + // Make real copy of the string so we fail here if the responseMIMEType > + // string is bad. > + const String responseMIMEType = loader->responseMIMEType(); > +#else > const String& responseMIMEType = loader->responseMIMEType(); > +#endif > > // FIXME: Mac's FrameLoaderClient::finishedLoading() method does work that is required even with Archive loads > // so we still need to call it. Other platforms should only call finishLoading for non-archive loads > diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog > index 0c505f8..8f395f0 100644 > --- a/WebKit/chromium/ChangeLog > +++ b/WebKit/chromium/ChangeLog > @@ -1,3 +1,21 @@ > +2010-03-21 Jeremy Moskovich <jeremy@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Add some diagnostics to try to track down cause of crash in ArchiveFactory::isArchiveMimeType(). > + > + https://bugs.webkit.org/show_bug.cgi?id=36426 > + > + * src/ResourceHandle.cpp: Track state across ResourceHandle invocations. > + (WebCore::ResourceHandleInternal::ResourceHandleInternal): > + (WebCore::ResourceHandleInternal::): > + (WebCore::ResourceHandleInternal::start): > + (WebCore::ResourceHandleInternal::cancel): > + (WebCore::ResourceHandleInternal::didReceiveResponse): > + (WebCore::ResourceHandleInternal::didReceiveData): > + (WebCore::ResourceHandleInternal::didFinishLoading): > + (WebCore::ResourceHandleInternal::didFail): > + > 2010-03-22 Kenneth Russell <kbr@google.com> > > Reviewed by Darin Fisher. > diff --git a/WebKit/chromium/src/ResourceHandle.cpp b/WebKit/chromium/src/ResourceHandle.cpp > index 206823c..51a43c6 100644 > --- a/WebKit/chromium/src/ResourceHandle.cpp > +++ b/WebKit/chromium/src/ResourceHandle.cpp > @@ -57,6 +57,7 @@ public: > : m_request(request) > , m_owner(0) > , m_client(client) > + , m_state(CONNECTION_STATE_NEW) > { > } > > @@ -74,14 +75,32 @@ public: > virtual void didFinishLoading(WebURLLoader*); > virtual void didFail(WebURLLoader*, const WebURLError&); > > + enum ConnectionState { > + CONNECTION_STATE_NEW, > + CONNECTION_STATE_STARTED, > + CONNECTION_STATE_RECEIVED_RESPONSE, > + CONNECTION_STATE_RECEIVING_DATA, > + CONNECTION_STATE_FINISHED_LOADING, > + CONNECTION_STATE_CANCELED, > + CONNECTION_STATE_FAILED, > + }; > + > + Only one newline.
> +#if PLATFORM(CHROMIUM) > + // https://bugs.webkit.org/show_bug.cgi?id=36426 > + // FIXME(jeremy@chromium.org): For debugging purposes, should be removed > + // before closing the bug. > + // Make real copy of the string so we fail here if the responseMIMEType > + // string is bad. > + const String responseMIMEType = loader->responseMIMEType(); > +#else > const String& responseMIMEType = loader->responseMIMEType(); > +#endif This probably won't have any effect. The problem with the out-of-orderness was that the MIMEType string is null, which is then looked up where null isn't expected. Making a copy of null is still null.
Created attachment 51483 [details] ASSERT() -> CRASH() * ASSERT() is disabled in release builds so call CRASH() explicitly. This matches other places in WebKit where CRASH() is used for fatal errors. * Fix Nits
Brady: Thanks, I hope to catch any violation in the network layer with the changes to ResourceHandle. The String copy lands us in the copy constructor for RefPtr which should crash if the mimetype is null.
Comment on attachment 51483 [details] ASSERT() -> CRASH() We really need some sort of "always assert...even outside of debug mode" assert. I thought one existed, but I can't find it. r=me but please do a follow up patch to add such an assert and convert this code to it. cc maciej, darin, and sam since it's quite likely they'll have an opinion on it.
Comment on attachment 51483 [details] ASSERT() -> CRASH() Clearing flags on attachment: 51483 Committed r56453: <http://trac.webkit.org/changeset/56453>
All reviewed patches have been landed. Closing bug.
Reopening to track removal of debug code.
Comment on attachment 51426 [details] Sanity Checks Cleared Jeremy Orlow's review+ from obsolete attachment 51426 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Reverted r56453 for reason: In Chromium port, it broke invalid-image-data-standalone.html invalid-image-data.html multipart-wait-before-boundary.html stop-crash.html win-boundary-crash.html video-aspect-ratio.html Committed r56469: <http://trac.webkit.org/changeset/56469>
I smile so hard every time someone uses "webkit-patch rollout" successfully. :) Please let me (or better yet, bugs.webkit.org) know if it ever causes you grief...
sheriff-bot is already able to detect regressions (and should have shouted at someone in #webkit, I wasn't on IRC when this broke, so I can't confirm or deny). The intent is to make it automatically rollout regressions caused by the commit-queue (like this one). Soon. Very soon.
Created attachment 52151 [details] Loosen sanity check - allow multiple consecutive calls to connectionDidReceiveResponse Updated patch for debugging purposes.
Created attachment 53247 [details] Allow didRecieveResponse/didReceiveData interleaving in the case of multipart responses The previous patch caused layout test failures due to the sanity checks being too strict, this version loosens the check up a bit.
Comment on attachment 53247 [details] Allow didRecieveResponse/didReceiveData interleaving in the case of multipart responses r- due to enum naming (and the inability to say fix it on landing). > diff --git a/WebCore/loader/FrameLoader.cpp b/WebCore/loader/FrameLoader.cpp > @@ -2870,7 +2870,16 @@ void FrameLoader::finishedLoadingDocument(DocumentLoader* loader) > #endif > > // If loading a webarchive, run through webarchive machinery > +#if PLATFORM(CHROMIUM) > + // https://bugs.webkit.org/show_bug.cgi?id=36426 > + // FIXME: jeremy@chromium.org - for debugging purposes, should be removed No need to add an email name here. (File history will reveal who added this if there is a question and is typical WK style to not add this.) > diff --git a/WebKit/chromium/src/ResourceHandle.cpp b/WebKit/chromium/src/ResourceHandle.cpp > + enum ConnectionState { > + CONNECTION_STATE_NEW, > + CONNECTION_STATE_STARTED, > + CONNECTION_STATE_RECEIVED_RESPONSE, > + CONNECTION_STATE_RECEIVING_DATA, > + CONNECTION_STATE_FINISHED_LOADING, > + CONNECTION_STATE_CANCELED, > + CONNECTION_STATE_FAILED, > + }; Enum members should user InterCaps with an initial capital letter. > ResourceRequest m_request; > ResourceHandle* m_owner; > ResourceHandleClient* m_client; > OwnPtr<WebURLLoader> m_loader; > + > + // Used for sanity checking to make sure we don't experience illegal state > + // transitions. > + ConnectionState m_state; > }; > > void ResourceHandleInternal::start() > { > + if (!(m_state == CONNECTION_STATE_NEW)) Why not simply if (m_state != CONNECTION_STATE_NEW) ? > + CRASH(); > @@ -135,6 +161,9 @@ void ResourceHandleInternal::didReceiveData( > WebURLLoader*, const char* data, int dataLength) > { > ASSERT(m_client); > + if (!(m_state == CONNECTION_STATE_RECEIVED_RESPONSE || m_state == CONNECTION_STATE_RECEIVING_DATA)) > + CRASH(); > + m_state = CONNECTION_STATE_RECEIVING_DATA; > > // FIXME(yurys): it looks like lengthReceived is always the same as > // dataLength and that the latter parameter can be eliminated. > @@ -145,12 +174,17 @@ void ResourceHandleInternal::didReceiveData( > void ResourceHandleInternal::didFinishLoading(WebURLLoader*) > { > ASSERT(m_client); > + if (!(m_state == CONNECTION_STATE_RECEIVED_RESPONSE > + || m_state == CONNECTION_STATE_RECEIVING_DATA)) There is no need to fall within 80 columns (and this line is no longer than the same line just a few lines above in this patch).
Created attachment 53253 [details] Fix review comments
Attachment 53253 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1577495
Created attachment 53255 [details] Fix Chromium compile
Comment on attachment 53255 [details] Fix Chromium compile Clearing flags on attachment: 53255 Committed r57520: <http://trac.webkit.org/changeset/57520>
Created attachment 56230 [details] Remove obsolete debug code
Comment on attachment 56230 [details] Remove obsolete debug code Creating a new bug to land this: Issue 39213