QtWebKit asserts when selecting elided text.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Christian S]

In Qt 4.7.0 RC1 there is an assertion in FontQt.cpp when selecting elided text.
In Qt 4.6.x it does not assert, but selecting the elided word will cause an overlay where the text is elided.


Steps to reproduce:

open the html file "test.html" with the Qt demo browser, or compile and run the example code in the attached file.


Attached file:
qtwebkit_bug.tar.bz2

archive content:
- app.pro
- main.cpp
- test.html
- bt.txt (backtrace from the demo browser)

[Robert Hogan]

You forgot to attach something!

[Christian S]

Created attachment 73979 [details]
Test case

Sorry for the late reply, the file should be attached now.

[Jan Erik Hanssen]

Created attachment 74996 [details]
Patch

Make sure that the length passed to fromRawDataWithoutRef() does not exceed the length of the string. This is essentially what happens in FontFastPath.cpp since that one uses WidthIterator for getting the 'to' value.

[Kenneth Rohde Christiansen]

Comment on attachment 74996 [details]
Patch

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

> WebKit/qt/tests/qwebview/tst_qwebview.cpp:325
> +    QTest::qWait(1000);

Why this long wait?

[Jan Erik Hanssen]

(In reply to comment #4)
> (From update of attachment 74996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74996&action=review
> 
> > WebKit/qt/tests/qwebview/tst_qwebview.cpp:325
> > +    QTest::qWait(1000);
> 
> Why this long wait?

To make sure the next paint event is received. Is there a way in QTestLib to wait for a particular event perhaps?

[Jan Erik Hanssen]

(In reply to comment #5)
> (In reply to comment #4)
> > Why this long wait?
> 
> To make sure the next paint event is received. Is there a way in QTestLib to wait for a particular event perhaps?

Looks like a simple QApplication::processEvent() is sufficient to trigger the problem. I'll upload a new patch.

[Jan Erik Hanssen]

Created attachment 74997 [details]
Patch

[Simon Hausmann]

It is tricky to test this reliably, processEvents() may be enough on one machine, it may not on another one. On desktop machines it's fairly easy to get somewhat reliable results, but on slower machines with entirely different windowing systems it becomes difficult. (something we've observed when running on meego/maemo devices for example)

Would this assertion trigger anyway when running the tests on the bots? After all we're doing release builds there.

I'm not a big fan of adding these kind of tests, given that there's a very small chance of actually catching a regression with them. At the same time they cause headaches when running the tests on other platforms.

I like the qWait() approach over calling processEvents() FWIW.

[Simon Hausmann]

Comment on attachment 74997 [details]
Patch

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

> WebKit/qt/tests/qwebview/tst_qwebview.cpp:325
> +    QApplication::processEvents();

Hmm, in this particular case I wonder if this is going to work at all on platforms that are slightly more asynchronous, such as Mac OS X?

[Kenneth Rohde Christiansen]

Comment on attachment 74997 [details]
Patch

r- due to Simon's comments.

[Jan Erik Hanssen]

(In reply to comment #9)
> (From update of attachment 74997 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74997&action=review
> 
> > WebKit/qt/tests/qwebview/tst_qwebview.cpp:325
> > +    QApplication::processEvents();
> 
> Hmm, in this particular case I wonder if this is going to work at all on platforms that are slightly more asynchronous, such as Mac OS X?

This was tested on OS X actually.

I can go back to qWait() if that's preferrable, or even use a QEventLoop to verify that a QPaintEvent has been received by the view.

[Jan Erik Hanssen]

Created attachment 75002 [details]
Patch

Updated the test to explicitly wait (up to 5 seconds) for the next QPaintEvent after updating the selection

[Jan Erik Hanssen]

(In reply to comment #8)
> Would this assertion trigger anyway when running the tests on the bots? After all we're doing release builds there.

Not entirely sure, but the QCOMPARE() at the end would fail even if the test does not assert in release mode.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74997 [details]:

http/tests/security/xssAuditor/dom-write-innerHTML.html
fast/profiler/throw-exception-from-eval.html

Please file bugs against the tests.  These tests were authored by [e-mail], [e-mail], [e-mail], [e-mail], and [e-mail].  The commit-queue is continuing to process your patch.

[Jan Erik Hanssen]

(In reply to comment #13)
> (In reply to comment #8)
> > Would this assertion trigger anyway when running the tests on the bots? After all we're doing release builds there.
> 
> Not entirely sure, but the QCOMPARE() at the end would fail even if the test does not assert in release mode.

Actually, that's not true, the test will pass with the assertion not active.

What I'd want to verify is the selection rectangle, but I can't find any way of retrieving that.
Any suggestions?

[Jan Erik Hanssen]

Created attachment 75008 [details]
Patch

New patch without a test

[WebKit Commit Bot]

Comment on attachment 75008 [details]
Patch

Clearing flags on attachment: 75008

Committed r72788: <http://trac.webkit.org/changeset/72788>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dawit A.]

Shouldn't this be cherry picked into qtwebkit-2.1 ? There is a downstream report of a crash that seems to have been caused by this bug. See https://bugs.kde.org/show_bug.cgi?id=259272.

[Benjamin Poulain]

(In reply to comment #19)
> Shouldn't this be cherry picked into qtwebkit-2.1 ? There is a downstream report of a crash that seems to have been caused by this bug. See https://bugs.kde.org/show_bug.cgi?id=259272.

I would not cherry pick targeting 2.1 for an assert. It is late in the process, only important changes should go in.
Qt scripts to build webkit always build in release.

[Ademar Reis]

(In reply to comment #20)
> (In reply to comment #19)
> > Shouldn't this be cherry picked into qtwebkit-2.1 ? There is a downstream report of a crash that seems to have been caused by this bug. See https://bugs.kde.org/show_bug.cgi?id=259272.
> 
> I would not cherry pick targeting 2.1 for an assert. It is late in the process, only important changes should go in.
> Qt scripts to build webkit always build in release.

I believe a fix for a crash that happens by openging a web page is worth including in the release (or in a later minor update)... It can even be classified as a security vuln (at minimum it's a DoS).

The problem I have is that I can't reproduce the crash with the supplied example:

Inside Font::selectionRectForText(), codePath(run) returns Complex and thus Font::selectionRectForSimpleText() (where the problem is) never gets called.

Anybody familiar with the code could tell what should I do to actually call Font::selectionRectForSimpleText()?

[Benjamin Poulain]

(In reply to comment #21)
> I believe a fix for a crash that happens by openging a web page is worth including in the release (or in a later minor update)... It can even be classified as a security vuln (at minimum it's a DoS).

Yep, I was under the assumption it is only invalid selection and assertion in debug. If this can cause a crash in release, I agree this need to go in 2.0 and 2.1.

[Jan Erik Hanssen]

(In reply to comment #22)
> (In reply to comment #21)
> > I believe a fix for a crash that happens by openging a web page is worth including in the release (or in a later minor update)... It can even be classified as a security vuln (at minimum it's a DoS).
> 
> Yep, I was under the assumption it is only invalid selection and assertion in debug. If this can cause a crash in release, I agree this need to go in 2.0 and 2.1.

I didn't see any crashes when testing in release mode myself but using a QString returned by fromRawDataWithoutRef() could certainly read and write to memory out of bounds.

[Ademar Reis]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > I believe a fix for a crash that happens by openging a web page is worth including in the release (or in a later minor update)... It can even be classified as a security vuln (at minimum it's a DoS).
> > 
> > Yep, I was under the assumption it is only invalid selection and assertion in debug. If this can cause a crash in release, I agree this need to go in 2.0 and 2.1.
> 
> I didn't see any crashes when testing in release mode myself but using a QString returned by fromRawDataWithoutRef() could certainly read and write to memory out of bounds.

So it's a simple fix and has potential security implications. I'm adding it as a blocker for 2.1 and 2.0.

[Ademar Reis]

Revision r72788 cherry-picked into qtwebkit-2.1 with commit 6aef00a <http://gitorious.org/webkit/qtwebkit/commit/6aef00a>