Bug 83565 – Crash when reloading page while it is loading

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: WebKit Qt

[Jonathan Liu]

The crash is caused by a null pointer dereference in Source/WebCore/loader/ResourceLoader.cpp in the function ResourceLoader::didCancel().
m_documentLoader->cancelPendingSubstituteLoad(this) is called without checking if m_documentLoader is null.

[Jonathan Liu]

Created attachment 136430 [details]
Stack trace of the crash

[Jonathan Liu]

Created attachment 136432 [details]
Patch to fix possible null pointer dereference

[WebKit Review Bot]

Attachment 136432 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:3:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Liu]

Created attachment 136433 [details]
Patch to fix possible null pointer dereference

[WebKit Review Bot]

Comment on attachment 136433 [details]
Patch to fix possible null pointer dereference

Rejecting attachment 136433 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Nate Chapin]

Comment on attachment 136433 [details]
Patch to fix possible null pointer dereference

View in context: https://bugs.webkit.org/attachment.cgi?id=136433&action=review

> Source/WebCore/loader/ResourceLoader.cpp:385
> -
> -        m_documentLoader->cancelPendingSubstituteLoad(this);
> +        if (m_documentLoader)
> +            m_documentLoader->cancelPendingSubstituteLoad(this);

We don't typically null-check ResourceLoader::m_documentLoader. The preferred technique is to check m_reachedTerminalState, which should catch every case where m_documentLoader is null.

It would be great if we had a layout test for this. At the very least, we should figure out where in here we're calling releasedResources(), which nulls m_documentLoader and sets m_reachedTerminalState to true.

[Brady Eidson]

(In reply to comment #6)
> It would be great if we had a layout test for this. At the very least, we should figure out where in here we're calling releasedResources(), which nulls m_documentLoader and sets m_reachedTerminalState to true.

Not only would it be great; We need one.

The original report is written as if this is easily reproducible, but I've never seen this myself.  And yes, I reload pages while they're still loading.

A better start to this would be providing actual steps to reproduce the bug, as well as details of what configuration you're using.

[Jonathan Liu]

The crash occurs with Qt 4.8.1 which has QtWebKit 2.2.0. Compiler is MinGW GCC 4.4.0. It was reproduced using the code and steps linked by the URL.

ResourceLoader::didFinishLoading is calling releaseResources() which nulls m_documentLoader and sets m_reachedTerminalState to true. ResourceLoader::didCancel is called afterwards on the same resource loader which results in the crash.

[Brady Eidson]

(In reply to comment #8)
> The crash occurs with Qt 4.8.1 which has QtWebKit 2.2.0. Compiler is MinGW GCC 4.4.0. It was reproduced using the code and steps linked by the URL.

I went to http://stackoverflow.com/questions/10072774/qtwebkit-2-2-segfaults-when-loading-specific-website

I saw a link to a URL (http://www.mayaposch.com/index.php) but no further instructions.

You mention in this bug report "reload a page while it is loading", but those instructions don't exist at the stackoverflow.com URL.

I can observe that this does not reproduce in Safari with a ToT WebKit build or in Chrome.  This is likely Qt port specific.

> ResourceLoader::didFinishLoading is calling releaseResources() which nulls m_documentLoader and sets m_reachedTerminalState to true. ResourceLoader::didCancel is called afterwards on the same resource loader which results in the crash.

Why do the other ports not crash here, then?

This change - while seemingly small and obvious on the surface - is in some of the most complicated code in the project whose edge cases are notoriously poorly tested.

At the minimum we should try to understand why Qt is hitting this but other platforms aren't.  We should also strive to have a reduced test case that remonstrates the crash which will go far towards making an automated regression test.

[Jonathan Liu]

Created attachment 144216 [details]
Test case for crash

[Jonathan Liu]

Created attachment 144220 [details]
 Test case for crash

Added ability to run test case non-interactively by defining INTERACTIVE_TEST to 0.

[Jonathan Liu]

The conditions leading to the crash seems to be the following:
-QWebView is configured to load icons
-A website with a favicon is loaded
-A meta http-equiv refresh with 0 delay is contained in the web page - this results in the page load being canceled
-The slot on_webView_loadFinished which handles the QWebView::loadFinished signal shows a message box which re-enters the event loop and delays returning control to the sender of the loadFinished signal for some time. During this time, other signals may fire and run in a different order than normally expected because control has not yet returned to the sender of the loadFinished signal.

I'm not sure the test case fits into the current layout test framework. Can you run custom Qt GUI code in the test framework and connect to a locally started server serving a favicon and webpage? Some guidance would be appreciated.

[Jonathan Liu]

This crash doesn't occur using QtWebKit included with Qt 5. Qt 4 is using a really old version of QtWebKit it seems.