Bug 93195

Most likely this needs something like http://trac.webkit.org/changeset/116296

Created attachment 156885 [details] Patch

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

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

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

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

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.

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

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

> 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

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

I would feel more comfortable if we changed the behavior of Apple Mac DRT, WTR and Chromium in a single patch.

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

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.

Created attachment 157096 [details] Patch Hey ObjC/WebKit2, I just met you / and this is crazy / but build my code, maybe?

(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 on attachment 157096 [details] Patch Attachment 157096 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13450703

Comment on attachment 157096 [details] Patch Attachment 157096 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13457302

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

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 on attachment 157096 [details] Patch Attachment 157096 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13446964

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

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

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

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

Since we're talking about other ports now, I'll just bring this up on webkit-dev.

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

It seems that this has already been done for GTK's DumpRenderTree in bug 96543 and in bug 90856 for WebKit2 GTK+.

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.

Created attachment 171115 [details] Patch

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

Created attachment 171298 [details] Patch

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 on attachment 171298 [details] Patch Clearing flags on attachment: 171298 Committed r133069: <http://trac.webkit.org/changeset/133069>