Bug 36426 – Chromium: Crash in WebCore::ArchiveFactory::isArchiveMimeType

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jeremy Moskovich]

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

[Jeremy Moskovich]

Created attachment 51426 [details]
Sanity Checks

Code to try to track down the crash.

[Jeremy Orlow]

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 @@
> [phone]  Jeremy Moskovich  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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([e-mail]): 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 @@
> [phone]  Jeremy Moskovich  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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.

[Brady Eidson]

> +#if PLATFORM(CHROMIUM)
> +    // https://bugs.webkit.org/show_bug.cgi?id=36426
> +    // FIXME([e-mail]): 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.

[Jeremy Moskovich]

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

[Jeremy Moskovich]

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.

[Jeremy Orlow]

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.

[WebKit Commit Bot]

Comment on attachment 51483 [details]
ASSERT() -> CRASH()

Clearing flags on attachment: 51483

Committed r56453: <http://trac.webkit.org/changeset/56453>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jeremy Moskovich]

Reopening to track removal of debug code.

[Eric Seidel (no email)]

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.

[Dmitry Titov]

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>

[Eric Seidel (no email)]

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...

[Eric Seidel (no email)]

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.

[Jeremy Moskovich]

Created attachment 52151 [details]
Loosen sanity check - allow multiple consecutive calls to connectionDidReceiveResponse

Updated patch for debugging purposes.

[Jeremy Moskovich]

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.

[David Levin]

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: [e-mail] - 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).

[Jeremy Moskovich]

Created attachment 53253 [details]
Fix review comments

[WebKit Review Bot]

Attachment 53253 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1577495

[Jeremy Moskovich]

Created attachment 53255 [details]
Fix Chromium compile

[WebKit Commit Bot]

Comment on attachment 53255 [details]
Fix Chromium compile

Clearing flags on attachment: 53255

Committed r57520: <http://trac.webkit.org/changeset/57520>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jeremy Moskovich]

Created attachment 56230 [details]
Remove obsolete debug code

[Jeremy Moskovich]

Comment on attachment 56230 [details]
Remove obsolete debug code

Creating a new bug to land this: Issue 39213