Bug 37323 – [Qt] tst_QWebHistoryInterface::visitedLinks() fails

• Component: Tools / Tests
• Status: CLOSED FIXED

[Simon Hausmann]

FAIL!  : tst_QWebHistoryInterface::visitedLinks() Compared values are not the same
   Actual (m_page->mainFrame()->evaluateJavaScript("document.querySelectorAll(':visited').length;").toString()): 0
   Expected (QString::fromLatin1("1")): 1
   Loc: [/home/shausman/src/webkit/trunk/WebKit/qt/tests/qwebhistoryinterface/tst_qwebhistoryinterface.cpp(90)]

Reproducible on Linux/X11.

[Jakub Wieczorek]

(In reply to comment #0)
> FAIL!  : tst_QWebHistoryInterface::visitedLinks() Compared values are not the
> same
>    Actual
> (m_page->mainFrame()->evaluateJavaScript("document.querySelectorAll(':visited').length;").toString()):
> 0
>    Expected (QString::fromLatin1("1")): 1
>    Loc:
> [/home/shausman/src/webkit/trunk/WebKit/qt/tests/qwebhistoryinterface/tst_qwebhistoryinterface.cpp(90)]
> 
> Reproducible on Linux/X11.

That would be the new :visited patch that has just landed: https://bugs.webkit.org/show_bug.cgi?id=24300.

[Yi Shen]

Created attachment 54060 [details]
proposed patch

Fix the tst_QWebHistoryInterface::visitedLinks by using DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo to get the correct info.
I have run the test on my local many many times and I am sure DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo won't cause any crash.
Please take a look at the comments of https://bugs.webkit.org/show_bug.cgi?id=37759. Thanks a lot!

[Simon Hausmann]

Comment on attachment 54060 [details]
proposed patch

Thanks a lot Yi! :-)

[WebKit Commit Bot]

Comment on attachment 54060 [details]
proposed patch

Rejecting patch 54060 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--force']" exit_code: 1
Last 500 characters of output:
g file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp
Hunk #2 succeeded at 416 with fuzz 2 (offset 45 lines).
patching file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h
Hunk #1 FAILED at 24.
Hunk #2 succeeded at 86 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h.rej
patching file WebKit/qt/tests/qwebhistoryinterface/tst_qwebhistoryinterface.cpp

Full output: http://webkit-commit-queue.appspot.com/results/1905083

[Yi Shen]

Is it because my patch is out of date? I will create a new one then.

(In reply to comment #4)
> (From update of attachment 54060 [details])
> Rejecting patch 54060 from commit-queue.
> 
> Failed to run
> "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> u'--reviewer', u'Simon Hausmann', u'--force']" exit_code: 1
> Last 500 characters of output:
> g file WebKit/qt/ChangeLog
> Hunk #1 succeeded at 1 with fuzz 3.
> patching file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp
> Hunk #2 succeeded at 416 with fuzz 2 (offset 45 lines).
> patching file WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h
> Hunk #1 FAILED at 24.
> Hunk #2 succeeded at 86 (offset 3 lines).
> 1 out of 2 hunks FAILED -- saving rejects to file
> WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h.rej
> patching file WebKit/qt/tests/qwebhistoryinterface/tst_qwebhistoryinterface.cpp
> 
> Full output: http://webkit-commit-queue.appspot.com/results/1905083

[Antonio Gomes]

hum, patch works, but doing this include from WebKit/qt/tests looks a bit strange to me =(

+#include "../WebCoreSupport/DumpRenderTreeSupportQt.h"
 #include <QtTest/QtTest>
 
+    QString linkColor = DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo(m_page->mainFrame(), "vlink")["color"].toString();


and this looks a bit intrusive ...

simon, any idea to make it work differently? if no, just ignore me

[Simon Hausmann]

(In reply to comment #6)
> hum, patch works, but doing this include from WebKit/qt/tests looks a bit
> strange to me =(
> 
> +#include "../WebCoreSupport/DumpRenderTreeSupportQt.h"
>  #include <QtTest/QtTest>
> 
> +    QString linkColor =
> DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo(m_page->mainFrame(),
> "vlink")["color"].toString();
> 
> 
> and this looks a bit intrusive ...
> 
> simon, any idea to make it work differently? if no, just ignore me

I don't see much of a choice at this point :( It's late in the game to add new APIs, so the DRT function for unit testing is fine with me.

[Antonio Gomes]

(In reply to comment #7)
> (In reply to comment #6)
> > hum, patch works, but doing this include from WebKit/qt/tests looks a bit
> > strange to me =(
> > 
> > +#include "../WebCoreSupport/DumpRenderTreeSupportQt.h"
> >  #include <QtTest/QtTest>
> > 
> > +    QString linkColor =
> > DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo(m_page->mainFrame(),
> > "vlink")["color"].toString();
> > 
> > 
> > and this looks a bit intrusive ...
> > 
> > simon, any idea to make it work differently? if no, just ignore me
> 
> I don't see much of a choice at this point :( It's late in the game to add new
> APIs, so the DRT function for unit testing is fine with me.

Simon, I think we can use

QWebElement::styleProperty(const QString &name, StyleResolveStrategy strategy)

i will upload a patch that shows what I have in mind.

[Antonio Gomes]

Created attachment 54562 [details]
alternative approach

this patch makes QWebElement::styleProperty method instead of DumpRenderTreeQt additional method.

ps: Yi's authorship is kept in the ChangeLog

[Yi Shen]

Created attachment 54565 [details]
fix the svn-apply issue

Thanks Antonio. Your patch looks nice for me but I am not familiar with QWebElement::styleProperty() changes. I also updated my patch and attached it for review. Let Simon decide which patch should be used :)

The reasons why I prefer to add DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo to handle this test case are
1) using DRT function for unit testing can also be found in tst_qwebpage.cpp.
2) we may need an implementation of LayoutTestController::computedStyleIncludingVisitedInfo for Qt, which can invoke DumpRenderTreeSupportQt::computedStyleIncludingVisitedInfo.
see https://bugs.webkit.org/show_bug.cgi?id=37759. I was fighting against the test script which causes the new interface crashes.

[Simon Hausmann]

Comment on attachment 54562 [details]
alternative approach

You're right Antonio, the reason for the change in behaviour of computedStyle() was to protect the user from malicious javascript.

There's little point in providing much security through our C++ API, given that it gives full control anyway.

Therefore your patch is much simpler :)

[Simon Hausmann]

Comment on attachment 54565 [details]
fix the svn-apply issue

Let's go for Antonio's solution :)

[Antonio Gomes]

Comment on attachment 54562 [details]
alternative approach

Clearing flags on attachment: 54562

Committed r58427: <http://trac.webkit.org/changeset/58427>

[Antonio Gomes]

Thank you Simon Hausmann!

[Antonio Gomes]

(In reply to comment #13)
> (From update of attachment 54562 [details])
> Clearing flags on attachment: 54562
> 
> Committed r58427: <http://trac.webkit.org/changeset/58427>

Err, I had a typo in the commit message:

"Reviewed by NOBODY Simon Hausmann."

hope it does not hurt much. sorry :(

[Simon Hausmann]

(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 54562 [details] [details])
> > Clearing flags on attachment: 54562
> > 
> > Committed r58427: <http://trac.webkit.org/changeset/58427>
> 
> Err, I had a typo in the commit message:
> 
> "Reviewed by NOBODY Simon Hausmann."
> 
> hope it does not hurt much. sorry :(

Not a problem at all, I had a good laugh :)

[Simon Hausmann]

Revision r58427 cherry-picked into qtwebkit-2.0 with commit adc47622d204163a416403abd925d2eff1817f0b

[Simon Hausmann]

(In reply to comment #17)
> Revision r58427 cherry-picked into qtwebkit-2.0 with commit
> adc47622d204163a416403abd925d2eff1817f0b

Rolled out with commit a3a6be07b35ace4738aab1f3b1dc2caa755175e9:


   ../../../WebCore/css/CSSComputedStyleDeclaration.h: In member function â<U+0080><U+0098>QString QWebElement::styleProperty(const QString&, QWebElement::StyleResolveStrategy) constâ<U+0080><U+0099>:
    ../../../WebCore/css/CSSComputedStyleDeclaration.h:75: error: too many arguments to function â<U+0080><U+0098>WTF::PassRefPtr WebCore::computedStyle(WTF::PassRefPtr)â<U+0080><U+0099>
    ../../../WebKit/qt/Api/qwebelement.cpp:824: error: at this point in file



The private links patch is not part of the branch. Not sure if we should actually pick it, it's quite intrusive...

[Antonio Gomes]

(In reply to comment #18)
> (In reply to comment #17)
> > Revision r58427 cherry-picked into qtwebkit-2.0 with commit
> > adc47622d204163a416403abd925d2eff1817f0b
> 
> Rolled out with commit a3a6be07b35ace4738aab1f3b1dc2caa755175e9:
> 
> 
>    ../../../WebCore/css/CSSComputedStyleDeclaration.h: In member function
> â<U+0080><U+0098>QString QWebElement::styleProperty(const QString&,
> QWebElement::StyleResolveStrategy) constâ<U+0080><U+0099>:
>     ../../../WebCore/css/CSSComputedStyleDeclaration.h:75: error: too many
> arguments to function â<U+0080><U+0098>WTF::PassRefPtr
> WebCore::computedStyle(WTF::PassRefPtr)â<U+0080><U+0099>
>     ../../../WebKit/qt/Api/qwebelement.cpp:824: error: at this point in file
> 
> 
> 
> The private links patch is not part of the branch. Not sure if we should
> actually pick it, it's quite intrusive...

Ahh! :( should we go with the other approach for the branch? I really did not realize it could happen ...

[Antonio Gomes]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Revision r58427 cherry-picked into qtwebkit-2.0 with commit
> > > adc47622d204163a416403abd925d2eff1817f0b
> > 
> > Rolled out with commit a3a6be07b35ace4738aab1f3b1dc2caa755175e9:
> > 
> > 
> >    ../../../WebCore/css/CSSComputedStyleDeclaration.h: In member function
> > â<U+0080><U+0098>QString QWebElement::styleProperty(const QString&,
> > QWebElement::StyleResolveStrategy) constâ<U+0080><U+0099>:
> >     ../../../WebCore/css/CSSComputedStyleDeclaration.h:75: error: too many
> > arguments to function â<U+0080><U+0098>WTF::PassRefPtr
> > WebCore::computedStyle(WTF::PassRefPtr)â<U+0080><U+0099>
> >     ../../../WebKit/qt/Api/qwebelement.cpp:824: error: at this point in file
> > 

> Ahh! :( should we go with the other approach for the branch? I really did not
> realize it could happen ...

ahh (again) the "other" approach also makes use of this the new parameter introduced by hyatt :(

> > The private links patch is not part of the branch. Not sure if we should
> > actually pick it, it's quite intrusive...

humm, it is considered a security fix. Are you considering to cherry pick it at all, simon, or it is out of the question?

[Antonio Gomes]

simon, ideas on what can be done here?

[Simon Hausmann]

(In reply to comment #21)
> simon, ideas on what can be done here?

I've looked into cherry-picking the private links feature, but it turns out to be a big effort. There have been a _lot_ of follow-up patches after the initial landing, mostly performance related.

I don't feel comfortable to cherry-pick a huge series of patches and risk missing one, to be honest. If one of you wants to do it, I'd be happy to review the picks.

My feeling is that it may be simpler to take this into the next 2.1 release. What do you think?

[Antonio Gomes]

(In reply to comment #22)
> (In reply to comment #21)
> > simon, ideas on what can be done here?
> 
> I've looked into cherry-picking the private links feature, but it turns out to
> be a big effort. There have been a _lot_ of follow-up patches after the initial
> landing, mostly performance related.
> 
> I don't feel comfortable to cherry-pick a huge series of patches and risk
> missing one, to be honest. If one of you wants to do it, I'd be happy to review
> the picks.
> 
> My feeling is that it may be simpler to take this into the next 2.1 release.
> What do you think?

I see. My concern is what to do with this test in the 2.0 branch? Maybe "skip" it, like a known test failure?

on trunk it is ok and will be in 2.1.

[Antonio Gomes]

It seems that tronical reported this bug because auto tests was failing on trunk, not on the qtwebkit-2.0 release branch. Hence, since the release branch does not have hyatt's "visited links fixes' series of patches either, this autotest does not fail on release branch, and this fix does not need to be cherry-picked.

I just build qtwebkit-2.0 b4aa5e1ddc41edab895132aba3cc66d9d7129444 , and auto test passes fine.

[Simon Hausmann]

Marking as closed, this is not meant for inclusion in the release branch.