Closed Bug 476731 Opened 11 years ago Closed 11 years ago

Media element should fire error event when src is a 404

Categories

(Core :: Audio/Video, defect, P2)

x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 4 obsolete files)

Attached file Mochitest (obsolete) —
If you have <video src="foo.ogg"></video> where "foo.ogg" resolves to a 404 error, the video element should send an error event to the video element after it fails to load.

See http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-load "If the media data can be fetched but is in an unsupported format, or can otherwise not be rendered at all"
Attached patch Patch v1 (obsolete) — Splinter Review
Chnages nsHTMLMediaElement::Load() so that it matches the spec and dispatches errors asynchronously when it can't find a valid resource to load. Load() also sends network errors when appropriate. Made nsHTMLMediaElement::NetworkError() send async events to match spec. Updated mochitests so that they stop the error event propagating - now when you have a media element with no src/source, it posts an error event when it's added to a document. This error can bubble up to the mochitest's body's onerror handler, and cause it to report a test failure.

This patch is based on top of Roc's patch for bug 455654.
Assignee: nobody → chris
Attachment #360390 - Attachment is obsolete: true
Attachment #360631 - Flags: superreview?(roc)
Attachment #360631 - Flags: review?(chris.double)
Attachment #360631 - Flags: superreview?(roc) → superreview+
Attachment #360631 - Flags: review?(chris.double) → review+
Blocks: 471220
Requesting blocking 1.9.1, this is needed for bug 464455 which is blocking 1.9.1.
Depends on: 455654
Flags: blocking1.9.1?
(In reply to comment #2)
> Requesting blocking 1.9.1, this is needed for bug 464455 which is blocking
> 1.9.1.

I mean blocking bug 462455.
Attached patch v1.1 Unbitrotten (obsolete) — Splinter Review
v1 unbitrotten. r+ doublec sr+ roc.
Attachment #360631 - Attachment is obsolete: true
Attachment #361199 - Flags: superreview+
Attachment #361199 - Flags: review+
If this is ready to land, mark it checkin-needed / [needs landing]?
Er, I guess it already is so marked. But on IRC you mentioned there was a problem with Wave. Is that resolved?
There's a livelock which shows up in bug 462455, this is ready to go. Bug 462455's patch requires this one.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Backed out due to test failures:
http://hg.mozilla.org/mozilla-central/rev/f51e5b101ca5
http://hg.mozilla.org/mozilla-central/rev/0088c5d48c8c

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234142462.1234145460.9210.gz
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/aspect-ratio-1b.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/aspect-ratio-2a.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/aspect-ratio-2b.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/basic-1.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/canvas-1a.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/canvas-1b.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/empty-1a.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/empty-1b.html | timed out waiting for reftest-wait to be removed (after onload fired)
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/ogg-video/zoomed-1.html | timed out waiting for reftest-wait to be removed (after onload fired)
Status: RESOLVED → REOPENED
Flags: blocking1.9.1+ → blocking1.9.1?
Priority: P2 → --
Resolution: FIXED → ---
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Attached patch Patch v2 (obsolete) — Splinter Review
As v1.1, but has fix for broken unit tests. A case in nsHTMLMediaElement::nsMediaLoadListener::OnStartRequest() would trigger for local files, where InitializeDecoderForChannel() would succeed, but return no listener, and the previous patch would trigger a network error in that case.

Also changes access controls mochitest so that it always recreates the video element on every individual test case. This prevents a random failure that was showing up. The previous load was not finishing before the next one started, so the networkState was not EMPTY, causing an abort event to fire when the load for the next test came in, which screwed things up.
Attachment #361199 - Attachment is obsolete: true
Attachment #361458 - Flags: superreview?(roc)
Attachment #361458 - Flags: review?(chris.double)
Attachment #361458 - Flags: superreview?(roc) → superreview+
Attachment #361458 - Flags: review?(chris.double) → review+
Duplicate of this bug: 475868
Keywords: checkin-needed
Whiteboard: [needs landing]
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment on attachment 361458 [details] [diff] [review]
Patch v2

Yay. Bitrot. test_play.html which was added since last revision is now failing.
Attachment #361458 - Attachment is obsolete: true
As v2, but with the recently added test fixed to not thrown an error in the new behaviour.
Attachment #362163 - Flags: superreview+
Attachment #362163 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Attachment #362163 - Attachment description: Patch v2.1 unbitrotten → Patch v2.1 unbitrotten [Checkin: Comment 15]
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [c-n: baking for 1.9.1]
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 362163 [details] [diff] [review]
Patch v2.1 unbitrotten
[Checkin: Comment 15+16]


http://hg.mozilla.org/mozilla-central/rev/c8cdcd82ba32
Missing file, copied from 'Patch v2'.

Please, check whether this file/code is up-to-date,
then attach a merged patch for 1.9.1.
Attachment #362163 - Attachment description: Patch v2.1 unbitrotten [Checkin: Comment 15] → Patch v2.1 unbitrotten [Checkin: Comment 15+16]
Patch for 1.9.1 branch. Note, this does not apply cleanly to 1.9.1. According to the whiteboard tags, the following video/audio bugs have been pushed to m-c but are still waiting to go into 1.9.1:

http://tinyurl.com/c5hffc

These probably need to be pushed to 1.9.1 before this patch will apply cleanly.

I used this query to generate that list:

http://tinyurl.com/as5r8u
Attachment #362492 - Flags: superreview+
Attachment #362492 - Flags: review+
Yeah. Serge: when you push patches to 1.9.1 (which I am very grateful for), please make sure you push in the order in which they landed in mozilla-central.
(In reply to comment #17)
> http://tinyurl.com/c5hffc

(In reply to comment #18)
> please make sure you push in the order in which they landed in mozilla-central.

I don't know (and won't look for) what this correct order is:
please update the whiteboard of these bugs with the needed details.
(Or (preferably !?) don't ask for c-n until this can actually be done.)
Fair enough. I have a script that will take a list of changesets and sort them into correct order which I use to do this.
Keywords: checkin-needed
Whiteboard: [c-n: baking for 1.9.1] → [needs 191 landing]
I think this created a regression  see.
Bug 478805 -  Changing VIDEO.src dont change the actual source file being played.
See bug 478805 comment 1. It's not a regression.
You need to log in before you can comment on or make changes to this bug.