Bug 26969 – Fix ASSERT in WebCore::DocumentThreadableLoader::didFail

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Robert Hogan]

Created attachment 32259 [details]
backtrace

Following the instructions at  http://meyerhome.net/~ben/frametest/ (see https://bugs.webkit.org/show_bug.cgi?id=25283) results in hitting an assert in WebCore::DocumentThreadableLoader::didFail.

The problem is that DocumentThreadableLoader calls WebCore::SubresourceLoader::create with a request containing an "OPTIONS" httpMethod() (set in WebCore::XMLHttpRequest::makeCrossOriginAccessRequestWithPreflight) . This method is not supported by QNetworkReplyHandler, which then calls didFail(). This call returns it to DocumentThreadableLoader::didFail, where of course the m_loader object is still null, since it is still being created.

I think didFail() should expect to handle calls when the m_loader object is still null. The m_loader object is inaccessible to QNetworkReplyHandler and it seems reasonable to call didFail() if the http method is not supported and therefore request has failed.

The following works for me:

void DocumentThreadableLoader::didFail(SubresourceLoader* loader, const ResourceError& error)
{
    ASSERT(m_client);
    // If the httpMethod() of the request passed to SubresourceLoader::create is not supported by
    // the client we must expect to call didFail() while m_loader is still null.
    ASSERT_UNUSED(loader, loader == m_loader || !m_loader);

    m_client->didFail(error);
}

[Robert Hogan]

Created attachment 32260 [details]
proposed patch

[Maciej Stachowiak]

Comment on attachment 32260 [details]
proposed patch

r=me

I would advise making the comment a bit shorter if possible. I don't think mentioning the bug number in the code is necessary, as long as it's in the ChangeLog.

[Robert Hogan]

Created attachment 32263 [details]
patch updated with shorter inline comment

[Maciej Stachowiak]

Comment on attachment 32263 [details]
patch updated with shorter inline comment

r=me

[Eric Seidel (no email)]

The ChangeLog line is wrong:

2009-07-04  Robert Hogan <[e-mail])

The script would have left 2 spaces between your name and < and would have closed the email address with a >

[Robert Hogan]

Created attachment 32325 [details]
updated changelog

oh gack - i'm using git so have been creating the changelog entries manually, a strategy that is clearly paying dividends.. :-/

[Eric Seidel (no email)]

prepare-ChangeLog works fine with git.  it even has some git-specific options:
prepare-ChangeLog --help
prepare-ChangeLog [-d|--diff] [-h|--help] [-o|--open] [--git-commit=<committish>] [--git-reviewer=<name>] [svndir1 [svndir2 ...]]
  -d|--diff      Spew diff to stdout when running
  --git-commit   Populate the ChangeLogs from the specified git commit
  --git-index    Populate the ChangeLogs from the git index only
  --git-reviewer When populating the ChangeLogs from a git commit claim that the spcified name reviewed the change.
                 This option is useful when the git commit lacks a Signed-Off-By: line
  -h|--help      Show this help message
  -o|--open      Open ChangeLogs in an editor when done
  --[no-]update  Update ChangeLogs from svn before adding entry (default: update)

commit-log-editor also works with git as well. :)

resolve-ChangeLogs -f
is helpful for when git mis-merges ChangeLogs.

[Eric Seidel (no email)]

Comment on attachment 32325 [details]
updated changelog

We normally use full english words, like "Constructor" instead of ctor.

This looks fine.  I assume this fails on the Qt or Gtk builds?  In which case it would have made sense to make a layout test for them.  I will OK it as is though.

[Alexey Proskuryakov]

From the bug description, it seems that existing layout tests for cross-origin XMLHttpRequest may trigger this crash on platforms that don't support OPTIONS method.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45935.

[Yael]

*** Bug 28242 has been marked as a duplicate of this bug. ***