462455 - Appending <source> child element to <video> element does not initiate media load

Status: RESOLVED FIXED

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

Description

[Matthew Gregan [:kinetik]]

Starting with a <video> element with no src attribute or <source> child and attempting to load one later by appending a <source> child element does not initiate media load.  Setting the src attribute works as expected.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: mochitest

Comment 2

[Matthew Gregan [:kinetik]]

I mentioned this bug to roc and he said it should be blocking; requesting b1.9.1.

Comment 3

[Damon Sicore (:damons)]

How we doing here?  Been a while since this blocker has been updated.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Should be relatively easy to fix.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - Override nsHTMLSourceElement::BindToTree()

Override nsHTMLSourceElement::BindToTree() to asynchronously ensure that the owning media element starts a load. The spec doesn't specify what to do if a child source element is removed from the media element, so I don't do anything on unbind. Includes mochitest based on Matthew Gregan's original mochitest.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  is(media.currentSrc, "http://localhost:8888/tests/content/media/video/test/320x240.ogv");

I don't think we should depend on the exact URL including port etc. Just check that the last part is "/320x240.ogv".

Otherwise the patch looks good. We don't want to do this event when the parent element itself has only just been bound to the tree and started loading, but I think that works OK here because if the parent has started loading, networkState will not be EMPTY.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

Similar to v1, except:
* Fixes livelock that showed up in nsWaveDecoder. Caused by us not being able to distinguish reading 0 bytes due to EOF with reading 0 bytes due to slow incoming data. ReadAll() in nsWaveDecoder.cpp would get stuck in an infinite loop if shutdown came in while it was reading.
* Added extra mochitest to ensure we don't start loading <video> until </video> is parsed.
* Enabled all tests in test_media_selection.html.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 361206 [details] [diff] [review]
Patch v2

+PRBool nsHTMLMediaElement::ShouldLoadOnSourceBind()
+{
+  // Binding a source element to a media element could trigger a new load.
+  // See HTML spec, '4.8.9 The source element' for conditions:
+  // http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#the-source-element
+  // Note: we must not start a load if we're in nsHTMLMediaElement::BindToTree(),
+  // that will trigger a load when it completes.
+  return IsInDoc() &&

You don't need to check IsInDoc here. A child <source> element can't bind to the tree while its parent is not in the tree.

+  nsCOMPtr<nsHTMLMediaElement> media = do_QueryInterface(aParent);

nsHTMLMediaElement has no IID so it's not really clear what this is doing. You probably need to give it one.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #8)

> +  nsCOMPtr<nsHTMLMediaElement> media = do_QueryInterface(aParent);
> 
> nsHTMLMediaElement has no IID so it's not really clear what this is doing. You
> probably need to give it one.

I'm unsure how to add an IID here. Do you know where I can find an example?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually a better idea is probably to add a bit to IsNodeOfType in nsINode.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #8)
> (From update of attachment 361206 [details] [diff] [review])
> +PRBool nsHTMLMediaElement::ShouldLoadOnSourceBind()
> +{
> +  // Binding a source element to a media element could trigger a new load.
> +  // See HTML spec, '4.8.9 The source element' for conditions:
> +  //
> http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#the-source-element
> +  // Note: we must not start a load if we're in
> nsHTMLMediaElement::BindToTree(),
> +  // that will trigger a load when it completes.
> +  return IsInDoc() &&
> 
> You don't need to check IsInDoc here. A child <source> element can't bind to
> the tree while its parent is not in the tree.

Not true, we're getting called with a null document when a source element is appended to a <video> which isn't yet in a doc, so we need the check there. Here's a stack:

gklayout.dll!nsHTMLSourceElement::BindToTree(nsIDocument * aDocument=0x00000000, nsIContent * aParent=0x05868408, nsIContent * aBindingParent=0x00000000, int aCompileEventHandlers=0x00000001)  Line 146	C++
gklayout.dll!nsGenericElement::doInsertChildAt(nsIContent * aKid=0x05833338, unsigned int aIndex=0x00000000, int aNotify=0x00000001, nsIContent * aParent=0x05868408, nsIDocument * aDocument=0x00000000, nsAttrAndChildArray & aChildArray={...})  Line 3257 + 0x1c bytes	C++
gklayout.dll!nsGenericElement::InsertChildAt(nsIContent * aKid=0x05833338, unsigned int aIndex=0x00000000, int aNotify=0x00000001)  Line 3202 + 0x25 bytes	C++
gklayout.dll!nsGenericElement::doReplaceOrInsertBefore(int aReplace=0x00000000, nsIDOMNode * aNewChild=0x05833358, nsIDOMNode * aRefChild=0x00000000, nsIContent * aParent=0x05868408, nsIDocument * aDocument=0x00000000, nsIDOMNode * * aReturn=0x002fe6a8)  Line 3942 + 0x1c bytes	C++
gklayout.dll!nsGenericElement::InsertBefore(nsIDOMNode * aNewChild=0x05833358, nsIDOMNode * aRefChild=0x00000000, nsIDOMNode * * aReturn=0x002fe6a8)  Line 3524 + 0x20 bytes	C++
gklayout.dll!nsHTMLVideoElement::InsertBefore(nsIDOMNode * newChild=0x05833358, nsIDOMNode * refChild=0x00000000, nsIDOMNode * * _retval=0x002fe6a8)  Line 52 + 0x18 bytes	C++
gklayout.dll!nsGenericElement::AppendChild(nsIDOMNode * aNewChild=0x05833358, nsIDOMNode * * aReturn=0x002fe6a8)  Line 502	C++
gklayout.dll!nsHTMLVideoElement::AppendChild(nsIDOMNode * newChild=0x05833358, nsIDOMNode * * _retval=0x002fe6a8)  Line 52 + 0x14 bytes	C++
xpc3250.dll!nsIDOMNode_AppendChild(JSContext * cx=0x0400b080, unsigned int argc=0x00000001, int * vp=0x041b4318)  Line 2415 + 0x3e bytes	C++
js3250.dll!js_Interpret(JSContext * cx=0x0400b080)  Line 5006 + 0x17 bytes	C++
[...]


(In reply to comment #10)
> Actually a better idea is probably to add a bit to IsNodeOfType in nsINode.

You mean like this?

>  if (!aParent->IsNodeOfType(nsINode::eMEDIA))
>    return NS_OK;
>
>  nsHTMLMediaElement* media = reinterpret_cast<nsHTMLMediaElement*>(aParent);

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, I lied about IsInDoc.

That's the right approach for IsNodeOfType, but you should use a static_cast instead of reinterpret_cast.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

As v2, with roc's review comments addressed. Based on top of patch for bug 476731.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+nsHTMLMediaElement::IsNodeOfType(PRUint32 aFlags) const
+{
+  return !(aFlags & ~(eCONTENT | eELEMENT | eMEDIA));
+}

Should call the superclass method here.

Maybe it would be simpler if nsHTMLSourceElement::BindToTree just called a method nsHTMLMediaElement::NotifyAddedSource which does the check and fires the event if necessary.

nsHTMLMediaElement::DoneAddingChildren calls Load synchronously. Perhaps it should be firing the event instead.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #14)
> +nsHTMLMediaElement::IsNodeOfType(PRUint32 aFlags) const
> +{
> +  return !(aFlags & ~(eCONTENT | eELEMENT | eMEDIA));
> +}
> 
> Should call the superclass method here.

Hmmm... No other implementation of IsNodeOfType() does. Do we want to diverge from the standard approach?

For it to be correct, it would have to be:

PRBool
nsHTMLMediaElement::IsNodeOfType(PRUint32 aFlags) const
{
  PRBool isGenericElement = nsGenericHTMLElement::IsNodeOfType(aFlags & ~eMEDIA);
  return isGenericElement ||
         isGenericElement && (aFlags & eMEDIA) ||
         aFlags == eMEDIA;
}

Surely the standard approach is cleaner? Oops, the original should return !(aFlags & ~(eCONTENT | eELEMENT | eHTML | eMEDIA)).

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #15)
> (In reply to comment #14)
> > +nsHTMLMediaElement::IsNodeOfType(PRUint32 aFlags) const
> > +{
> > +  return !(aFlags & ~(eCONTENT | eELEMENT | eMEDIA));
> > +}
> > 
> > Should call the superclass method here.
> 
> Hmmm... No other implementation of IsNodeOfType() does. Do we want to diverge
> from the standard approach?

No, although that standard approach is pretty bad, because it means if you want to add a flag say to HTML elements you have to find all the subclasses implementing IsNodeOfType and modify them. But let's do that anyway.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4

As v3, but with nsHTMLMediaElement::NotifySourceAdded(), as per comment 14.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

MediaLoadEvent should move to nsHTMLMediaElement.cpp, it's not needed in the header file anymore.

Shouldn't we coalesce multiple pending load events? Or rather, have Load() clear out all pending load events, since we want an explicit load() call to cancel any pending events too?

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #18)
> Shouldn't we coalesce multiple pending load events? Or rather, have Load()
> clear out all pending load events, since we want an explicit load() call to
> cancel any pending events too?

That's not specified behaviour... Chris Double also raised this issue, and he's going to post to WHATWG to try and get some feedback on this.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For the record, we decided it actually is the specified behaviour.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5

With load task coalesced, includes mochitest for that case.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This isn't quite the right way to do it. Consider this example:
1) script appends <source> to trigger implicit load, sets mLoadTaskQueued to true
2) script calls load() explicitly, sets mLoadTaskQueued to false
3) script triggers some asynchronous event dispatch, say volumechanged
4) script appends another <source> to trigger another implicit load, sets 
mLoadTaskQueued to true
5) MediaLoadEvent from step #1 runs, does the load(), which fires "aborted" synchronously and sets mLoadTaskQueued to false
6) "volumechanged" handler runs

Basically the MediaLoadEvent from step #1 should not run, the MediaLoadEvent from step #4 should run. Script can observe the difference because the implicit load appears to happen out of order with other asynchronously dispatched events.

How about we fix this by creating a load object for each load, like we talked about for the new load algorithm. But here we don't have to move any functionality into the load object, we can just use it to check if events correspond to the current load and have them do nothing if they're not.

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v6

Patch v6. As v5 expect:
* Adds nsMediaLoadObject to track loads/events.
* Modified test_load_coalescing.html to test case mentioned by Roc in comment 22.
* Changed load() so that it returns NS_OK on failure - failure codes are interpreted as exceptions in JS, and that's not supposed to happen, errors in load() are expressed by error events. This was required for test_load_coalescing.html to work.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 361851 [details] [diff] [review]
Patch v6

Your patch will need to be updated to the trunk since I landed some changes last night.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This class shouldn't need to inherit from nsISupports just to get refcounting. We should use inline Addref/Release like Thebes does:
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/public/gfxTypes.h#89
But we don't want to use these Thebes-specific macros. Benjamin, I think those macros should move to nsISupportsImpl.h (and be given generic names), what do you think?

Comment 26

[cajbir (:cajbir)]

Comment on attachment 361851 [details] [diff] [review]
Patch v6

>+  // Set a new load object. This will cause implicit load events which were
>+  // enqueued before with a differnet load object to silently be cancelled.

Nit: "differnet" spelling

Comment 27

[Benjamin Smedberg]

If you're going to have generic macros, please do it right, as in bug 421127. nsISupportsImpl.h is definitely not a good place for macros that aren't related to implementing nsISupports at all.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, we'll just use nsISupports here and wait on 421127 to be resolved.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or rather, *not* wait.

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

We could use nsCountedRef from nsAutoRef.h, but still we'd need to add something similar to THEBES_INLINE_DECL_REFCOUNTING() in nsAutoRef.h, as nsAutoRef.h doesn't add a refcounting implementation, just a pointer to objects that are refcounted.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v7

As v6, but updated to tip, with spelling error fixed. Still using nsISupports for nsMediaLoad. r+ doublec, sr+ roc.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Note this patch is based on bug 476731, that must land before this one can.

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v7.1 - with missing test files

I re-checked out my tree and forgot to re-add the test files, here's the patch v7 with tests re-added. Still applies cleanly to trunk.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/599deb701590

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed bustage fix http://hg.mozilla.org/mozilla-central/rev/506199089c3c

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0f5a0a5a2301
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9083582856d2