Bug 83565 - Crash when reloading page while it is loading
Summary: Crash when reloading page while it is loading
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL: http://stackoverflow.com/questions/10...
Keywords:
Depends on:
Blocks: 68616
  Show dependency treegraph
 
Reported: 2012-04-10 05:13 PDT by Jonathan Liu
Modified: 2013-03-07 04:03 PST (History)
5 users (show)

See Also:


Attachments
Stack trace of the crash (7.25 KB, text/plain)
2012-04-10 05:16 PDT, Jonathan Liu
no flags Details
Patch to fix possible null pointer dereference (1.19 KB, patch)
2012-04-10 05:24 PDT, Jonathan Liu
no flags Details | Formatted Diff | Diff
Patch to fix possible null pointer dereference (1.20 KB, patch)
2012-04-10 05:29 PDT, Jonathan Liu
japhet: review-
net147: commit-queue?
Details | Formatted Diff | Diff
Test case for crash (1.46 KB, text/x-c++)
2012-05-26 22:08 PDT, Jonathan Liu
no flags Details
Test case for crash (1.85 KB, text/x-c++)
2012-05-27 00:38 PDT, Jonathan Liu
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Liu 2012-04-10 05:13:49 PDT
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.
Comment 1 Jonathan Liu 2012-04-10 05:16:05 PDT
Created attachment 136430 [details]
Stack trace of the crash
Comment 2 Jonathan Liu 2012-04-10 05:24:42 PDT
Created attachment 136432 [details]
Patch to fix possible null pointer dereference
Comment 3 WebKit Review Bot 2012-04-10 05:27:10 PDT
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.
Comment 4 Jonathan Liu 2012-04-10 05:29:45 PDT
Created attachment 136433 [details]
Patch to fix possible null pointer dereference
Comment 5 WebKit Review Bot 2012-04-10 06:44:50 PDT
Comment on attachment 136433 [details]
Patch to fix possible null pointer dereference

Rejecting attachment 136433 [details] from review queue.

net147@gmail.com 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.
Comment 6 Nate Chapin 2012-04-10 09:39:40 PDT
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.
Comment 7 Brady Eidson 2012-04-10 13:28:31 PDT
(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.
Comment 8 Jonathan Liu 2012-04-10 16:16:54 PDT
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.
Comment 9 Brady Eidson 2012-04-10 16:30:20 PDT
(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.
Comment 10 Jonathan Liu 2012-05-26 22:08:40 PDT
Created attachment 144216 [details]
Test case for crash
Comment 11 Jonathan Liu 2012-05-27 00:38:37 PDT
Created attachment 144220 [details]
 Test case for crash

Added ability to run test case non-interactively by defining INTERACTIVE_TEST to 0.
Comment 12 Jonathan Liu 2012-09-24 07:16:11 PDT
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.
Comment 13 Jonathan Liu 2013-03-07 04:03:00 PST
This crash doesn't occur using QtWebKit included with Qt 5. Qt 4 is using a really old version of QtWebKit it seems.