Bug 92824

Summary: media/video-poster-blocked-by-willsendrequest.html isn't blocking poster on willsendrequest
Product: WebKit Reporter: Ami Fischman <fischman>
Component: Tools / TestsAssignee: Ami Fischman <fischman>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dpranke, eric.carlson, eric, feature-media-reviews, fischman, fishd, pkasting, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ami Fischman 2012-07-31 19:17:36 PDT
@rniwa: in http://trac.webkit.org/changeset/119194 you added this comment to this expectation:

// The result of jquery/traversing.html is bleeding into this test. 
BUGDPRANKE : media/video-poster-blocked-by-willsendrequest.html = TEXT PASS 

Do you remember what you saw that made you say that, or do you have any other state on this?

As an example, I don't see a failure on my linux desktop running:
./Tools/Scripts/new-run-webkit-tests --child-processes=1 --no-retry --debug jquery/traversing.html media/video-poster-blocked-by-willsendrequest.html
Comment 1 Ryosuke Niwa 2012-07-31 19:18:44 PDT
(In reply to comment #0)
> @rniwa: in http://trac.webkit.org/changeset/119194 you added this comment to this expectation:
> 
> // The result of jquery/traversing.html is bleeding into this test. 
> BUGDPRANKE : media/video-poster-blocked-by-willsendrequest.html = TEXT PASS 
> 
> Do you remember what you saw that made you say that, or do you have any other state on this?

I looked at the flakiness dashboard results.
Comment 2 Ami Fischman 2012-07-31 19:20:35 PDT
(In reply to comment #1)
> (In reply to comment #0)
> > @rniwa: in http://trac.webkit.org/changeset/119194 you added this comment to this expectation:
> > 
> > // The result of jquery/traversing.html is bleeding into this test. 
> > BUGDPRANKE : media/video-poster-blocked-by-willsendrequest.html = TEXT PASS 
> > 
> > Do you remember what you saw that made you say that, or do you have any other state on this?
> 
> I looked at the flakiness dashboard results.

I don't follow; is there something on 
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=media%2Fvideo-poster-blocked-by-willsendrequest.html%20
that mentions jquery/traversing.html ?
Comment 3 Ryosuke Niwa 2012-07-31 19:22:26 PDT
There used to be. Maybe this bug no longer reproduces.
Comment 4 Peter Kasting 2012-07-31 23:00:38 PDT
We also found this happening with a console.log from platform/chromium/fast/forms/color/color-suggestion-picker-appearance.html bleeding into platform/chromium/fast/loader/create-view-target-blank.html .

The latter test's output would get a spurious "CONSOLE MESSAGE: line 36: [object PagePopupController]" from the former test.
Comment 5 Peter Kasting 2012-07-31 23:14:25 PDT
Note: That same test also bleeds into platform/chromium/fast/text/international/complex-text-rectangle.html sometimes.
Comment 6 Ami Fischman 2012-08-01 15:13:51 PDT
Context: IRC conversation around 22:40-22:54 July 31 (pacific) included the wisdom that tests' console.log() & alert() sometimes bleed into subsequent tests, and that this has been the case for a very long time, and that nobody knows exactly why this happens, though a suspicion is that these APIs are too async for the way DRT works (test calls console.log() and immediately endTest(), but DRT/WebKit don't flush outstanding async work, which gets executed in the context of the next test).

Given that context, I don't think bleeding is still a problem here.  The test in question (media/video-poster-blocked-by-willsendrequest.html) is failing almost 100% on the bots, but looking like it indicates a real bug (poster isn't being blocked by willsendrequest).
Comment 7 Alexey Proskuryakov 2012-08-01 15:20:42 PDT
> the wisdom that tests' console.log() & alert() sometimes bleed into subsequent tests

Is that Chromium specific? We have a lot of tests that alert(), and I don't think that it happened with Mac WK1 or WK2 any time recently.
Comment 8 Ami Fischman 2012-08-01 15:47:17 PDT
(In reply to comment #7)
> > the wisdom that tests' console.log() & alert() sometimes bleed into subsequent tests
> 
> Is that Chromium specific?

I don't know, but my impression from IRC was that Eric Seidel was saying it was not port-specific.
Eric: anything you want to add to my summary above?
Comment 9 Eric Seidel (no email) 2012-08-01 15:58:03 PDT
I believe alert() and console.log() are synchronous in Mac WK/DRT.

I don't know why they wouldn't be for Chromium, but I don't know much about that part of the code.

These are the callbacks from WebCore:
http://trac.webkit.org/browser/trunk/Source/WebCore/page/ChromeClient.h#L140
http://trac.webkit.org/browser/trunk/Source/WebCore/page/ChromeClient.h#L133

We'd have to look at the chromium implementation of those to know if they're async.
Comment 10 Ami Fischman 2012-08-04 23:21:16 PDT
Figured out the problem with this test on the bots: if content/abe.png is fetched  by any other test before the test in question runs, then even though the test blocks the load (I verified that WebViewHost.cpp:blockRequest() is called even when the test fails) the media element finds the resource's dimensions and fails the test.

Quick repro of the problem:
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 

I'm going to land a patch that appends a random query string param to the URL for this test and filed 93195 to fix the apparent bug in chromium's DRT where it allows caches to persist between tests.
Comment 11 Ami Fischman 2012-08-04 23:29:29 PDT
Created attachment 156552 [details]
Patch
Comment 12 Ami Fischman 2012-08-04 23:36:27 PDT
Created attachment 156553 [details]
Patch
Comment 13 WebKit Review Bot 2012-08-04 23:39:45 PDT
Attachment 156553 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1
LayoutTests/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Ami Fischman 2012-08-04 23:52:20 PDT
Created attachment 156554 [details]
Patch
Comment 15 Ryosuke Niwa 2012-08-05 01:08:10 PDT
Comment on attachment 156554 [details]
Patch

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

> LayoutTests/media/video-poster-blocked-by-willsendrequest.html:43
> +                video.poster = "content/abe.png?" + Math.random();

Can we append timestamp instead of Math.random() so that it'll be different without doubt? i.e. P(query being same) === 0 instead of arbitrary small epsilon?
Comment 16 Ami Fischman 2012-08-05 01:12:17 PDT
Created attachment 156555 [details]
Patch
Comment 17 Ami Fischman 2012-08-05 01:12:34 PDT
(In reply to comment #15)
> (From update of attachment 156554 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156554&action=review
> 
> > LayoutTests/media/video-poster-blocked-by-willsendrequest.html:43
> > +                video.poster = "content/abe.png?" + Math.random();
> 
> Can we append timestamp instead of Math.random() so that it'll be different without doubt? i.e. P(query being same) === 0 instead of arbitrary small epsilon?

Good point.  Done.
Comment 18 Ryosuke Niwa 2012-08-05 01:30:28 PDT
Comment on attachment 156555 [details]
Patch

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

> LayoutTests/media/video-poster-blocked-by-willsendrequest.html:42
> +                // Since this is testing resource-loading being blocked, append a unique query string to avoid cache hits from other tests.
> +                // FIXME: it's madness that this needs to account for other tests.
> +                // Why isn't the cache being cleared between tests?  Looks like port-specific DRT bugs (chromium: 93195, mac: 82976, gtk: 79760).

You may want to make this comment more concise.
Comment 19 Ami Fischman 2012-08-05 08:36:50 PDT
Created attachment 156561 [details]
Patch
Comment 20 Ami Fischman 2012-08-05 08:37:22 PDT
(In reply to comment #18)
> (From update of attachment 156555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156555&action=review
> 
> > LayoutTests/media/video-poster-blocked-by-willsendrequest.html:42
> > +                // Since this is testing resource-loading being blocked, append a unique query string to avoid cache hits from other tests.
> > +                // FIXME: it's madness that this needs to account for other tests.
> > +                // Why isn't the cache being cleared between tests?  Looks like port-specific DRT bugs (chromium: 93195, mac: 82976, gtk: 79760).
> 
> You may want to make this comment more concise.

Done.  Thanks for the review.
Comment 21 WebKit Review Bot 2012-08-05 09:28:59 PDT
Comment on attachment 156561 [details]
Patch

Clearing flags on attachment: 156561

Committed r124717: <http://trac.webkit.org/changeset/124717>
Comment 22 WebKit Review Bot 2012-08-05 09:29:05 PDT
All reviewed patches have been landed.  Closing bug.