Bug 29425 – [Qt] SIGSEGV after WebCore::Frame::loader()

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Tor Arne Vestbø]

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();
}

//---------------------------------------

[Jędrzej Nowacki]

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 ;-)

[Jędrzej Nowacki]

Created attachment 40789 [details]
patch v1

Two null pointer checks.
New autotest.

[Antonio Gomes]

(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);

[Benjamin Poulain]

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.

[Jędrzej Nowacki]

Comment on attachment 40789 [details]
patch v1

I will change it

[Jędrzej Nowacki]

Created attachment 40937 [details]
patch

New & better patch

[Holger Freyther]

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?

[Jędrzej Nowacki]

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.

[Eric Seidel (no email)]

The ChangeLog is missing the bug url.  prepare-ChagneLog --bug 29425 would have inserted it automatically.

[Jędrzej Nowacki]

(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.

[Eric Seidel (no email)]

I think Adam Barth and a Qt person should look at this.

[Adam Barth]

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?

[Jędrzej Nowacki]

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.

[Antonio Gomes]

(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.
+

[Jędrzej Nowacki]

Comment on attachment 42088 [details]
patch v3 (Alternative)

Bad change log

[Jędrzej Nowacki]

Created attachment 42091 [details]
patch v4

Change log was updated

[Adam Barth]

(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.

[Jędrzej Nowacki]

(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();

[Adam Barth]

Comment on attachment 42091 [details]
patch v4

Ah, of course!  I see it now.  Thanks.  :)

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[Jędrzej Nowacki]

(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 :-)

[Jędrzej Nowacki]

Created attachment 42308 [details]
patch v5

Updated

[WebKit Commit Bot]

Comment on attachment 42308 [details]
patch v5

Clearing flags on attachment: 42308

Committed r50415: <http://trac.webkit.org/changeset/50415>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.