Bug 39670 – [Qt] WebView::setUrl crashes after Qt4.7

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[xxx]

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)

[Jan Erik Hanssen]

Created attachment 72416 [details]
Patch

[Jan Erik Hanssen]

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.

[Jan Erik Hanssen]

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.

[xxx]

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

[Jan Erik Hanssen]

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

[Adam Barth]

Comment on attachment 72416 [details]
Patch

We need a test.

[Jan Erik Hanssen]

Created attachment 73158 [details]
Patch

[Jan Erik Hanssen]

(In reply to comment #7)
> Created an attachment (id=73158) [details]
> Patch

Note that this patch only implements the test case for Qt.

[Robert Hogan]

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?

[Benjamin Poulain]

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

[Eric Seidel (no email)]

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.

[Jan Erik Hanssen]

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

[Benjamin Poulain]

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?

[Ademar Reis]

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

[Jan Erik Hanssen]

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

[Jan Erik Hanssen]

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.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

I'm not sure this is right.  @abarth?

[Adam Barth]

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

[Jan Erik Hanssen]

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

[Adam Barth]

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

[Andreas Kling]

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