Bug 39670 - [Qt] WebView::setUrl crashes after Qt4.7
Summary: [Qt] WebView::setUrl crashes after Qt4.7
Status: RESOLVED DUPLICATE of bug 49216
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P1 Critical
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-05-25 04:41 PDT by xxx
Modified: 2011-04-19 05:15 PDT (History)
9 users (show)

See Also:


Attachments
Qt source code to reproduce the problem (1.80 KB, application/octet-stream)
2010-05-25 04:41 PDT, xxx
no flags Details
Patch (1.54 KB, patch)
2010-10-29 17:08 PDT, Jan Erik Hanssen
no flags Details | Formatted Diff | Diff
Patch (12.09 KB, patch)
2010-11-05 19:08 PDT, Jan Erik Hanssen
no flags Details | Formatted Diff | Diff
Patch (5.36 KB, patch)
2010-12-07 18:20 PST, Jan Erik Hanssen
abarth: review-
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description xxx 2010-05-25 04:41:35 PDT
Created attachment 57005 [details]
Qt source code to reproduce the problem

I attach a use case that causes WebKit to crash. Just press on the button, which causes setUrl to be called twice, once directly, and the second one through a user event.
I get the same crash with both Qt4.7 beta1 and latest Qt 4.8 from git.
Fyi, if I remove the link to css from the index.html, it does not crash anymore. On the other hand, it crashes no matter whether the css file exists or not

This is what I get with valgrind

==6355== Invalid write of size 4 
==6355== at 0x47FF055: WebCore::CachedResource::setDocLoader(WebCore::DocLoader*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x480E2C2: WebCore::DocLoader::~DocLoader() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x4649988: void WTF::deleteOwnedPtr<WebCore::DocLoader>(WebCore::DocLoader*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x4646BAA: WTF::OwnPtr<WebCore::DocLoader>::clear() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x4634342: WebCore::Document::~Document() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x46327E1: WebCore::Document::removedLastRef() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x41F92B0: WebCore::TreeShared<WebCore::Node>::deref() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x4260A8E: void WTF::derefIfNotNull<WebCore::Document>(WebCore::Document*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x462CF20: WTF::RefPtr<WebCore::Document>::~RefPtr() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x483EFEC: WebCore::Loader::Host::didFail(WebCore::SubresourceLoader*, bool) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x483F8B2: WebCore::Loader::Host::cancelRequests(WebCore::DocLoader*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x483DE85: WebCore::Loader::cancelRequests(WebCore::DocLoader*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== Address 0x110 is not stack'd, malloc'd or (recently) free'd 
==6355== 
==6355== 
==6355== Process terminating with default action of signal 11 (SIGSEGV) 
==6355== Access not within mapped region at address 0x110 
==6355== at 0x47FF055: WebCore::CachedResource::setDocLoader(WebCore::DocLoader*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x480E2C2: WebCore::DocLoader::~DocLoader() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x4649988: void WTF::deleteOwnedPtr<WebCore::DocLoader>(WebCore::DocLoader*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x4646BAA: WTF::OwnPtr<WebCore::DocLoader>::clear() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x4634342: WebCore::Document::~Document() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x46327E1: WebCore::Document::removedLastRef() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x41F92B0: WebCore::TreeShared<WebCore::Node>::deref() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x4260A8E: void WTF::derefIfNotNull<WebCore::Document>(WebCore::Document*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0) 
==6355== by 0x462CF20: WTF::RefPtr<WebCore::Document>::~RefPtr() (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0)
==6355== by 0x483EFEC: WebCore::Loader::Host::didFail(WebCore::SubresourceLoader*, bool) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0)
==6355== by 0x483F8B2: WebCore::Loader::Host::cancelRequests(WebCore::DocLoader*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0)
==6355== by 0x483DE85: WebCore::Loader::cancelRequests(WebCore::DocLoader*) (in /opt/qtsdk-4.70-beta1/lib/libQtWebKit.so.4.7.0)
Comment 1 Jan Erik Hanssen 2010-10-29 17:08:58 PDT
Created attachment 72416 [details]
Patch
Comment 2 Jan Erik Hanssen 2010-10-29 17:09:35 PDT
What seems to happen here is that when DocumentWriter::begin() is called while a load is in progress, setDocument(0) will be called which in turn causes the current document to be destroyed. However, the Document destructor destroys its CachedResourceLoader instance which will cancel all pending loads, but Loader::Host::didFail() tries to ref the Document that's being destroyed. This is what seems to cause the crash.

The proposed patch solves this by cancelling all requests before Document is being destroyed.
Comment 3 Jan Erik Hanssen 2010-10-29 17:12:05 PDT
Note that the patch does not currently have a test case, I'm not entirely sure how to write one that tests this behavior (i.e. starting a page load and then afterwards making sure that DocumentWriter::begin() is called before receiving any data.
Comment 4 xxx 2010-10-29 23:18:03 PDT

(In reply to comment #3)
> Note that the patch does not currently have a test case, I'm not entirely sure 

Jan Erik,
thank you for the patch.
I attached a test case at https://bugs.webkit.org/attachment.cgi?id=57005 that reproduces the problem. Does it not crash now with the patch?
Comment 5 Jan Erik Hanssen 2010-10-30 06:36:52 PDT
(In reply to comment #4)
> thank you for the patch.
> I attached a test case at https://bugs.webkit.org/attachment.cgi?id=57005 that reproduces the problem. Does it not crash now with the patch?

Your test case no longer crashes with the patch applied.

However, that test case can unfortunately not be used for inclusion with the patch since it's using Qt.
Comment 6 Adam Barth 2010-10-31 18:01:34 PDT
Comment on attachment 72416 [details]
Patch

We need a test.
Comment 7 Jan Erik Hanssen 2010-11-05 19:08:28 PDT
Created attachment 73158 [details]
Patch
Comment 8 Jan Erik Hanssen 2010-11-05 19:09:29 PDT
(In reply to comment #7)
> Created an attachment (id=73158) [details]
> Patch

Note that this patch only implements the test case for Qt.
Comment 9 Robert Hogan 2010-11-30 14:14:31 PST
I can't reproduce this crash against Qt 4.6 and ToT WebKit or against qt-git and ToT WebKit. There's no longer a WebCore::CachedResource::setDocLoader in WebCore. Maybe it has been fixed by a drive-by?
Comment 10 Benjamin Poulain 2010-12-01 04:35:30 PST
(In reply to comment #9)
> I can't reproduce this crash against Qt 4.6 and ToT WebKit or against qt-git and ToT WebKit. There's no longer a WebCore::CachedResource::setDocLoader in WebCore. Maybe it has been fixed by a drive-by?

I keep the bug open. If it is fixed in trunk, we still need to cherry-pick the patch to QtWebkit 2.0 and 2.1.
Comment 11 Eric Seidel (no email) 2010-12-03 10:59:26 PST
Comment on attachment 73158 [details]
Patch

I'm not sure why this bug is still open for WebKit's bug database.  Certainy this doesnt' need to have r? set on it given the above commetns.
Comment 12 Jan Erik Hanssen 2010-12-05 05:39:44 PST
(In reply to comment #9)
> I can't reproduce this crash against Qt 4.6 and ToT WebKit or against qt-git and ToT WebKit. There's no longer a WebCore::CachedResource::setDocLoader in WebCore. Maybe it has been fixed by a drive-by?

Just had time to look at this again and still seeing the problem as of r73340 with Qt 4.7.1.
My patch still solves the problem.

Current valgrind output:

==31218== Invalid free() / delete / delete[]
==31218==    at 0x4025504: operator delete(void*) (vg_replace_malloc.c:387)
==31218==    by 0x46A630D: WebCore::Document::~Document() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x47C5F41: WebCore::HTMLDocument::~HTMLDocument() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x46A1D96: WebCore::Document::removedLastRef() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x492B8DB: WebCore::Frame::setDocument(WTF::PassRefPtr<WebCore::Document>) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x48BDCA8: WebCore::FrameLoader::clear(bool, bool, bool) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x48B7585: WebCore::DocumentWriter::begin(WebCore::KURL const&, bool, WebCore::SecurityOrigin*) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x4B3816A: QWebFrame::setUrl(QUrl const&) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x4B4EF03: QWebView::setUrl(QUrl const&) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x804AD2D: pal::MainWindow::setUrl() (in /home/jhanssen/dev/webkit-bugs/39670/webKitSetUrlCrash)
==31218==    by 0x804AE2F: pal::MainWindow::event(QEvent*) (in /home/jhanssen/dev/webkit-bugs/39670/webKitSetUrlCrash)
==31218==    by 0x54FCF4B: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /home/jhanssen/dev/qt-everywhere-opensource-src-4.7.1/lib/libQtGui.so.4.7.1)


==31218==  Address 0x9021268 is 0 bytes inside a block of size 776 free'd
==31218==    at 0x4025504: operator delete(void*) (vg_replace_malloc.c:387)
==31218==    by 0x46AC5DD: WebCore::Document::~Document() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x46A1D96: WebCore::Document::removedLastRef() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x48D4F08: WebCore::Loader::didFail(WebCore::SubresourceLoader*, bool) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x48D50AF: WebCore::Loader::cancelRequests(WebCore::CachedResourceLoader*) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x48AC54E: WebCore::CachedResourceLoader::~CachedResourceLoader() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x46A5DAE: WebCore::Document::~Document() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x47C5F41: WebCore::HTMLDocument::~HTMLDocument() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x46A1D96: WebCore::Document::removedLastRef() (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x492B8DB: WebCore::Frame::setDocument(WTF::PassRefPtr<WebCore::Document>) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x48BDCA8: WebCore::FrameLoader::clear(bool, bool, bool) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
==31218==    by 0x48B7585: WebCore::DocumentWriter::begin(WebCore::KURL const&, bool, WebCore::SecurityOrigin*) (in /home/jhanssen/dev/WebKit/WebKitBuild-qt/Release/lib/libQtWebKit.so.4.9.0)
Comment 13 Benjamin Poulain 2010-12-06 08:10:52 PST
Ademar, could you please solve this bug?

Crashes are P1, this should be fixed for the next patch release of Qt 4.7.

If this crash is in 2.0, could you please update the 2.0 branch, either by finding and backporting the fix of trunk, or by reviewing the patch of Jan Erik?
Comment 14 Ademar Reis 2010-12-06 13:59:41 PST
(In reply to comment #12)
> (In reply to comment #9)
> > I can't reproduce this crash against Qt 4.6 and ToT WebKit or against qt-git and ToT WebKit. There's no longer a WebCore::CachedResource::setDocLoader in WebCore. Maybe it has been fixed by a drive-by?
> 
> Just had time to look at this again and still seeing the problem as of r73340 with Qt 4.7.1.

I succeeded reproducing it with qtwebkit-2.0 (the version released with qt-4.7) and with trunk (r73392), but not on the qtwebkit-2.1 branch... I'll investigate more later... maybe it's a race... any ideas?

Could you prepare a patch for trunk inclusion? Backporting your current patch for 2.0 was simple, but the right thing to do is to fix this on trunk (with proper review) and then cherry-pick/backport it to stable branches.
Comment 15 Jan Erik Hanssen 2010-12-06 15:55:56 PST
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #9)
> > > I can't reproduce this crash against Qt 4.6 and ToT WebKit or against qt-git and ToT WebKit. There's no longer a WebCore::CachedResource::setDocLoader in WebCore. Maybe it has been fixed by a drive-by?
> > 
> > Just had time to look at this again and still seeing the problem as of r73340 with Qt 4.7.1.
> 
> I succeeded reproducing it with qtwebkit-2.0 (the version released with qt-4.7) and with trunk (r73392), but not on the qtwebkit-2.1 branch... I'll investigate more later... maybe it's a race... any ideas?
> 
> Could you prepare a patch for trunk inclusion? Backporting your current patch for 2.0 was simple, but the right thing to do is to fix this on trunk (with proper review) and then cherry-pick/backport it to stable branches.

There is a race, yes. If the subresource (the css file) has been processed by the loader before the second setUrl() is called then the problem will not occur. My test case attempts to avoid this by loading the resource from a php script that calls sleep() to ensure that the subresource is still being loaded at this point.

I'll upload a new patch for trunk inclusion.
Comment 16 Jan Erik Hanssen 2010-12-07 18:20:56 PST
Created attachment 75858 [details]
Patch

Seems better to clear the pending loads when DocumentWriter::begin() is called (from QWebFrame), this is the existing behavior in e.g. DocumentWriter::replaceDocument(). As part of this the test case has also been ported to Qt.
Comment 17 WebKit Review Bot 2010-12-07 21:58:19 PST
Attachment 75858 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Eric Seidel (no email) 2010-12-10 00:09:53 PST
I'm not sure this is right.  @abarth?
Comment 19 Adam Barth 2010-12-23 17:40:15 PST
Comment on attachment 75858 [details]
Patch

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

> WebKit/qt/Api/qwebframe.cpp:709
> +    if (d->frame->loader()->isLoading())
> +        d->frame->loader()->stopAllLoaders();
>      d->frame->loader()->writer()->begin(absolute);
>      d->frame->loader()->writer()->end();

These methods are all way too low-level to be called from WebKit.  You should call some API on FrameLoader that does what you want.  This code might well be a security vulnerability too.  I'd have to look into the details.  The issue is you're spamming "absolute" over the document, which will give whoever has a pointer to the document access the cookies for "absolute".
Comment 20 Jan Erik Hanssen 2010-12-23 18:07:42 PST
(In reply to comment #19)
> (From update of attachment 75858 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75858&action=review
> 
> > WebKit/qt/Api/qwebframe.cpp:709
> > +    if (d->frame->loader()->isLoading())
> > +        d->frame->loader()->stopAllLoaders();
> >      d->frame->loader()->writer()->begin(absolute);
> >      d->frame->loader()->writer()->end();
> 
> These methods are all way too low-level to be called from WebKit.  You should call some API on FrameLoader that does what you want.  This code might well be a security vulnerability too.  I'd have to look into the details.  The issue is you're spamming "absolute" over the document, which will give whoever has a pointer to the document access the cookies for "absolute".

That could very well be (I'm not too familiar with this code yet), though the call to DocumentWriter::begin() predates this proposed bug fix, it seems to have been around for quite some time.
Comment 21 Adam Barth 2010-12-23 18:12:02 PST
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 75858 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=75858&action=review
> > 
> > > WebKit/qt/Api/qwebframe.cpp:709
> > > +    if (d->frame->loader()->isLoading())
> > > +        d->frame->loader()->stopAllLoaders();
> > >      d->frame->loader()->writer()->begin(absolute);
> > >      d->frame->loader()->writer()->end();
> > 
> > These methods are all way too low-level to be called from WebKit.  You should call some API on FrameLoader that does what you want.  This code might well be a security vulnerability too.  I'd have to look into the details.  The issue is you're spamming "absolute" over the document, which will give whoever has a pointer to the document access the cookies for "absolute".
> 
> That could very well be (I'm not too familiar with this code yet), though the call to DocumentWriter::begin() predates this proposed bug fix, it seems to have been around for quite some time.

Yeah, this has probably been wrong for a while.  The right fix is likely to remove the wrong code and call through to the appropriate high-level API for loading a URL.  One option is something like scheduleLocationChange.  That should do all the low-level state tweaking for you.
Comment 22 Andreas Kling 2011-02-16 17:02:16 PST

*** This bug has been marked as a duplicate of bug 49216 ***