Bug 83874 – [GTK] r114021 triggered media flakyness

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Philippe Normand]

At least on GTK skipping:

media/video-load-require-user-gesture.html 
media/video-play-require-user-gesture.html

works around the issue but a proper solution is required. I won't have time to work on it this week though. Skipping those two for now.

[Philippe Normand]

The flaky tests list:

Unexpected flakiness: text diff mismatch (25)
  fast/forms/text-input-event.html = TEXT PASS
  media/track/track-mode.html = TEXT PASS
  media/video-loop.html = TEXT PASS
  media/video-muted.html = TEXT PASS
  media/video-pause-immediately.html = TEXT PASS
  media/video-play-empty-events.html = TEXT PASS
  media/video-play-pause-events.html = TEXT PASS
  media/video-play-pause-exception.html = TEXT PASS
  media/video-playbackrate.html = TEXT PASS
  media/video-played-collapse.html = TEXT PASS
  media/video-played-ranges-1.html = TEXT PASS
  media/video-played-reset.html = TEXT PASS
  media/video-preload.html = TEXT PASS
  media/video-reverse-play-duration.html = TEXT PASS
  media/video-seek-past-end-paused.html = TEXT PASS
  media/video-seek-past-end-playing.html = TEXT PASS
  media/video-set-rate-from-pause.html = TEXT PASS
  media/video-source-load.html = TEXT PASS
  media/video-src-invalid-poster.html = TEXT PASS
  media/video-src-none.html = TEXT PASS
  media/video-timeupdate-during-playback.html = TEXT PASS
  media/video-timeupdate-reverse-play.html = TEXT PASS
  media/video-transformed.html = TEXT PASS
  media/video-volume.html = TEXT PASS
  media/video-zoom-controls.html = TEXT PASS

[Simon Pena]

Running the tests with --run-singly (so each test uses its 
own DumpRenderTree) works fine. That reinforces the idea that the
MediaPlaybackRequiresUserGesture property, set on the following tests:

media/video-load-require-user-gesture.html 
media/video-play-require-user-gesture.html

is affecting the others.

[Simon Pena]

Created attachment 137687 [details]
Patch

[Martin Robinson]

Comment on attachment 137687 [details]
Patch

The API looks good to me. We just need one more GTK+ reviwer to approve the API and give the official r+.

[Philippe Normand]

Comment on attachment 137687 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 137687 [details]
Patch

Clearing flags on attachment: 137687

Committed r114506: <http://trac.webkit.org/changeset/114506>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Comment on attachment 137687 [details]
Patch

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

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430
> +                 "media-playback-requires-user-gesture", FALSE,

This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. 

Does the problem reproduce without this?

[Philippe Normand]

Reopening because the patch was rolled out.

[Philippe Normand]

(In reply to comment #8)
> (From update of attachment 137687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137687&action=review
> 
> > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430
> > +                 "media-playback-requires-user-gesture", FALSE,
> 
> This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. 
> 

Right. 

> Does the problem reproduce without this?

No, but I don't think it's an issue to keep this line anyway.

I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :)

Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException);

And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false);

Fixing the tests fixes flakyness here. But I think we still need Simon's patch anyway, maybe in a new bugzilla entry Simon?

[Philippe Normand]

Created attachment 137788 [details]
Patch

[Eric Carlson]

(In reply to comment #10)
> 
> I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :)
> 
> Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException);
> 
> And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false);
> 
  Oops :-(

[Eric Carlson]

Comment on attachment 137788 [details]
Patch

Thanks for fixing my mistake!

[Philippe Normand]

Comment on attachment 137788 [details]
Patch

Clearing flags on attachment: 137788

Committed r114586: <http://trac.webkit.org/changeset/114586>

[Philippe Normand]

All reviewed patches have been landed.  Closing bug.

[Simon Pena]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 137687 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137687&action=review
> > 
> > > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:430
> > > +                 "media-playback-requires-user-gesture", FALSE,
> > 
> > This shouldn't be necessary because both tests *should* call window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false) before ending. Also, I don't believe that any of the other DRT implementations does this cleanup. 
> > 
> 
> Right. 
> 
> > Does the problem reproduce without this?
> 
> No, but I don't think it's an issue to keep this line anyway.

When I added that line to the DumpRenderTree, I was assuming that, under some
concurrency situations, some tests were run with that setting enabled. Since
they passed fine when I used the --run-singly option, I though I was right.

However, after Phil's findings:

> I've been trying Simon's patch, after some investigation I think the tests are actually badly calling the InternalSettings API :)
> 
> Idl file has: void setMediaPlaybackRequiresUserGesture(in boolean enabled) raises(DOMException);
> 
> And the tests do: window.internals.settings.setMediaPlaybackRequiresUserGesture(document, false);

This does seem the cause for flakyness, and not the other.
 
> Fixing the tests fixes flakyness here. But I think we still need Simon's patch anyway, maybe in a new bugzilla entry Simon?

Yes, I guess we should still provide the API to enable/disable the
mediaPlaybackRequiresUserGesture setting (it is provided in other ports),
but we no longer need to clean up that setting from the DumpRenderTree.

I'll file a new bug for that, and attaching this patch without the DumpRenderTree part.

Thanks for the investigation, and sorry for the trouble!