Bug 92824 – media/video-poster-blocked-by-willsendrequest.html isn't blocking poster on willsendrequest

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ami Fischman]

@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

[Ryosuke Niwa]

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

[Ami Fischman]

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

[Ryosuke Niwa]

There used to be. Maybe this bug no longer reproduces.

[Peter Kasting]

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.

[Peter Kasting]

Note: That same test also bleeds into platform/chromium/fast/text/international/complex-text-rectangle.html sometimes.

[Ami Fischman]

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

[Alexey Proskuryakov]

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

[Ami Fischman]

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

[Eric Seidel (no email)]

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.

[Ami Fischman]

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.

[Ami Fischman]

Created attachment 156552 [details]
Patch

[Ami Fischman]

Created attachment 156553 [details]
Patch

[WebKit Review Bot]

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.

[Ami Fischman]

Created attachment 156554 [details]
Patch

[Ryosuke Niwa]

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?

[Ami Fischman]

Created attachment 156555 [details]
Patch

[Ami Fischman]

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

[Ryosuke Niwa]

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.

[Ami Fischman]

Created attachment 156561 [details]
Patch

[Ami Fischman]

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

[WebKit Review Bot]

Comment on attachment 156561 [details]
Patch

Clearing flags on attachment: 156561

Committed r124717: <http://trac.webkit.org/changeset/124717>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.