Closed Bug 971229 Opened 6 years ago Closed 6 years ago

test_videocontrols times out if it loads real quick

Categories

(Toolkit :: Video/Audio Controls, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: cpearce, Assigned: cpearce)

Details

Attachments

(1 file, 1 obsolete file)

test_videocontrols can fail to start if the tests loads very quickly. I discovered this when I was improving our video decoding pipeline; I must have made it faster! ;)

What can happen is since we only add the load and canplaythrough listeners in startTest(), and since startTest() is called as a SpecialPowers.pushPrefEnv callback, if the page loads and canplaythrough fires before the pref callback has a chance to run the test's maybeStartTest() function will never be called as the "load" and "canplaythrough" listeners won't receive any events since those events have already been dispatched. And so we'll timeout.

You can see this behaviour if you reload the page after the test completes; because all the page's content is in the HTTP cache, it'll load fast and we'll hit this condition.

I'll upload a patch.
Add the "load" and "canplaythrough" listeners earlier, but still delay test starting until the pref is set. Add a check to ensure the pref doesn't start twice (since we have to call maybeStartTest() inside startTest(), we need to prevent this).
Attachment #8374399 - Flags: review?(jaws)
Comment on attachment 8374399 [details] [diff] [review]
Patch: Add load and canplaythrough listeners earlier in test_videocontrols

Review of attachment 8374399 [details] [diff] [review]:
-----------------------------------------------------------------

What if we just don't set the mediaElement.src until the test is ready?

::: toolkit/content/tests/widgets/test_videocontrols.html
@@ +46,5 @@
>  const scrubberCenterY = videoHeight - Math.round(scrubberHeight / 2);
>  
>  function runTest(event) {
> +  ok(true, "----- test #" + testnum + " ----- event.type=" + event.type);
> +  

nit, whitespace added here
Attachment #8374399 - Flags: review?(jaws)
Attached patch Patch v2Splinter Review
Yes, that is simpler. :)
Attachment #8374399 - Attachment is obsolete: true
Attachment #8374485 - Flags: review?(jaws)
Comment on attachment 8374485 [details] [diff] [review]
Patch v2

Review of attachment 8374485 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks!
Attachment #8374485 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/1255b096d6d8
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.