HTMLMediaElement::webMediaPlayer() should never be null if m_readyState >= HAVE_METADATA (multiple …

HTMLMediaElement::webMediaPlayer() should never be null if m_readyState >= HAVE_METADATA (multiple source)

BUG=382721
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177353

[Srirama, 2014-06-23 15:00:49]

This is to fix another case where webMediaPlayer() will be null when
m_readyState >= HAVE_METADATA.
I have verified with firefox and it does ignore the source element insertion
when the first source was already loaded.
PTAL.

[acolwell GONE FROM CHROMIUM, 2014-06-23 16:49:41]

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
File LayoutTests/media/media-source-append-multiple.html (right):

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:8: function logEvent(audio,
name) {
Remove. You should be using waitForEvent instead.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:16: endTest();
Add a check before this that makes sure that currentSrc still equals the sourceA
URL?

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:19: function
loadedMetadataA() {
nit: { on next line here an every other instance like this.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:24:
audio.addEventListener("loadedmetadata", loadedMetadataB);
nit: Use waitForEventAndFail() here instead.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:36: logEvent(audio,
"loadstart");
Why don't you just use waitForEvent() since you are already including
video-test.js? This should work if you call findMediaElement() at the top of
this function.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:52:
audio.addEventListener("loadedmetadata", loadedMetadataA);
nit: Use waitForEventOnce("loadedmetadata", loadedMetadataA) here so you don't
have to remove the event listener above.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:60: <audio id="a"
controls></audio>
nit: remove controls since it is not needed for this test.

[Srirama, 2014-06-23 19:19:26]

PTAL.

Is the expected output correct with respect to the url check? it prints the
absolute url in the expected output, do i need to make it general?

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
File LayoutTests/media/media-source-append-multiple.html (right):

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:8: function logEvent(audio,
name) {
On 2014/06/23 16:49:40, acolwell wrote:
> Remove. You should be using waitForEvent instead.

Done.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:16: endTest();
On 2014/06/23 16:49:40, acolwell wrote:
> Add a check before this that makes sure that currentSrc still equals the
sourceA
> URL?

Done.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:19: function
loadedMetadataA() {
On 2014/06/23 16:49:40, acolwell wrote:
> nit: { on next line here an every other instance like this.

Done.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:24:
audio.addEventListener("loadedmetadata", loadedMetadataB);
On 2014/06/23 16:49:40, acolwell wrote:
> nit: Use waitForEventAndFail() here instead.

Done.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:36: logEvent(audio,
"loadstart");
Used waitForEvent() but not able to use findMediaElement() as it was not finding
audio element.
On 2014/06/23 16:49:40, acolwell wrote:
> Why don't you just use waitForEvent() since you are already including
> video-test.js? This should work if you call findMediaElement() at the top of
> this function.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:52:
audio.addEventListener("loadedmetadata", loadedMetadataA);
On 2014/06/23 16:49:40, acolwell wrote:
> nit: Use waitForEventOnce("loadedmetadata", loadedMetadataA) here so you don't
> have to remove the event listener above.

Done.

https://codereview.chromium.org/349973006/diff/60001/LayoutTests/media/media-...
LayoutTests/media/media-source-append-multiple.html:60: <audio id="a"
controls></audio>
On 2014/06/23 16:49:40, acolwell wrote:
> nit: remove controls since it is not needed for this test.

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-23 20:30:50]

lgtm % nits

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
File LayoutTests/media/media-source-append-multiple-expected.txt (right):

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
LayoutTests/media/media-source-append-multiple-expected.txt:9: EXPECTED
(audio.currentSrc ==
'file:///E:/chrome/winchrome/src/third_party/WebKit/LayoutTests/media/content/test.wav')
OK
You should not have an absolute URL in the expectations since none of the bots
will actually match this. Try using testExpected("audio.currentSrc ==
sourceA.src", true);

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
File LayoutTests/media/media-source-append-multiple.html (right):

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
LayoutTests/media/media-source-append-multiple.html:21:
waitForEventAndFail("loadedmetadata", null);
nit: You shouldn't need the null. waitForEventAndFail() only has one parameter
in video-test.js

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
LayoutTests/media/media-source-append-multiple.html:34:
audio.addEventListener('canplaythrough', canplaythrough);
nit: Use waitForEventOnce() here as well and remove the consoleWrite() method
from canplaythrough().

[Srirama, 2014-06-24 07:04:47]

Cleaned up as per the comments before final checkin.

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
File LayoutTests/media/media-source-append-multiple-expected.txt (right):

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
LayoutTests/media/media-source-append-multiple-expected.txt:9: EXPECTED
(audio.currentSrc ==
'file:///E:/chrome/winchrome/src/third_party/WebKit/LayoutTests/media/content/test.wav')
OK
On 2014/06/23 20:30:49, acolwell wrote:
> You should not have an absolute URL in the expectations since none of the bots
> will actually match this. Try using testExpected("audio.currentSrc ==
> sourceA.src", true);

Done.

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
File LayoutTests/media/media-source-append-multiple.html (right):

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
LayoutTests/media/media-source-append-multiple.html:21:
waitForEventAndFail("loadedmetadata", null);
On 2014/06/23 20:30:49, acolwell wrote:
> nit: You shouldn't need the null. waitForEventAndFail() only has one parameter
> in video-test.js

Done.

https://codereview.chromium.org/349973006/diff/100001/LayoutTests/media/media...
LayoutTests/media/media-source-append-multiple.html:34:
audio.addEventListener('canplaythrough', canplaythrough);
On 2014/06/23 20:30:49, acolwell wrote:
> nit: Use waitForEventOnce() here as well and remove the consoleWrite() method
> from canplaythrough().

Done.

[Srirama, 2014-06-24 07:05:07]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2014-06-24 07:05:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/srirama.m@samsung.com/349973006/120001

[commit-bot: I haz the power, 2014-06-24 08:09:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/12511)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13272)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 08:32:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 08:32:57]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[Srirama, 2014-06-27 15:51:38]

Updated with test case failures. Modified the logic now.
If there is no current source and there is no nextchildtoconsider then we use
the new source as nextchildtoconsider.
PTAL

[philipj_slow, 2014-06-27 22:06:14]

Sorry I'm late to the party, but I don't think I understand this patch. Can you
describe how "HTMLMediaElement::webMediaPlayer() should never be null if
m_readyState >= HAVE_METADATA" was previously violated?

AFAICT, the code in PS5 is going amount to a spec violation if it actually runs,
because the condition that follows will be ignored. If it's guaranteed that the
following condition won't apply, maybe you can change the order and/or
ASSERT(networkState() != HTMLMediaElement::NETWORK_EMPTY) if that's why it's
safe?

[Srirama, 2014-06-28 06:23:04]

On 2014/06/27 22:06:14, philipj wrote:
> Sorry I'm late to the party, but I don't think I understand this patch. Can
you
> describe how "HTMLMediaElement::webMediaPlayer() should never be null if
> m_readyState >= HAVE_METADATA" was previously violated?
> 

When a source element is playing its networks state will not be NETWORK_EMPTY
and its readystate will be >= HAVE_METADATA. Now webMediaPlayer() is not null
and is fine. But at this point if we remove the current source using js then
currentsource will be set to null in sourceaWasRemoved. At this point both
m_currentSourceNode and m_nextChildNodeToConsider are null and network state is
not NETWORK_EMPTY. Now if we add a new source element through js then it skips
through all the conditions in sourceWasAdded and triggers
scheduleNextSourceChild(). This will finally call loadNextSourceChild() which is
creating mediaplayer again. This will clear the webMediaPlayer() but the
readystate is still >= HAVE_METADATA. This is what i am tying to address through
the current patch.

> AFAICT, the code in PS5 is going amount to a spec violation if it actually
runs,
> because the condition that follows will be ignored. If it's guaranteed that
the
> following condition won't apply, maybe you can change the order and/or
> ASSERT(networkState() != HTMLMediaElement::NETWORK_EMPTY) if that's why it's
> safe?

I didn't exactly understand your query here. we will discuss directly when
possible.
Let me explain why have i have done the current changes.
As per spec 
Note: Dynamically modifying a source element and its attribute when the element
is already inserted in a video or audio element will have no effect. To change
what is playing, just use the src attribute on the media element directly.

So as i understand even though current source is removed and new source is
inserted, we should keep playing the current media as it has already started. I
have verified in firefox and the behaviour is same. In the DOM even though the
current source is removed and new source is inserted still video.currentSrc
points to current source element's src value.

[philipj_slow, 2014-06-30 11:35:51]

On 2014/06/28 06:23:04, Srirama wrote:
> On 2014/06/27 22:06:14, philipj wrote:
> > Sorry I'm late to the party, but I don't think I understand this patch. Can
> you
> > describe how "HTMLMediaElement::webMediaPlayer() should never be null if
> > m_readyState >= HAVE_METADATA" was previously violated?
> > 
> 
> When a source element is playing its networks state will not be NETWORK_EMPTY
> and its readystate will be >= HAVE_METADATA. Now webMediaPlayer() is not null
> and is fine. But at this point if we remove the current source using js then
> currentsource will be set to null in sourceaWasRemoved. At this point both
> m_currentSourceNode and m_nextChildNodeToConsider are null and network state
is
> not NETWORK_EMPTY. Now if we add a new source element through js then it skips
> through all the conditions in sourceWasAdded and triggers
> scheduleNextSourceChild(). This will finally call loadNextSourceChild() which
is
> creating mediaplayer again. This will clear the webMediaPlayer() but the
> readystate is still >= HAVE_METADATA. This is what i am tying to address
through
> the current patch.

Ah, thank you for this detailed analysis. It seems like the current code has at
least these bugs:

1. In sourceWasRemoved, m_currentSourceNode is cleared, which apart from the bug
you've found means that no error event will be fired on it in case of a network
error.

2. In sourceWasAdded, the "m_currentSourceNode && source ==
m_currentSourceNode->nextSibling()" condition looks broken, because it assumes
that there is no non-source Node between m_currentSourceNode and source, which
need not be the case.

3. In sourceWasAdded, there's confusion about where in the resource selection
algorithm we are. Even if the above were fixed, it still looks like it could
fall through all of the conditions and mistakenly think that it's in the waiting
step of the resource selection algorithm. To me it looks like sourceWasAdded()
should check m_loadState == WaitingForSource before jumping in to the resource
selection algorithm steps, or at the very least ASSERT it if the other checks
combined would imply it, which I think they currently do not.

As a quickfix, simply checking m_loadState == WaitingForSource would be an
improvement. I'm not certain, but I think fixing bug 1 and 2 above could also
fix the problem.

> > AFAICT, the code in PS5 is going amount to a spec violation if it actually
> runs,
> > because the condition that follows will be ignored. If it's guaranteed that
> the
> > following condition won't apply, maybe you can change the order and/or
> > ASSERT(networkState() != HTMLMediaElement::NETWORK_EMPTY) if that's why it's
> > safe?
> 
> I didn't exactly understand your query here. we will discuss directly when
> possible.
> Let me explain why have i have done the current changes.
> As per spec 
> Note: Dynamically modifying a source element and its attribute when the
element
> is already inserted in a video or audio element will have no effect. To change
> what is playing, just use the src attribute on the media element directly.
> 
> So as i understand even though current source is removed and new source is
> inserted, we should keep playing the current media as it has already started.
I
> have verified in firefox and the behaviour is same. In the DOM even though the
> current source is removed and new source is inserted still video.currentSrc
> points to current source element's src value.

Yes, that's all correct, but what I meant was that by adding an early return
before "if (networkState() == HTMLMediaElement::NETWORK_EMPTY)" it makes the
code look like that could be skipped in some situations where the spec doesn't.

[Srirama, 2014-06-30 14:13:54]

On 2014/06/30 11:35:51, philipj wrote:
> On 2014/06/28 06:23:04, Srirama wrote:
> > On 2014/06/27 22:06:14, philipj wrote:
> > > Sorry I'm late to the party, but I don't think I understand this patch.
Can
> > you
> > > describe how "HTMLMediaElement::webMediaPlayer() should never be null if
> > > m_readyState >= HAVE_METADATA" was previously violated?
> > > 
> > 
> > When a source element is playing its networks state will not be
NETWORK_EMPTY
> > and its readystate will be >= HAVE_METADATA. Now webMediaPlayer() is not
null
> > and is fine. But at this point if we remove the current source using js then
> > currentsource will be set to null in sourceaWasRemoved. At this point both
> > m_currentSourceNode and m_nextChildNodeToConsider are null and network state
> is
> > not NETWORK_EMPTY. Now if we add a new source element through js then it
skips
> > through all the conditions in sourceWasAdded and triggers
> > scheduleNextSourceChild(). This will finally call loadNextSourceChild()
which
> is
> > creating mediaplayer again. This will clear the webMediaPlayer() but the
> > readystate is still >= HAVE_METADATA. This is what i am tying to address
> through
> > the current patch.
> 
> Ah, thank you for this detailed analysis. It seems like the current code has
at
> least these bugs:
> 
> 1. In sourceWasRemoved, m_currentSourceNode is cleared, which apart from the
bug
> you've found means that no error event will be fired on it in case of a
network
> error.
> 

Yes, but when the source itself is removed do we still need to fire error
events?
I feel this will be a content issue rather than bug in code. Either content
author 
needs to remove the source before media play starts or he needs to change the
src 
attribute of video if he really intends to change what is currently playing as
per spec.
Please correct me if i am wrong.

> 2. In sourceWasAdded, the "m_currentSourceNode && source ==
> m_currentSourceNode->nextSibling()" condition looks broken, because it assumes
> that there is no non-source Node between m_currentSourceNode and source, which
> need not be the case.
> 

Yes, any node can be there between source elements but again i think, the code
is
written just to check whether the source insertion is next to the current source
or some where else. Generally insertions happen with respect to source elements,
but 
again if someone intentionally does insert after some element like <span> in
below piece 
of html code then your point is absolutely correct.

> 3. In sourceWasAdded, there's confusion about where in the resource selection
> algorithm we are. Even if the above were fixed, it still looks like it could
> fall through all of the conditions and mistakenly think that it's in the
waiting
> step of the resource selection algorithm. To me it looks like sourceWasAdded()
> should check m_loadState == WaitingForSource before jumping in to the resource
> selection algorithm steps, or at the very least ASSERT it if the other checks
> combined would imply it, which I think they currently do not.
> 
> As a quickfix, simply checking m_loadState == WaitingForSource would be an
> improvement. I'm not certain, but I think fixing bug 1 and 2 above could also
> fix the problem.
> 

yes, you are right, adding this check may avoid some of the issues.

> > > AFAICT, the code in PS5 is going amount to a spec violation if it actually
> > runs,
> > > because the condition that follows will be ignored. If it's guaranteed
that
> > the
> > > following condition won't apply, maybe you can change the order and/or
> > > ASSERT(networkState() != HTMLMediaElement::NETWORK_EMPTY) if that's why
it's
> > > safe?
> > 
> > I didn't exactly understand your query here. we will discuss directly when
> > possible.
> > Let me explain why have i have done the current changes.
> > As per spec 
> > Note: Dynamically modifying a source element and its attribute when the
> element
> > is already inserted in a video or audio element will have no effect. To
change
> > what is playing, just use the src attribute on the media element directly.
> > 
> > So as i understand even though current source is removed and new source is
> > inserted, we should keep playing the current media as it has already
started.
> I
> > have verified in firefox and the behaviour is same. In the DOM even though
the
> > current source is removed and new source is inserted still video.currentSrc
> > points to current source element's src value.
> 
> Yes, that's all correct, but what I meant was that by adding an early return
> before "if (networkState() == HTMLMediaElement::NETWORK_EMPTY)" it makes the
> code look like that could be skipped in some situations where the spec
doesn't.

ok.

Please let me know if i can land the current changes and track the 
above issues (after confirmation) as a separate bug.

[philipj_slow, 2014-06-30 21:43:26]

On 2014/06/30 14:13:54, Srirama wrote:
> On 2014/06/30 11:35:51, philipj wrote:
> > On 2014/06/28 06:23:04, Srirama wrote:
> > > On 2014/06/27 22:06:14, philipj wrote:
> > > > Sorry I'm late to the party, but I don't think I understand this patch.
> Can
> > > you
> > > > describe how "HTMLMediaElement::webMediaPlayer() should never be null if
> > > > m_readyState >= HAVE_METADATA" was previously violated?
> > > > 
> > > 
> > > When a source element is playing its networks state will not be
> NETWORK_EMPTY
> > > and its readystate will be >= HAVE_METADATA. Now webMediaPlayer() is not
> null
> > > and is fine. But at this point if we remove the current source using js
then
> > > currentsource will be set to null in sourceaWasRemoved. At this point both
> > > m_currentSourceNode and m_nextChildNodeToConsider are null and network
state
> > is
> > > not NETWORK_EMPTY. Now if we add a new source element through js then it
> skips
> > > through all the conditions in sourceWasAdded and triggers
> > > scheduleNextSourceChild(). This will finally call loadNextSourceChild()
> which
> > is
> > > creating mediaplayer again. This will clear the webMediaPlayer() but the
> > > readystate is still >= HAVE_METADATA. This is what i am tying to address
> > through
> > > the current patch.
> > 
> > Ah, thank you for this detailed analysis. It seems like the current code has
> at
> > least these bugs:
> > 
> > 1. In sourceWasRemoved, m_currentSourceNode is cleared, which apart from the
> bug
> > you've found means that no error event will be fired on it in case of a
> network
> > error.
> > 
> 
> Yes, but when the source itself is removed do we still need to fire error
> events?
> I feel this will be a content issue rather than bug in code. Either content
> author 
> needs to remove the source before media play starts or he needs to change the
> src 
> attribute of video if he really intends to change what is currently playing as
> per spec.
> Please correct me if i am wrong.

The spec is unambiguous about this: "Failed with elements: Queue a task to fire
a simple event named error at the candidate element." The candidate remains
unchanged even if it's removed from the document.

That being said, I think what the spec says is annoying and in fact Presto had
exactly the bug that Blink has here, but on balance I don't think it's worth
trying to change the spec. Here's a test for it if you're curious:
http://w3c-test.org/html/semantics/embedded-content/media-elements/loading-th...

> > 2. In sourceWasAdded, the "m_currentSourceNode && source ==
> > m_currentSourceNode->nextSibling()" condition looks broken, because it
assumes
> > that there is no non-source Node between m_currentSourceNode and source,
which
> > need not be the case.
> > 
> 
> Yes, any node can be there between source elements but again i think, the code
> is
> written just to check whether the source insertion is next to the current
source
> or some where else. Generally insertions happen with respect to source
elements,
> but 
> again if someone intentionally does insert after some element like <span> in
> below piece 
> of html code then your point is absolutely correct.

I tried out a fix for this earlier today:
https://codereview.chromium.org/353383003/

It passes existing tests, which means that there's no test coverage for the case
we're discussing. I'd have to add a test to land that CL.

> > 3. In sourceWasAdded, there's confusion about where in the resource
selection
> > algorithm we are. Even if the above were fixed, it still looks like it could
> > fall through all of the conditions and mistakenly think that it's in the
> waiting
> > step of the resource selection algorithm. To me it looks like
sourceWasAdded()
> > should check m_loadState == WaitingForSource before jumping in to the
resource
> > selection algorithm steps, or at the very least ASSERT it if the other
checks
> > combined would imply it, which I think they currently do not.
> > 
> > As a quickfix, simply checking m_loadState == WaitingForSource would be an
> > improvement. I'm not certain, but I think fixing bug 1 and 2 above could
also
> > fix the problem.
> > 
> 
> yes, you are right, adding this check may avoid some of the issues.
> 
> > > > AFAICT, the code in PS5 is going amount to a spec violation if it
actually
> > > runs,
> > > > because the condition that follows will be ignored. If it's guaranteed
> that
> > > the
> > > > following condition won't apply, maybe you can change the order and/or
> > > > ASSERT(networkState() != HTMLMediaElement::NETWORK_EMPTY) if that's why
> it's
> > > > safe?
> > > 
> > > I didn't exactly understand your query here. we will discuss directly when
> > > possible.
> > > Let me explain why have i have done the current changes.
> > > As per spec 
> > > Note: Dynamically modifying a source element and its attribute when the
> > element
> > > is already inserted in a video or audio element will have no effect. To
> change
> > > what is playing, just use the src attribute on the media element directly.
> > > 
> > > So as i understand even though current source is removed and new source is
> > > inserted, we should keep playing the current media as it has already
> started.
> > I
> > > have verified in firefox and the behaviour is same. In the DOM even though
> the
> > > current source is removed and new source is inserted still
video.currentSrc
> > > points to current source element's src value.
> > 
> > Yes, that's all correct, but what I meant was that by adding an early return
> > before "if (networkState() == HTMLMediaElement::NETWORK_EMPTY)" it makes the
> > code look like that could be skipped in some situations where the spec
> doesn't.
> 
> ok.

Oh my, I see I've been misreading the diff. You added the code before 4.8.9.5, I
thought it was before 4.8.8. The comment I made makes no sense!
 
> Please let me know if i can land the current changes and track the 
> above issues (after confirmation) as a separate bug.

As you've discovered, it's possible to reach these steps when both
m_currentSourceNode and m_nextChildNodeToConsider are null, and there's a case
where the resource selection steps shouldn't be run. However, after some
pondering I think I've found a case where they *should* be run, and wrote a test
that I've verified currently passes but fails with your change:
https://codereview.chromium.org/360103003/ (If you like you can add it to this
CL, otherwise I'll land it separately.)

I think a more conservative fix here would be "if (m_loadState !=
WaitingForSource) return", if that's enough to fix the problem.

[Srirama, 2014-07-01 02:55:00]

On 2014/06/30 21:43:26, philipj wrote:
> On 2014/06/30 14:13:54, Srirama wrote:
> > On 2014/06/30 11:35:51, philipj wrote:
> > > On 2014/06/28 06:23:04, Srirama wrote:
> > > > On 2014/06/27 22:06:14, philipj wrote:
> > > > > Sorry I'm late to the party, but I don't think I understand this
patch.
> > Can
> > > > you
> > > > > describe how "HTMLMediaElement::webMediaPlayer() should never be null
if
> > > > > m_readyState >= HAVE_METADATA" was previously violated?
> > > > > 
> > > > 
> > > > When a source element is playing its networks state will not be
> > NETWORK_EMPTY
> > > > and its readystate will be >= HAVE_METADATA. Now webMediaPlayer() is not
> > null
> > > > and is fine. But at this point if we remove the current source using js
> then
> > > > currentsource will be set to null in sourceaWasRemoved. At this point
both
> > > > m_currentSourceNode and m_nextChildNodeToConsider are null and network
> state
> > > is
> > > > not NETWORK_EMPTY. Now if we add a new source element through js then it
> > skips
> > > > through all the conditions in sourceWasAdded and triggers
> > > > scheduleNextSourceChild(). This will finally call loadNextSourceChild()
> > which
> > > is
> > > > creating mediaplayer again. This will clear the webMediaPlayer() but the
> > > > readystate is still >= HAVE_METADATA. This is what i am tying to address
> > > through
> > > > the current patch.
> > > 
> > > Ah, thank you for this detailed analysis. It seems like the current code
has
> > at
> > > least these bugs:
> > > 
> > > 1. In sourceWasRemoved, m_currentSourceNode is cleared, which apart from
the
> > bug
> > > you've found means that no error event will be fired on it in case of a
> > network
> > > error.
> > > 
> > 
> > Yes, but when the source itself is removed do we still need to fire error
> > events?
> > I feel this will be a content issue rather than bug in code. Either content
> > author 
> > needs to remove the source before media play starts or he needs to change
the
> > src 
> > attribute of video if he really intends to change what is currently playing
as
> > per spec.
> > Please correct me if i am wrong.
> 
> The spec is unambiguous about this: "Failed with elements: Queue a task to
fire
> a simple event named error at the candidate element." The candidate remains
> unchanged even if it's removed from the document.
> 
> That being said, I think what the spec says is annoying and in fact Presto had
> exactly the bug that Blink has here, but on balance I don't think it's worth
> trying to change the spec. Here's a test for it if you're curious:
>
http://w3c-test.org/html/semantics/embedded-content/media-elements/loading-th...
> 
> > > 2. In sourceWasAdded, the "m_currentSourceNode && source ==
> > > m_currentSourceNode->nextSibling()" condition looks broken, because it
> assumes
> > > that there is no non-source Node between m_currentSourceNode and source,
> which
> > > need not be the case.
> > > 
> > 
> > Yes, any node can be there between source elements but again i think, the
code
> > is
> > written just to check whether the source insertion is next to the current
> source
> > or some where else. Generally insertions happen with respect to source
> elements,
> > but 
> > again if someone intentionally does insert after some element like <span> in
> > below piece 
> > of html code then your point is absolutely correct.
> 
> I tried out a fix for this earlier today:
> https://codereview.chromium.org/353383003/
> 
> It passes existing tests, which means that there's no test coverage for the
case
> we're discussing. I'd have to add a test to land that CL.
> 
> > > 3. In sourceWasAdded, there's confusion about where in the resource
> selection
> > > algorithm we are. Even if the above were fixed, it still looks like it
could
> > > fall through all of the conditions and mistakenly think that it's in the
> > waiting
> > > step of the resource selection algorithm. To me it looks like
> sourceWasAdded()
> > > should check m_loadState == WaitingForSource before jumping in to the
> resource
> > > selection algorithm steps, or at the very least ASSERT it if the other
> checks
> > > combined would imply it, which I think they currently do not.
> > > 
> > > As a quickfix, simply checking m_loadState == WaitingForSource would be an
> > > improvement. I'm not certain, but I think fixing bug 1 and 2 above could
> also
> > > fix the problem.
> > > 
> > 
> > yes, you are right, adding this check may avoid some of the issues.
> > 
> > > > > AFAICT, the code in PS5 is going amount to a spec violation if it
> actually
> > > > runs,
> > > > > because the condition that follows will be ignored. If it's guaranteed
> > that
> > > > the
> > > > > following condition won't apply, maybe you can change the order and/or
> > > > > ASSERT(networkState() != HTMLMediaElement::NETWORK_EMPTY) if that's
why
> > it's
> > > > > safe?
> > > > 
> > > > I didn't exactly understand your query here. we will discuss directly
when
> > > > possible.
> > > > Let me explain why have i have done the current changes.
> > > > As per spec 
> > > > Note: Dynamically modifying a source element and its attribute when the
> > > element
> > > > is already inserted in a video or audio element will have no effect. To
> > change
> > > > what is playing, just use the src attribute on the media element
directly.
> > > > 
> > > > So as i understand even though current source is removed and new source
is
> > > > inserted, we should keep playing the current media as it has already
> > started.
> > > I
> > > > have verified in firefox and the behaviour is same. In the DOM even
though
> > the
> > > > current source is removed and new source is inserted still
> video.currentSrc
> > > > points to current source element's src value.
> > > 
> > > Yes, that's all correct, but what I meant was that by adding an early
return
> > > before "if (networkState() == HTMLMediaElement::NETWORK_EMPTY)" it makes
the
> > > code look like that could be skipped in some situations where the spec
> > doesn't.
> > 
> > ok.
> 
> Oh my, I see I've been misreading the diff. You added the code before 4.8.9.5,
I
> thought it was before 4.8.8. The comment I made makes no sense!
>  
> > Please let me know if i can land the current changes and track the 
> > above issues (after confirmation) as a separate bug.
> 
> As you've discovered, it's possible to reach these steps when both
> m_currentSourceNode and m_nextChildNodeToConsider are null, and there's a case
> where the resource selection steps shouldn't be run. However, after some
> pondering I think I've found a case where they *should* be run, and wrote a
test
> that I've verified currently passes but fails with your change:
> https://codereview.chromium.org/360103003/ (If you like you can add it to this
> CL, otherwise I'll land it separately.)
> 
> I think a more conservative fix here would be "if (m_loadState !=
> WaitingForSource) return", if that's enough to fix the problem.

Thanks for the verification, i will try with the above change.

[Srirama, 2014-07-01 10:52:39]

The suggested changes looks fine. All the tests are passing.
PTAL.

[philipj_slow, 2014-07-01 21:37:59]

LGTM!

[Srirama, 2014-07-02 02:38:44]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2014-07-02 02:39:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/srirama.m@samsung.com/349973006/180001

[commit-bot: I haz the power, 2014-07-02 06:59:54]

Change committed as 177353