Bug 29425 - [Qt] SIGSEGV after WebCore::Frame::loader()
Summary: [Qt] SIGSEGV after WebCore::Frame::loader()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Critical
Assignee: Benjamin Poulain
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-09-18 07:43 PDT by Tor Arne Vestbø
Modified: 2009-11-02 10:09 PST (History)
6 users (show)

See Also:


Attachments
Backtrace (3.31 KB, text/plain)
2009-10-07 05:17 PDT, Jędrzej Nowacki
no flags Details
patch v1 (4.23 KB, patch)
2009-10-07 08:23 PDT, Jędrzej Nowacki
jedrzej.nowacki: review-
jedrzej.nowacki: commit-queue-
Details | Formatted Diff | Diff
patch (5.74 KB, patch)
2009-10-09 01:16 PDT, Jędrzej Nowacki
zecke: review-
Details | Formatted Diff | Diff
patch v3 (7.54 KB, patch)
2009-10-12 06:39 PDT, Jędrzej Nowacki
abarth: review-
Details | Formatted Diff | Diff
patch v3 (Alternative) (7.51 KB, patch)
2009-10-29 03:25 PDT, Jędrzej Nowacki
jedrzej.nowacki: review-
jedrzej.nowacki: commit-queue-
Details | Formatted Diff | Diff
patch v4 (7.54 KB, patch)
2009-10-29 04:59 PDT, Jędrzej Nowacki
abarth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch v5 (7.55 KB, patch)
2009-11-02 01:37 PST, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Arne Vestbø 2009-09-18 07:43:10 PDT
This bug report originated from issue QTBUG-3876
<http://bugreports.qt.nokia.com/browse/QTBUG-3876>

--- Description ---

QWebView crashes (on Linux at least) when calling stop().

Test case:
//--------------------------------------------

class WebView : public QWebView
{
    Q_OBJECT

public:
    WebView(QWidget *parent = 0);

private slots:
    void loadingStarted();
    void reloadingStarted();
    void loading(int progress);

private:
    int counter;
    QTimer *timer;
};

WebView::WebView(QWidget *parent)
    : QWebView(parent), counter(0)
{
    timer = new QTimer(this);
    timer->setSingleShot(true);
    timer->setInterval(5000);

    connect(timer, SIGNAL(timeout()), this, SLOT(reloadingStarted()));
    connect(this, SIGNAL(loadProgress(int)), this, SLOT(loading(int)));
    connect(this, SIGNAL(loadStarted()), this, SLOT(loadingStarted()));
}

void WebView::loadingStarted()
{
    qDebug() << "Started...";
    counter++;
}

void WebView::reloadingStarted()
{
    qDebug() << "Reloading...";
    reload();
}

void WebView::loading(int progress)
{
    qDebug() << "loading: " << progress;
    if (progress >= 50 && counter == 2) {
        stop();
        timer->start();
    }
}

int main(int argc, char *argv[])
{
    QApplication app(argc,argv);
    WebView *view = new WebView();
    QUrl url = QUrl("http://www.qtsoftware.com");
    view->setUrl(url);
    view->load(url);
    view->show();
    return app.exec();
}

//---------------------------------------
Comment 1 Jędrzej Nowacki 2009-10-07 05:17:17 PDT
Created attachment 40779 [details]
Backtrace

I have reproduced crash. It is caused by call QWebView::stop() slot in method triggered by QWebView::loadProgress signal.

I see three solutions:
 1. Document that "feature". Why actually you want to call stop from loadProgress?
 2. Save local reference to frame inside ProgressTracker::incremetProgress()
 3. Add few checks for null pointer

1. I believe that it is not the best bug policy... :-)
2. After ::stop() developer will receive new estimations, which is rather weired
3. Let's go to work ;-)
Comment 2 Jędrzej Nowacki 2009-10-07 08:23:34 PDT
Created attachment 40789 [details]
patch v1

Two null pointer checks.
New autotest.
Comment 3 Antonio Gomes 2009-10-07 08:40:20 PDT
(In reply to comment #2)
> Created an attachment (id=40789) [details]
> patch v1
> 
> Two null pointer checks.
> New autotest.

although patch and autotest looks good (i am not a reviewer), generally i think it is something to be avoided to rely on internet for autotests.

+    QWebView view;
+    WebViewCrashTest tester(&view);
+    QUrl url("http://www.qtsoftware.com");
+    view.setUrl(url);
+    view.load(url);
Comment 4 Benjamin Poulain 2009-10-07 08:42:51 PDT
You could use QTRY_VERIFY (of tests/util.h) instead of QVERIFY in the test, so it is more likely to work on the slow test machines (and you can reduce the time of qWait()).

You do not need view.setUrl(url);, view.load(url); should be enough.
Comment 5 Jędrzej Nowacki 2009-10-08 00:49:27 PDT
Comment on attachment 40789 [details]
patch v1

I will change it
Comment 6 Jędrzej Nowacki 2009-10-09 01:16:09 PDT
Created attachment 40937 [details]
patch

New & better patch
Comment 7 Holger Freyther 2009-10-10 06:17:34 PDT
Comment on attachment 40937 [details]
patch


> diff --git a/WebCore/loader/ProgressTracker.cpp b/WebCore/loader/ProgressTracker.cpp
> index e682b9b..5f6892c 100644
> --- a/WebCore/loader/ProgressTracker.cpp
> +++ b/WebCore/loader/ProgressTracker.cpp
> @@ -178,6 +178,8 @@ void ProgressTracker::incrementProgress(unsigned long identifier, const char*, i
>          return;
>      
>      m_originatingProgressFrame->loader()->client()->willChangeEstimatedProgress();
> +    if (!m_originatingProgressFrame)
> +        return; // Client cancelled request.
>      
>      unsigned bytesReceived = length;
>      double increment, percentOfRemainingBytes;
> @@ -228,6 +230,8 @@ void ProgressTracker::incrementProgress(unsigned long identifier, const char*, i
>          }
>      }
>      
> +    if (!m_originatingProgressFrame)
> +        return; // Client cancelled request in postProgressEstimateChangedNotification.
>      m_originatingProgressFrame->loader()->client()->didChangeEstimatedProgress();


In both cases you are stepping back through a deleted object? Have you considered doing the
usual trick of having a RefPtr<Frame> frame = m_originatingProgressFrame; in the method entry?

This is already done in some of the methods of the ProgressTracker.




> +
> +    WebViewCrashTest(QWebView *view) : v(view), executed(false)


coding style violation, the c++ initializer list must be one variable per line.. and the naming of variables is also defined by the coding style.



       WebViewCrashTest(QWebView *view)
           : m_view(view)
           , m_executed(false)


> +// Should not crash.
> +void tst_QWebView::crashTests()
> +{
> +    // Test if loading can be stopped in loadProgress handler without crash.
> +    // Test page should have frames.
> +    QWebView view;
> +    WebViewCrashTest tester(&view);
> +    QUrl url("qrc:///data/index.html");
> +    view.load(url);
> +    QTRY_VERIFY(tester.executed); // If fail it means that the test wasn't executed.


stupid question. how robust is that? should you wait for a loadFinished() signal or such?
Comment 8 Jędrzej Nowacki 2009-10-12 06:39:54 PDT
Created attachment 41036 [details]
patch v3

> In both cases you are stepping back through a deleted object? 
Yes, it is true (null pointer access).

> Have you considered doing the usual trick of having a 
> RefPtr<Frame> frame = m_originatingProgressFrame; in the method entry?

Yes, I have. Actually it was my first solution. But than I found an issue. Should we call the didChangeEstimatedProgress hook if in the postProgressEstimateChangedNotification developer try to stop loading? Moreover the estimations are strange because loading is canceled (reset()). I found it a bit confusing. But after a discussion in office with Jocelyn we decided that the didChangeEstimatedProgress might be used as a cleanup function, so it is better to call it. The new patch use RefPtr pattern.

> coding style violation, the c++ initializer list must be one variable per
> line.. and the naming of variables is also defined by the coding style.
Changed.

>> +    QTRY_VERIFY(tester.executed); // If fail it means that the test wasn't executed.
> stupid question. how robust is that? should you wait for a loadFinished()
> signal or such?
QTRY_VERIFY could wait up to 5 seconds and the web pages are compiled into resources ("qrc://..."). 
Even a slow machine should be fast enough :-). If you look into qtwebkit autotests suite there is a lot of
similar solutions.
Comment 9 Eric Seidel (no email) 2009-10-19 14:43:30 PDT
The ChangeLog is missing the bug url.  prepare-ChagneLog --bug 29425 would have inserted it automatically.
Comment 10 Jędrzej Nowacki 2009-10-20 08:28:25 PDT
(In reply to comment #9)
> The ChangeLog is missing the bug url.  prepare-ChagneLog --bug 29425 would have
> inserted it automatically.

Could you check one more time? In both Changelog's files there is the bug's url.
Btw. thanks for hint, next time I will use the option.
Comment 11 Eric Seidel (no email) 2009-10-26 16:14:27 PDT
I think Adam Barth and a Qt person should look at this.
Comment 12 Adam Barth 2009-10-27 08:57:22 PDT
Comment on attachment 41036 [details]
patch v3

This looks good based on my understanding of hos this works.

 180     RefPtr<Frame> frame(m_originatingProgressFrame);  // Delete protection.

We prefer RefPtr<Frame> frame = m_originatingProgressFrame;
Also, the comment is unnecessary.

 181     FrameLoader* loader = frame->loader();  //Shortcut.
 
I wouldn't bother with this short cut.

 183     loader->client()->willChangeEstimatedProgress();

Why did we add this client callback?
Comment 13 Jędrzej Nowacki 2009-10-29 03:25:38 PDT
Created attachment 42088 [details]
patch v3 (Alternative)

Alternative patch including all suggestions. Personally I prefer the previous one (patch v3), but from technical point of view it is the same.

(In reply to comment #12)
> (From update of attachment 41036 [details])
> This looks good based on my understanding of hos this works.
> 
>  180     RefPtr<Frame> frame(m_originatingProgressFrame);  // Delete
> protection.
> 
> We prefer RefPtr<Frame> frame = m_originatingProgressFrame;
> Also, the comment is unnecessary.
Could you point me to the rule? I have checked coding style guide, the mailing list discussion about unwritten rules of webkit style, source code of RefPtr and PassRefPtr. In the FrameLoader.cpp this is a standard pattern to call the constructor (RefPtr<Frame> protect(m_frame);) 


>  181     FrameLoader* loader = frame->loader();  //Shortcut.
> 
> I wouldn't bother with this short cut.
It is easier to read then xxx.yyyy->zzz->aaa();
 
>  183     loader->client()->willChangeEstimatedProgress();
> 
> Why did we add this client callback?
I guess... To checkout an old version of estimation, before it will be overwritten? It would be nice to have comments :-)


Thanks for reviewing.
Comment 14 Antonio Gomes 2009-10-29 04:37:18 PDT
(In reply to comment #13)
> Created an attachment (id=42088) [details]
> patch v3 (Alternative)
> 

maybe the below is part of a previous patch ?

+        Two null pointer checks were added in the ProgressTracker class.
+
Comment 15 Jędrzej Nowacki 2009-10-29 04:45:58 PDT
Comment on attachment 42088 [details]
patch v3 (Alternative)

Bad change log
Comment 16 Jędrzej Nowacki 2009-10-29 04:59:05 PDT
Created attachment 42091 [details]
patch v4

Change log was updated
Comment 17 Adam Barth 2009-10-29 07:21:02 PDT
(In reply to comment #13)
> >  183     loader->client()->willChangeEstimatedProgress();
> > 
> > Why did we add this client callback?
>
> I guess... To checkout an old version of estimation, before it will be
> overwritten? It would be nice to have comments :-)

That seems unrelated to the crash.  It's possible we should add that callback, but we should do that another bug/patch.
Comment 18 Jędrzej Nowacki 2009-10-29 07:40:50 PDT
(In reply to comment #17)
> That seems unrelated to the crash.  It's possible we should add that callback,
> but we should do that another bug/patch.
Oh, I see. I didn't created the hook. I replaced all "m_originatingProgressFrame" with "frame", and diff got confused and produced a bit messy output. 

So this:
-    m_originatingProgressFrame->loader()->client()->willChangeEstimatedProgress();
+    RefPtr<Frame> frame = m_originatingProgressFrame;
+
+    frame->loader()->client()->willChangeEstimatedProgress();

could be translated to:
+    RefPtr<Frame> frame = m_originatingProgressFrame;
+
-    m_originatingProgressFrame->loader()->client()->willChangeEstimatedProgress();
+    frame->loader()->client()->willChangeEstimatedProgress();
Comment 19 Adam Barth 2009-10-29 08:01:51 PDT
Comment on attachment 42091 [details]
patch v4

Ah, of course!  I see it now.  Thanks.  :)
Comment 20 WebKit Commit Bot 2009-10-29 10:42:52 PDT
Comment on attachment 42091 [details]
patch v4

Rejecting patch 42091 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adam Barth', '--force']" exit_code: 1
Last 500 characters of output:
dex.html
patching file WebKit/qt/tests/qwebview/qwebview.pro
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED -- saving rejects to file WebKit/qt/tests/qwebview/qwebview.pro.rej
patching file WebKit/qt/tests/qwebview/tst_qwebview.cpp
Hunk #1 succeeded at 20 (offset 1 line).
Hunk #2 FAILED at 44.
Hunk #3 succeeded at 150 with fuzz 1 (offset -19 lines).
1 out of 3 hunks FAILED -- saving rejects to file WebKit/qt/tests/qwebview/tst_qwebview.cpp.rej
patching file WebKit/qt/tests/qwebview/tst_qwebview.qrc
Comment 21 Eric Seidel (no email) 2009-10-29 10:44:33 PDT
My apologies for the delay.  The commit-queue was down for several hours due to bug 30869.  I hope to fix that issue today.  It looks like this patch is simply out of date at this point and will need to be either updated here, or landed by hand.
Comment 22 Jędrzej Nowacki 2009-10-30 02:58:41 PDT
(In reply to comment #21)
> My apologies for the delay.  The commit-queue was down for several hours due to
> bug 30869.  I hope to fix that issue today.  It looks like this patch is simply
> out of date at this point and will need to be either updated here, or landed by
> hand.

OK, I'll remade and post it again :-)
Comment 23 Jędrzej Nowacki 2009-11-02 01:37:02 PST
Created attachment 42308 [details]
patch v5

Updated
Comment 24 WebKit Commit Bot 2009-11-02 10:09:30 PST
Comment on attachment 42308 [details]
patch v5

Clearing flags on attachment: 42308

Committed r50415: <http://trac.webkit.org/changeset/50415>
Comment 25 WebKit Commit Bot 2009-11-02 10:09:36 PST
All reviewed patches have been landed.  Closing bug.