835075 - Setting src property on <video> tag fails if done after page loading is complete

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Mauricio Collares [:collares]]

Coursera's video player stopped working correctly for lectures that contain in-video quizzes.

How to reproduce the bug: Go to <https://class.coursera.org/automata/lecture/preview>. The second lecture ("Informal introduction to finite automata") contains an in-video quiz, as can be noted by looking at the yellow indicator in the seek bar; notice that all you get is a black screen. Compare that to the "Course outline and motivation" video (which does not contain a quiz).

I manually bisected this and found that the change in behavior was introduced by <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=904caf7811e4>. Was this caused by bug 804875, perhaps?

Comment 1

[Mauricio Collares [:collares]]

Actually, it's just easier to reproduce the bug on videos with quizzes, but the problem shows up on videos without quizzes as well. I now know that I don't have a deterministic way of reproducing the bug, but it happens very frequently. Let me know if you have trouble seeing what the problem is.

Comment 2

[Paul Adenot (:padenot)]

I can't reproduce here, could you provide precise step to reproduce? Note that the patches you point in comment 0 could certainly be the cause of the problem.

Comment 3

[Alice0775 White]

http://hg.mozilla.org/mozilla-central/rev/80fed51ae074
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130128 Firefox/21.0 ID:20130128030943

I can reproduce the problem in Windows7 if HWA disabled, but I cannot if HWA enabled

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/959b9ee8ac2f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130124 Firefox/21.0 ID:20130124110237
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/904caf7811e4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130124 Firefox/21.0 ID:20130124112935
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=959b9ee8ac2f&tochange=904caf7811e4

Comment 4

[Alice0775 White]

>I can reproduce the problem in Windows7 if HWA disabled, but I cannot if HWA enabled
this is wrong.

The problem happens with regardless of HWA setting  in windows7

STR
1. Open https://class.coursera.org/automata/lecture/preview
2. Click "Informal introduction to finite automata"
3. Click pause after the video started
4. Repeat Step 1 and step 2

Comment 5

[Mauricio Collares [:collares]]

Attachment: Reduced testcase

Comment 6

[Mauricio Collares [:collares]]

In the testcase, spark.js is only imported for getting jQuery, which MediaElement.js seemingly requires. MediaElement.js is available at <http://mediaelementjs.com/>. I guess this points at MediaElement.js,o but the demo on its website works, so I'm not sure.

Comment 7

[Mauricio Collares [:collares]]

I forgot to mention an important step in reproducing the reduced testcase, so here's what I did:

1) Load the file. You should get the video displaying just fine.
2) Hit Ctrl-R. It's gone!

Comment 8

[Paul Adenot (:padenot)]

I still can't repro on Linux, Mac, Windows, all three latest Nightly.

Comment 9

[Alice0775 White]

Umm, 
I can reproduce easily on Ubuntu and Windows7 with clean profile...

Comment 10

[Mauricio Collares [:collares]]

Attachment: Testcase with inlined unminified MediaElement.js

Stepping through in the debugger, line 1002 in this testcase is where the video disappears:

1002					htmlMediaElement.src = result.url;

This is the first (and only?) time this line is hit while running the script, so setting a breakpoint on it should give plenty of context. Is there any other information I can provide?

Comment 11

[Alice0775 White]

In local build(ubuntu12.04.1 32bit)
Last Good: 09ab58c800a1
First Bad: 49eaea676d49

Comment 12

[Mauricio Collares [:collares]]

Attachment: Minimized testcase

So it had nothing to do with MediaElement.js after all! The problem is somewhere in spark.js. I'll minimize it further, the bug should be easier to detect after that.

Comment 13

[Mauricio Collares [:collares]]

There's almost certainly some race condition going on; it's much much harder to reproduce the bug if I also save spark.js locally. Paul, can you try reproducing the bug by saving the HTML testcase on local disk (but keeping spark.js remote)? Try reloading the page a few times in case it doesn't work on first try, just in case.

Comment 14

[Mauricio Collares [:collares]]

Attachment: Minimized testcase

Step to reproduce: Click outside the video (there's an onclick handler on <body>).

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: mochitest patch v0

Thanks for the testcase, it's easy to reproduce for me using that.  I did need to click on the video itself (or on the whitespace to the right of it), because the <body> doesn't extend much beyond that.

It looks like this is specific to cases where the same resource is (re)used when setting the src.  When this happens, nsHTMLMediaElement detects that the resource can be shared in LoadResource (via LookupMediaElementURITable) and clones the existing decoder rather than creating a fresh one.

I've attached a patch for the mochitest added in bug 804875 that'll catch this bug.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Fixed by backout.
https://hg.mozilla.org/mozilla-central/rev/3bc1aa6145ed

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 707399 [details] [diff] [review]
mochitest patch v0

I'm r+ing that since I'm landing it along the fixes in bug 804875.

Comment 18

[Paul Adenot (:padenot)]

(Reopened so it can be closed again, not sure if this is right).

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2cb7a3df54

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed2cb7a3df54

Comment 20

[Mauricio Collares [:collares]]

Verified on 2013-02-05's nightly. Thanks a lot Paul, Mathew and Alice0775!

(By the way: The behavior of the testcase post-bugfix is a bit different than it was before; the video disappears and then comes back after a second. Not a problem at all, but I feel it's worth mentioning.)

Comment 21

[Mauricio Collares [:collares]]

*Matthew, sorry :)