Bug 36426 - Chromium: Crash in WebCore::ArchiveFactory::isArchiveMimeType
Summary: Chromium: Crash in WebCore::ArchiveFactory::isArchiveMimeType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Jeremy Moskovich
URL:
Keywords:
Depends on:
Blocks: 39213
  Show dependency treegraph
 
Reported: 2010-03-21 06:43 PDT by Jeremy Moskovich
Modified: 2010-05-17 03:39 PDT (History)
11 users (show)

See Also:


Attachments
Sanity Checks (5.55 KB, patch)
2010-03-23 08:03 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
ASSERT() -> CRASH() (5.74 KB, patch)
2010-03-24 01:10 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Loosen sanity check - allow multiple consecutive calls to connectionDidReceiveResponse (5.79 KB, patch)
2010-03-31 04:51 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Allow didRecieveResponse/didReceiveData interleaving in the case of multipart responses (5.89 KB, patch)
2010-04-13 07:16 PDT, Jeremy Moskovich
levin: review-
levin: commit-queue-
Details | Formatted Diff | Diff
Fix review comments (5.79 KB, patch)
2010-04-13 08:19 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Fix Chromium compile (5.94 KB, patch)
2010-04-13 08:39 PDT, Jeremy Moskovich
no flags Details | Formatted Diff | Diff
Remove obsolete debug code (2.11 KB, text/plain)
2010-05-17 03:21 PDT, Jeremy Moskovich
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2010-03-21 06:43:55 PDT
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
Comment 1 Jeremy Moskovich 2010-03-23 08:03:39 PDT
Created attachment 51426 [details]
Sanity Checks

Code to try to track down the crash.
Comment 2 Jeremy Orlow 2010-03-23 08:12:14 PDT
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.
Comment 3 Brady Eidson 2010-03-23 10:02:48 PDT
> +#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.
Comment 4 Jeremy Moskovich 2010-03-24 01:10:24 PDT
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
Comment 5 Jeremy Moskovich 2010-03-24 01:54:31 PDT
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 6 Jeremy Orlow 2010-03-24 02:54:33 PDT
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 7 WebKit Commit Bot 2010-03-24 13:20:39 PDT
Comment on attachment 51483 [details]
ASSERT() -> CRASH()

Clearing flags on attachment: 51483

Committed r56453: <http://trac.webkit.org/changeset/56453>
Comment 8 WebKit Commit Bot 2010-03-24 13:20:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Jeremy Moskovich 2010-03-24 13:30:23 PDT
Reopening to track removal of debug code.
Comment 10 Eric Seidel (no email) 2010-03-24 14:31:39 PDT
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.
Comment 11 Dmitry Titov 2010-03-24 16:30:45 PDT
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>
Comment 12 Eric Seidel (no email) 2010-03-24 16:32:18 PDT
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...
Comment 13 Eric Seidel (no email) 2010-03-24 16:33:46 PDT
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.
Comment 14 Jeremy Moskovich 2010-03-31 04:51:45 PDT
Created attachment 52151 [details]
Loosen sanity check - allow multiple consecutive calls to connectionDidReceiveResponse

Updated patch for debugging purposes.
Comment 15 Jeremy Moskovich 2010-04-13 07:16:19 PDT
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 16 David Levin 2010-04-13 07:46:24 PDT
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).
Comment 17 Jeremy Moskovich 2010-04-13 08:19:39 PDT
Created attachment 53253 [details]
Fix review comments
Comment 18 WebKit Review Bot 2010-04-13 08:30:32 PDT
Attachment 53253 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1577495
Comment 19 Jeremy Moskovich 2010-04-13 08:39:06 PDT
Created attachment 53255 [details]
Fix Chromium compile
Comment 20 WebKit Commit Bot 2010-04-13 10:08:42 PDT
Comment on attachment 53255 [details]
Fix Chromium compile

Clearing flags on attachment: 53255

Committed r57520: <http://trac.webkit.org/changeset/57520>
Comment 21 WebKit Commit Bot 2010-04-13 10:08:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Jeremy Moskovich 2010-05-17 03:21:34 PDT
Created attachment 56230 [details]
Remove obsolete debug code
Comment 23 Jeremy Moskovich 2010-05-17 03:38:56 PDT
Comment on attachment 56230 [details]
Remove obsolete debug code

Creating a new bug to land this: Issue 39213