Open Bug 697955 Opened 11 years ago Updated 3 years ago

There is no onload event fired for VideoDocuments

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

ASSIGNED

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

References

Details

(Whiteboard: [jetpack:p2])

Attachments

(1 file, 3 obsolete files)

So after I implemented document-element-inserted event for MediaDocuments and created jetpack tests for it I also realised this issue. 

If I understand it correctly the reason behind this issue is that in nsDocumentViewer.cpp here:

  // XXXbz imagelib kills off the document load for a full-page image with
  // NS_ERROR_PARSED_DATA_CACHED if it's in the cache.  So we want to treat
  // that one as a success code; otherwise whether we fire onload for the image
  // will depend on whether it's cached!
  if(window &&
     (NS_SUCCEEDED(aStatus) || aStatus == NS_ERROR_PARSED_DATA_CACHED)) {

the aStatus is NS_BINDING_ABORTED for video documents that come from:

MediaDocument.cpp MediaDocumentStreamListener::OnStartRequest

If I understand onload event correctly we could simply solve this by enable aStatus == NS_BINDING_ABORTED, however I have no clue yet what side effect might that cause...
Enabling onload for NS_BINDING_ABORTED would be totally wrong.

We should change the cancel code used by MediaDocumentStreamListener::OnStartRequest.  Why is it canceling the channel at that point anyway, exactly?
Component: DOM → Video/Audio
QA Contact: general → video.audio
I only found this: bug 466384
Component: Video/Audio → DOM
Ah. Yeah, that should be returning NS_ERROR_PARSED_DATA_CACHED then.
And note that this behavior is claimed to be specific to file://, yes?
Component: DOM → Video/Audio
(In reply to Boris Zbarsky (:bz) from comment #4)
> And note that this behavior is claimed to be specific to file://, yes?

Actually I have the same behaviour for any online ogv video as well, not only local files. Also, I'm still debugging where is that NS_BINDING_ABORTED coming from. I assumed that it must come from the MediaDocumentStreamListener::OnStartRequest but I'm not sure. It originates from the nsDocLoader::DocLoaderIsEmpty, from a mLoadGroup->GetStatus(&loadGroupStatus) call. Currently I'm trying to catch that moment where it is set, and understand more about this whole area...
Component: Video/Audio → DOM
> I assumed that it must come from the MediaDocumentStreamListener::OnStartRequest

That seems like the most likely place to me, yes.  Does changing that not fix the bug?
Attached patch proposed fix (obsolete) — Splinter Review
It does fix it. I just wanted to be sure that it fixes it in every cases, since there are tons of places in the code where NS_BINDING_ABORTED is used. But I could not reproduce the bug after the fix. I would love to run all tests to be sure that it does not cause any side effects though. Shall I ask for an L1 password for this?
Assignee: nobody → gkrizsanits
Status: NEW → ASSIGNED
Attachment #570703 - Flags: review?(bzbarsky)
Blocks: 697775
Comment on attachment 570703 [details] [diff] [review]
proposed fix

Please add a test for this?  If you put a media document in an iframe, does the onload on the iframe fire without this patch, for example?
Attachment #570703 - Flags: review?(bzbarsky) → review+
Sure, I will. I'm having some serious dev. env. issues right now, but will get back to this soon after I have my system stable and reliable again.
Attached patch proposed fix with test (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #8)
> Please add a test for this?  If you put a media document in an iframe, does
> the onload on the iframe fire without this patch, for example?

The same thing for in an iframe. Without the patch the event is not fired with the patch it is fired.
Attachment #570703 - Attachment is obsolete: true
Attachment #572804 - Flags: review?(bzbarsky)
Comment on attachment 572804 [details] [diff] [review]
proposed fix with test

The timeout thing is no good; it'll trigger random orange.

Why not just do this:

  <script>
    SimpleTest.waitForExplicitFinish();
  </script>
  <iframe src="short-video.ogv" onload="SimpleTest.finish()"></iframe>

which should be non-racy and all?  The failure behavior if the event doesn't fire will be the test timing out, but that's fine.
Attachment #572804 - Flags: review?(bzbarsky) → review-
Attached patch fixed test (obsolete) — Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #11)
> Comment on attachment 572804 [details] [diff] [review] [diff] [details] [review]
> Why not just do this:

I wanted the test to guard against double event too. I think I just overcomplicated it...
Attachment #572804 - Attachment is obsolete: true
Attachment #573179 - Flags: review?(bzbarsky)
bz: Is this test good as it is or shall I change something on it?
Comment on attachment 573179 [details] [diff] [review]
fixed test

r=me
Attachment #573179 - Flags: review?(bzbarsky) → review+
Attached patch final versionSplinter Review
I found some CR's in the previous patch so I fixed it. Other than that it's the same. Ed, could you check this in for me?
Attachment #573179 - Attachment is obsolete: true
Attachment #581575 - Flags: review+
Attachment #581575 - Flags: checkin?(bmo)
This is waiting on bug 709193. Also, has this been past try? :-)
Keywords: checkin-needed
Whiteboard: [jetpack:p2] → [jetpack:p2] [waiting on bug 709193]
(In reply to Ed Morley [:edmorley] from comment #16)
> This is waiting on bug 709193. Also, has this been past try? :-)

This looks like a pass to me: https://tbpl.mozilla.org/?tree=Try&rev=4a33ea8a7a0a
The jetpack tests had some oranges but I checked with Alex and it's a none issue ( bug 705011 )
So I misinterpreted the output of the try server, ignore my previous comment.
So I found the reason why is the test failing: Bug 521359
I need to rethink without this how can we use the document-element-inserted in jetpack page-mods.
Keywords: checkin-needed
Whiteboard: [jetpack:p2] [waiting on bug 709193] → [jetpack:p2]
Attachment #581575 - Flags: checkin?(bmo)
Since this onload event for MediaDocuments (videos and audios) is going against the specs, this is a won't fix...
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #20)
> Since this onload event for MediaDocuments (videos and audios) is going
> against the specs, this is a won't fix...

Is it? Link?
(In reply to Ms2ger from comment #21)
> Is it? Link?

I'll be honest, I've never seen that spec... According to the old fashioned definition of onload event I even thought that we should fire this event. But I've just seen Bug 521359 and then read Robert's blog about it: http://robert.ocallahan.org/2009/10/removing-media-element-event_16.html and it was convincing enough for me. Shall I CC-him? Also, if I'm overlooking something here let me know.
That's something else entirely afaict.
Ms2ger is correct. That blog post says that we no longer fire a "load" event targeted at <video> and <audio> elements when they complete loading.

Documents containing media elements should still fire a "load" event targeted at the document, but note that media elements in a document delay the "load" event of the document until all contained media elements have either reached HAVE_CURRENT_DATA readyState or they've decided they can't load any resource.

For more info see:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#delaying-the-load-event-flag

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#concept-media-load-resource
I've got back to this bug and I'm a bit stuck on this one. Now it is clear what we should do but I don't get how to do it and why does my patch has this side effect. So my patch allowed to fired exactly the document load event (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#1048) and I don't get how does the load event for the media element fired because of this (this event is fired as a side effect: http://mxr.mozilla.org/mozilla-central/source/content/media/test/test_playback.html?force=1#66)

Anyone could give me a hint how to progress with this bug? I cannot find the place where the load event related to the media element is fired...
The test looks valid to me. This entire bug is unrelated to whether elements fire a "load" event. Can you explain why the test fails?
No, I don't understand it. The reason why I was mixed up the two events because this test is failing if I apply my patch. So I played with it a little and it turns out that even without my patch running only the test_playback.html test in standalone mode, it fails. But if I run all the tests then it's passing (still without my patch), and fails every now and then with my patch. So it looks like the load event for media elements are fired without my patch just because some timing issue the test fails to detect it, and my patch is enough change to make the test work all of a sudden (not in every case though...). Then again, I have no clue where is the media element related load even fired... But running the test_playback.html standalone fails 100% of the time, unrelated to my patch so I guess we should open a new bug for that and when it is fixed I could land this patch (after updating it...).
You're saying test_playback sees a load event fire on the element for you?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> You're saying test_playback sees a load event fire on the element for you?

Yes. Let me be very explicit. Clean checkout, Win 7, 32 bit debug build (no opt) command: 
TEST_PATH=content/media/test/test_playback.html make -C obj-debug mochitest-plain fails with: 

Passed: 1299
Failed: 70
Todo: 0
failed | r11025_u8_c1_trunc.wav should not fire 'load' event
failed | r11025_u8_c1.wav should not fire 'load' event
failed | r11025_s16_c1_trailing.wav should not fire 'load' event
failed | r11025_s16_c1_trailing.wav should not fire 'load' event
failed | r16000_u8_c1_list.wav should not fire 'load' event
failed | r16000_u8_c1_list.wav should not fire 'load' event
failed | bug461281.ogg should not fire 'load' event
failed | bug461281.ogg should not fire 'load' event
failed | bug482461.ogv should not fire 'load' event
failed | bug482461.ogv should not fire 'load' event
failed | bug482461-theora.ogv should not fire 'load' event
failed | bug482461-theora.ogv should not fire 'load' event
failed | bug500311.ogv should not fire 'load' event
failed | bug500311.ogv should not fire 'load' event
failed | small-shot.ogg should not fire 'load' event
failed | small-shot.ogg should not fire 'load' event
failed | short-video.ogv should not fire 'load' event
failed | short-video.ogv should not fire 'load' event
failed | bug504613.ogv should not fire 'load' event
failed | bug504613.ogv should not fire 'load' event
failed | bug516323.ogv should not fire 'load' event
failed | bug516323.ogv should not fire 'load' event
failed | bug556821.ogv should not fire 'load' event
failed | bug556821.ogv should not fire 'load' event
failed | beta-phrasebook.ogg should not fire 'load' event
failed | beta-phrasebook.ogg should not fire 'load' event
failed | bug520493.ogg should not fire 'load' event
failed | bug520493.ogg should not fire 'load' event
failed | bug520500.ogg should not fire 'load' event
failed | bug520500.ogg should not fire 'load' event
failed | bug499519.ogv should not fire 'load' event
failed | bug499519.ogv should not fire 'load' event
failed | bug506094.ogv should not fire 'load' event
failed | bug506094.ogv should not fire 'load' event
failed | bug498855-1.ogv should not fire 'load' event
failed | bug498855-1.ogv should not fire 'load' event
failed | bug498855-2.ogv should not fire 'load' event
failed | bug498855-2.ogv should not fire 'load' event
failed | bug498855-3.ogv should not fire 'load' event
failed | bug498855-3.ogv should not fire 'load' event
failed | bug504644.ogv should not fire 'load' event
failed | bug504644.ogv should not fire 'load' event
failed | chain.ogv should not fire 'load' event
failed | chain.ogv should not fire 'load' event
failed | bug523816.ogv should not fire 'load' event
failed | bug523816.ogv should not fire 'load' event
failed | bug495129.ogv should not fire 'load' event
failed | bug495129.ogv should not fire 'load' event
failed | bug498380.ogv should not fire 'load' event
failed | bug498380.ogv should not fire 'load' event
failed | bug495794.ogg should not fire 'load' event
failed | bug495794.ogg should not fire 'load' event
failed | bug557094.ogv should not fire 'load' event
failed | bug557094.ogv should not fire 'load' event
failed | multiple-bos.ogg should not fire 'load' event
failed | multiple-bos.ogg should not fire 'load' event
failed | audio-overhang.ogg should not fire 'load' event
failed | audio-overhang.ogg should not fire 'load' event
failed | video-overhang.ogg should not fire 'load' event
failed | video-overhang.ogg should not fire 'load' event
failed | audio-gaps.ogg should not fire 'load' event
failed | audio-gaps.ogg should not fire 'load' event
failed | redirect.sjs?domain=mochi.test:8888&file=320x240.ogv should not fire 'load' event
failed | redirect.sjs?domain=mochi.test:8888&file=320x240.ogv should not fire 'load' event
failed | seek.webm should not fire 'load' event
failed | seek.webm should not fire 'load' event
failed | split.webm should not fire 'load' event
failed | split.webm should not fire 'load' event
failed | spacestorm-1000Hz-100ms.ogg should not fire 'load' event
failed | spacestorm-1000Hz-100ms.ogg should not fire 'load' event



however when running command: TEST_PATH=content/media make -C obj-debug mochitest-plain the very same test (and every other media test) passes nicely.

Since I cannot find the place where this load even is fired in the source code I could not really debug it. I suggest to file a new bug about this and make this one dependent on that new bug.
I filed bug 715469 for that with some debugging info.
Depends on: 715469
So I tested the patch after the fix of bug 715469 and the previous issue was fixed but the test_playback.html is still failing randomly.

https://tbpl.mozilla.org/?tree=Try&rev=192141ee8a5e

77818 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | short-video.ogv duration (Infinity) should be around 1.081

Based on the text, the error comes from a previous test: http://mxr.mozilla.org/mozilla-central/source/content/media/test/test_replay_metadata.html?force=1#60

How can the currentTime be Infinity? Other times the test works just fine (locally at least). roc: do you have any clue how can this happen?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #31)
> 77818 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/media/test/test_playback.html | short-video.ogv duration
> (Infinity) should be around 1.081
> 
> Based on the text, the error comes from a previous test:
> http://mxr.mozilla.org/mozilla-central/source/content/media/test/
> test_replay_metadata.html?force=1#60

Why do you think it's not test_playback?

> How can the currentTime be Infinity? Other times the test works just fine
> (locally at least). roc: do you have any clue how can this happen?

That means we couldn't find a duration or we thought the resource was a live stream. Odd... Are you sure this isn't a known orange?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
Thanks for the quick response, and sorry for the late answer but it looks like I don't get bugzilla email notifications sometimes so I found your response just now... 

> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #31)
> 
> Why do you think it's not test_playback?

So I double checked this it's more likely from http://mxr.mozilla.org/mozilla-central/source/content/media/test/manifest.js#279 which I don't know much about. 

> That means we couldn't find a duration or we thought the resource was a live
> stream. Odd... Are you sure this isn't a known orange?

No, I'm not, actually my hope that it is a known orange, since it does not really seem to be related to my patch at first glance... So if someone could validate me that this is a good enough result: https://tbpl.mozilla.org/?tree=Try&rev=192141ee8a5e that would be great... Do you know a person I could ping about this?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #33)
> So if someone could
> validate me that this is a good enough result:
> https://tbpl.mozilla.org/?tree=Try&rev=192141ee8a5e that would be great...

That's orange on every mochitest-1 box; the boxes which run media tests. So I it's a pretty safe bet that's not a known random orange, its a new perma-orange introduced by your patch.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.