Bug 93195

Summary: [chromium] DRT and WTR should clear the cache between tests
Product: WebKit Reporter: Ami Fischman <fischman>
Component: Tools / TestsAssignee: Ami Fischman <fischman>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, brian.holt, dglazkov, eric.carlson, feature-media-reviews, fischman, gns, ojan, philn, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from gce-cr-linux-03
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from gce-cr-linux-05
none
Patch
tony: review+, webkit.review.bot: commit-queue-
Patch none

Description Ami Fischman 2012-08-04 23:20:36 PDT
While investigating bug 92824 I ran across the fact that if one layouttest loads a resource and then another test that runs in the same process requests that resource after calling testRunner.setWillSendRequestReturnsNull(true) then the second test will still see information about the resource, presumably from some sort of cache (in-memory? on-disk? IDK) that DRT is maintaining between tests.  This breaks test hermeticness and should be fixed.
Comment 1 Ami Fischman 2012-08-04 23:31:01 PDT
Most likely this needs something like http://trac.webkit.org/changeset/116296
Comment 2 Ami Fischman 2012-08-07 00:26:44 PDT
Created attachment 156885 [details]
Patch
Comment 3 Ami Fischman 2012-08-07 00:29:54 PDT
Comment on attachment 156885 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestShell.cpp:37
>  #include "MockWebPrerenderingSupport.h"

I couldn't figure out where to place the new 
#include "WebCache.h"
to appease webkit-patch's notion of sorting, so M-x sort-lines RET'd the whole shebang.

> LayoutTests/media/video-poster-blocked-by-willsendrequest.html:40
> +                // FIXME: the getTime() below works around a bug in DRT where caches aren't cleared between tests; mac: 82976, gtk: 79760.

Tested the patch by:
s/?" + (new Date().getTime())/"/
and then running:
rm -rf /tmp/x2 && time ./Tools/Scripts/new-run-webkit-tests --child-processes=1 --debug --no-retry --iterations=2 --results-directory=/tmp/x2  media/video-poster-blocked-by-willsendrequest.html media/video-poster.html
to observe the failure.  Then added the WebCache::clear() above and re-ran to observe success.
Comment 4 Tony Chang 2012-08-07 00:31:53 PDT
Comment on attachment 156885 [details]
Patch

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

Why does the resource being cached cause the test to fail?  Is this a bug in the test?

> LayoutTests/media/video-poster-blocked-by-willsendrequest.html:41
> -                // FIXME: the getTime() below works around a bug in DRT where caches aren't cleared between tests; chromium: 93195, mac: 82976, gtk: 79760.
> +                // FIXME: the getTime() below works around a bug in DRT where caches aren't cleared between tests; mac: 82976, gtk: 79760.
>                  video.poster = "content/abe.png?" + (new Date().getTime());

Why doesn't adding a random parameter to the test work?
Comment 5 Ami Fischman 2012-08-07 00:36:53 PDT
(In reply to comment #4)
> (From update of attachment 156885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156885&action=review
> 
> Why does the resource being cached cause the test to fail?  Is this a bug in the test?

No; the test is testing testRunner.setWillSendRequestReturnsNull().  But blocking the request doesn't help when the resource can be loaded from cache.

> > LayoutTests/media/video-poster-blocked-by-willsendrequest.html:41
> > -                // FIXME: the getTime() below works around a bug in DRT where caches aren't cleared between tests; chromium: 93195, mac: 82976, gtk: 79760.
> > +                // FIXME: the getTime() below works around a bug in DRT where caches aren't cleared between tests; mac: 82976, gtk: 79760.
> >                  video.poster = "content/abe.png?" + (new Date().getTime());
> 
> Why doesn't adding a random parameter to the test work?

That's what I originally did in 92824, but rniwa@ requested a time-based approach in https://bugs.webkit.org/show_bug.cgi?id=92824#c15.
I don't know how to evaluate either of:
P((""+Math.random()) returning equal values in two test runs)
P(two tests running within the same millisecond of system clock (possibly after clock has its time adjusted)
but I don't think either probability is big enough to register as more than noise in our layouttest systems...
Comment 6 Tony Chang 2012-08-07 00:40:45 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Why doesn't adding a random parameter to the test work?
> 
> That's what I originally did in 92824, but rniwa@ requested a time-based approach in https://bugs.webkit.org/show_bug.cgi?id=92824#c15.

Sorry, what I meant was how do we end up getting the resource from the cache if we added this parameter?
Comment 7 Ami Fischman 2012-08-07 00:42:45 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Why doesn't adding a random parameter to the test work?
> > 
> > That's what I originally did in 92824, but rniwa@ requested a time-based approach in https://bugs.webkit.org/show_bug.cgi?id=92824#c15.
> 
> Sorry, what I meant was how do we end up getting the resource from the cache if we added this parameter?

I very recently added this parameter to make the test pass consistently, and added the FIXME to remove it once all DRTs ran tests hermetically (at least in the sense of not sharing a cache).
Comment 8 Tony Chang 2012-08-07 00:48:28 PDT
I think this change is OK.

That said, it's not clear to me what the expected behavior of DRT is (whether it should clear the cache or not).  I suspect there are testing benefits to either behavior.

Historically, we try to match what Apple DRT/WTR does as the gold standard of "expected" behavior.  If clearing the cache is what all DRTs and WTR are supposed to do during tests, then it would be nice to update Apple DRT and WTR as well since it's the gold standard.
Comment 9 Ami Fischman 2012-08-07 00:52:17 PDT
(In reply to comment #8)
> I think this change is OK.
> 
> That said, it's not clear to me what the expected behavior of DRT is (whether it should clear the cache or not).  I suspect there are testing benefits to either behavior.
> 
> Historically, we try to match what Apple DRT/WTR does as the gold standard of "expected" behavior.  If clearing the cache is what all DRTs and WTR are supposed to do during tests, then it would be nice to update Apple DRT and WTR as well since it's the gold standard.

Not sure what WTR is, but fixing similar flakiness in the mac port of DRT is bug 82976.
Comment 10 WebKit Review Bot 2012-08-07 01:06:29 PDT
Comment on attachment 156885 [details]
Patch

Attachment 156885 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13444710

New failing tests:
svg/dynamic-updates/SVGUseElement-svgdom-href1-prop.html
svg/dynamic-updates/SVGUseElement-dom-href1-attr.html
platform/chromium/virtual/gpu/canvas/philip/tests/2d.text.draw.fontface.notinpage.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
Comment 11 WebKit Review Bot 2012-08-07 01:06:34 PDT
Created attachment 156891 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 12 Adam Barth 2012-08-07 11:27:53 PDT
> Not sure what WTR is, but fixing similar flakiness in the mac port of DRT is bug 82976.

WTR is WebKitTestRunner, which is more-or-less DumpRenderTree for WebKit2:

http://trac.webkit.org/browser/trunk/Tools/WebKitTestRunner
Comment 13 Ami Fischman 2012-08-07 19:19:12 PDT
(In reply to comment #12)
> > Not sure what WTR is, but fixing similar flakiness in the mac port of DRT is bug 82976.
> 
> WTR is WebKitTestRunner, which is more-or-less DumpRenderTree for WebKit2:
> 
> http://trac.webkit.org/browser/trunk/Tools/WebKitTestRunner

Thanks for filling in the blank.
Do you have an opinion about Tony's comment #8?  (or in general, about this patch?)
Comment 14 Tony Chang 2012-08-07 19:48:19 PDT
I would feel more comfortable if we changed the behavior of Apple Mac DRT, WTR and Chromium in a single patch.
Comment 15 Ami Fischman 2012-08-07 19:56:41 PDT
(In reply to comment #14)
> I would feel more comfortable if we changed the behavior of Apple Mac DRT, WTR and Chromium in a single patch.

I don't build on mac (or know ObjC).  If you point me at the relevant files for DRT/WRT I can hack blindly and let the CQ sort things out.
Comment 17 Ami Fischman 2012-08-07 20:32:13 PDT
Created attachment 157096 [details]
Patch

Hey ObjC/WebKit2, I just met you / and this is crazy / but build my code, maybe?
Comment 18 Ami Fischman 2012-08-07 20:47:37 PDT
(In reply to comment #16)
> http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm#L1235
> http://trac.webkit.org/browser/trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp#L360
> 
> All I did was grep for resetInternalsObject.


Yeah, umm, I realize that was a stupid offer I made.  I know nothing of how the WebKit2 code is laid out (i.e. what is its version of WebCache.h?) or how to find the answer to that question, and it doesn't seem worthwhile for me to develop the necessary expertise as I hack on webkit pretty infrequently.
Comment 19 Build Bot 2012-08-07 20:59:06 PDT
Comment on attachment 157096 [details]
Patch

Attachment 157096 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13450703
Comment 20 Early Warning System Bot 2012-08-07 21:11:18 PDT
Comment on attachment 157096 [details]
Patch

Attachment 157096 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13457302
Comment 21 WebKit Review Bot 2012-08-07 21:25:28 PDT
Comment on attachment 157096 [details]
Patch

Attachment 157096 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13460024

New failing tests:
svg/dynamic-updates/SVGUseElement-svgdom-href1-prop.html
svg/dynamic-updates/SVGUseElement-dom-href1-attr.html
platform/chromium/virtual/gpu/canvas/philip/tests/2d.text.draw.fontface.notinpage.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
Comment 22 WebKit Review Bot 2012-08-07 21:25:34 PDT
Created attachment 157102 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 23 Gyuyoung Kim 2012-08-07 22:01:20 PDT
Comment on attachment 157096 [details]
Patch

Attachment 157096 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13446964
Comment 24 Adam Barth 2012-08-07 22:44:52 PDT
> Do you have an opinion about Tony's comment #8?  (or in general, about this patch?)

I agree with Tony that we should keep all the test harnesses consistent on this point.  I don't have a particular opinion about whether clearing the cache is a win.
Comment 25 Ami Fischman 2012-08-08 08:40:31 PDT
(In reply to comment #24)
> > Do you have an opinion about Tony's comment #8?  (or in general, about this patch?)
> 
> I agree with Tony that we should keep all the test harnesses consistent on this point.  

It is not the case that without my patch all the harnesses are consistent.

> I don't have a particular opinion about whether clearing the cache is a win.

I'm surprised.  We're talking about test hermeticness.  I take it as an absolute axiom that a useful testing system must have a known state to start each test with to be useful.  If any state bleeds between tests it becomes impossible to say what it is that tests are testing.

Ojan: did you want to weigh in here?
Eric: I'm curious about your opinion on this issue.
Comment 26 Adam Barth 2012-08-08 10:03:13 PDT
> > I agree with Tony that we should keep all the test harnesses consistent on this point.  
> 
> It is not the case that without my patch all the harnesses are consistent.

Which ones clear the cache already?
Comment 27 Ami Fischman 2012-08-08 10:15:09 PDT
(In reply to comment #26)
> > > I agree with Tony that we should keep all the test harnesses consistent on this point.  
> > 
> > It is not the case that without my patch all the harnesses are consistent.
> 
> Which ones clear the cache already?

EFL; see bug 85609.
Comment 28 Ojan Vafai 2012-08-08 10:47:11 PDT
Since we're talking about other ports now, I'll just bring this up on webkit-dev.
Comment 29 Tony Chang 2012-08-08 14:18:09 PDT
(In reply to comment #27)
> (In reply to comment #26)
> > > > I agree with Tony that we should keep all the test harnesses consistent on this point.  
> > > 
> > > It is not the case that without my patch all the harnesses are consistent.
> > 
> > Which ones clear the cache already?
> 
> EFL; see bug 85609.

We should still try to keep all the test harnesses consistent.

It doesn't sound like there's any opposition to clearing the cache between each test (I didn't think there would be), someone just needs to do the work for all the ports.
Comment 30 Brian Holt 2012-10-26 06:56:40 PDT
It seems that this has already been done for GTK's DumpRenderTree in bug 96543 and in bug 90856 for WebKit2 GTK+.
Comment 31 Tony Chang 2012-10-26 14:05:29 PDT
FWIW, I think it's fine to land the Chromium change to clear the cache at this point. It doesn't seem like there's consensus on how DRT/WTR should behave here, which is quite unfortunate, but that seems to suggest that it doesn't matter what Chromium does.
Comment 32 Ami Fischman 2012-10-27 20:37:43 PDT
Created attachment 171115 [details]
Patch
Comment 33 WebKit Review Bot 2012-10-28 04:26:38 PDT
Comment on attachment 171115 [details]
Patch

Attachment 171115 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14607772

New failing tests:
svg/dynamic-updates/SVGUseElement-svgdom-href1-prop.html
svg/dynamic-updates/SVGUseElement-dom-href1-attr.html
Comment 34 Tony Chang 2012-10-29 13:20:01 PDT
Comment on attachment 171115 [details]
Patch

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

> Tools/ChangeLog:8
> +
> +

Please make a note that this matches GTK+, Qt and Efl.
Comment 35 Ami Fischman 2012-10-29 13:28:20 PDT
Created attachment 171298 [details]
Patch
Comment 36 Ami Fischman 2012-10-31 13:15:41 PDT
Comment on attachment 171298 [details]
Patch

Since:
- the webkit-dev thread (subject: "DRT/WTR should clear the cache at the beginning of each test?") has stopped making forward progress; and
- this change makes the chromium port compatible with the majority of ports (all but mac); and
- this change is confined to the chromium port; and
- none of the people objecting to the change work on the chromium port
I'm going to land this patch based on Tony's r+.
Comment 37 WebKit Review Bot 2012-10-31 13:35:25 PDT
Comment on attachment 171298 [details]
Patch

Clearing flags on attachment: 171298

Committed r133069: <http://trac.webkit.org/changeset/133069>