Bug 76699 - [WTR] Visited link tracking is not disabled properly
Summary: [WTR] Visited link tracking is not disabled properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andras Becsi
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 76619 79666
  Show dependency treegraph
 
Reported: 2012-01-20 04:30 PST by Csaba Osztrogonác
Modified: 2012-05-16 02:58 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (7.20 KB, patch)
2012-05-15 11:33 PDT, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-01-20 04:30:10 PST
http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r105504%20%2818894%29/results.html

fast/block/margin-collapse/056.html:
-        RenderInline {A} at (0,0) size 56x21 [color=#0000EE]
+        RenderInline {A} at (0,0) size 56x21 [color=#551A8B]

tables tests:
-          RenderInline {A} at (0,0) size 255x17 [color=#FFFF00]
+          RenderInline {A} at (0,0) size 255x17 [color=#999900]
Comment 1 Csaba Osztrogonác 2012-01-20 05:50:20 PST
And additionally it made 30 tests flakey ... :-/
Comment 2 Csaba Osztrogonác 2012-01-20 05:56:39 PST
Shouldn't we roll out r105461?
Comment 3 Alexey Proskuryakov 2012-01-20 09:08:16 PST
Changed results are quite likely to be improvements - did you find any actual regressions?

Are you sure that this change made tests flaky? It seems quite unlikely.
Comment 4 Csaba Osztrogonác 2012-01-20 09:14:14 PST
(In reply to comment #3)
> Changed results are quite likely to be improvements - did you find any actual regressions?
Without this patch WK1 and WK2 results for these tests were same,
but with this patch now the results are diverging. 
 
> Are you sure that this change made tests flaky? It seems quite unlikely.
I'm absolutely sure. Check the following two test:
r105460 - 1 failures 7 flakes 28 missing results - http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/18859
r105461 - 9 failures 36 flakes 28 missing results - http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/18860
Comment 5 Alexey Proskuryakov 2012-01-20 10:39:57 PST
> fast/block/margin-collapse/056.html

This test, and all tables/mozilla tests pass on Mac WebKit2.

r105461 was a fix for an obvious bug. While it could have caused changes in test results, I don't see how it could introduce failures or flakiness. Perhaps there is some Qt specific code that used to neuter this bug, and now misbehaves?

I'd like to help, but at this point, I think that some investigation from Qt side is needed.
Comment 6 Rafael Brandao 2012-01-24 09:00:05 PST
(In reply to comment #5)
> > fast/block/margin-collapse/056.html
> 
> This test, and all tables/mozilla tests pass on Mac WebKit2.
> 
> r105461 was a fix for an obvious bug. While it could have caused changes in test results, I don't see how it could introduce failures or flakiness. Perhaps there is some Qt specific code that used to neuter this bug, and now misbehaves?
> 
> I'd like to help, but at this point, I think that some investigation from Qt side is needed.

It seems very unlikely that those changes are behind this. I will take a look and try to figure out why this is happening, but when I ran just fast/block/margin-collapse/056.html I've got more diff than that pasted here (some widths were different).
Comment 7 Rafael Brandao 2012-01-24 14:56:02 PST
It's a bit hard to realize what could be wrong here, I'm getting the same diff for head at r105461 and the previous one: https://gist.github.com/3d7570e546cb05e5be68 (the same results I get from current head). Ossy, do you have any idea how a test that only uses colors red, aqua and yellow could have produced the color #551A8B (purple)? Maybe it is blending colors? It would be nice if there could be a way for us to setup an environment that would produce the same results as our buildbot or if we could access it to run tests remotely. I'm using x86-64, maybe this can only be reproduced on 32... Any ideas?
Comment 8 Balazs Kelemen 2012-01-29 14:58:03 PST
(In reply to comment #7)
> It's a bit hard to realize what could be wrong here, I'm getting the same diff for head at r105461 and the previous one: https://gist.github.com/3d7570e546cb05e5be68 (the same results I get from current head). Ossy, do you have any idea how a test that only uses colors red, aqua and yellow could have produced the color #551A8B (purple)? Maybe it is blending colors? It would be nice if there could be a way for us to setup an environment that would produce the same results as our buildbot or if we could access it to run tests remotely. I'm using x86-64, maybe this can only be reproduced on 32... Any ideas?

You could use QBAT (http://webkit.sed.hu/blog/20101028/qtwebkit-builder-and-tester-virtual-machine) or copy the amazon ec2 vm (azbest_hu can help you with that). If you use a distro other than Debian or Ubuntu that can also be a reason for mismatches, so you could set up an Ubuntu chroot that should produce the same results.
Comment 9 Csaba Osztrogonác 2012-03-27 02:35:19 PDT
I skipped tables/mozilla/marvin/backgr_simple-table-cell.html because it fails always on Qt-WK2 because of this bug - http://trac.webkit.org/changeset/112232.
Comment 10 Zan Dobersek 2012-04-28 04:37:54 PDT
These failures occur simply because of visited links being colored as visited - #551A8B in fast/block/margin-collapse/056.html and #999900 in tables tests.

Some tests then turn out as flaky as the links haven't been visited in the rerun due to a more narrow test list while others fail again because of specific tests being tested before again.

To stop this problem from occurring, the back-forward list should be cleared after each test. A similar fix was required for the Gtk WebKit1 port at bug #71052.
Comment 11 Csaba Osztrogonác 2012-05-14 03:54:33 PDT
Szilárd started working on fix suggested in Comment #10.
Comment 12 Andras Becsi 2012-05-15 07:23:06 PDT
Taking over as discussed with Ossy.

The issue seems a bit more complex than described in Comment #10.

Looks like the m_visitedLinkTable which is shared between the WebContext and WebProcess does not get cleaned properly / in time before style recalc, so links to previous tests appear in visited color.

I'll try to find a solution and upload a patch.
Comment 13 Andras Becsi 2012-05-15 11:33:01 PDT
Created attachment 142012 [details]
proposed patch
Comment 14 Andras Becsi 2012-05-15 11:46:42 PDT
(In reply to comment #13)
> Created an attachment (id=142012) [details]
> proposed patch

Looks like before r105461 local files were not tracked in visited links properly because of the missing file:// prefix. This concealed the issue that WebKitTestRunner always tracked visited links, thus tests which have local links to previous tests began to fail, producing links with visited color.

Not sure why this does not show up on Mac, though. WTR on Mac might not use PLATFORM_STRATEGIES.
Comment 15 Jocelyn Turcotte 2012-05-15 12:20:37 PDT
Comment on attachment 142012 [details]
proposed patch

Looks good to me from what I've checked.
It would be nice to know how this end up being disabled on the Mac port, assuming that it doesn't use PageGroup::is/addVisitedLink either.
Comment 16 Andras Becsi 2012-05-16 02:58:07 PDT
Comment on attachment 142012 [details]
proposed patch

Clearing flags on attachment: 142012

Committed r117248: <http://trac.webkit.org/changeset/117248>
Comment 17 Andras Becsi 2012-05-16 02:58:16 PDT
All reviewed patches have been landed.  Closing bug.