[webkit-reviews] review granted: [Bug 36426] Chromium: Crash in WebCore::ArchiveFactory::isArchiveMimeType : [Attachment 51426] Sanity Checks

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has granted Jeremy Moskovich
<playmobil at google.com>'s request for review:
Bug 36426: Chromium: Crash in WebCore::ArchiveFactory::isArchiveMimeType
https://bugs.webkit.org/show_bug.cgi?id=36426

Attachment 51426: Sanity Checks
https://bugs.webkit.org/attachment.cgi?id=51426&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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 at 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 at 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 at 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 at 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 at 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.