1303970 - Intermittent dom/media/test/test_audio2.html | Assertion count 1 is greater than expected range 0-0 assertions.

Status: RESOLVED FIXED

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

Description

[Treeherder Bug Filer]

Filed by: tomcat [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=36109988&repo=mozilla-inbound

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-macosx64-debug/1474337956/mozilla-inbound_yosemite_r7-debug_test-mochitest-media-e10s-bm133-tests1-macosx-build115.txt.gz

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1303970: P1. MEDIA_ERR_ABORTED can be returned even if readyState is HAVE_NOTHING.

So change assertion accordingly.

Review commit: https://reviewboard.mozilla.org/r/79722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79722/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1303970: P2. Fix "If the media data fetching process is aborted by the user steps".

Review commit: https://reviewboard.mozilla.org/r/79726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79726/

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8792830 [details]
Bug 1303970: P1. MEDIA_ERR_ABORTED can be returned even if readyState is HAVE_NOTHING.

https://reviewboard.mozilla.org/r/79722/#review78700

Can you explain the root cause of this issue?

Comment 4

[Jean-Yves Avenard [:jya]]

you can see what's happening on the stack trace provided in the bug description

 20:41:43     INFO -  [Child 1774] ###!!! ASSERTION: Shouldn't be called when readyState is HAVE_NOTHING: 'mReadyState > HAVE_NOTHING', file /builds/slave/m-in-m64-d-0000000000000000000/build/src/dom/html/HTMLMediaElement.cpp, line 4471
 20:42:24     INFO -  #01: non-virtual thunk to mozilla::dom::HTMLMediaElement::LoadAborted() [xpcom/string/nsTSubstring.h:95]
 20:42:24     INFO -  #02: mozilla::detail::RunnableFunction<mozilla::MediaDecoder::ResourceCallback::NotifyDataEnded(nsresult)::$_2>::Run() [dom/media/MediaDecoder.cpp:251]

MediaDecoder::ResourceCallback::NotifyDataEnded calls LoadAborted() which calls Error()

HTMLMEdiaElement::Error now expects readyState to be >= HAVE_METADATA which isn't always true.

Additionally, the handling of abort was wrong anyway and the required events wasn't fired (that's what P2 fixes)

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8792831 [details]
Bug 1303970: P2. Fix "If the media data fetching process is aborted by the user steps".

https://reviewboard.mozilla.org/r/79726/#review78706

::: dom/html/HTMLMediaElement.cpp:4489
(Diff revision 1)
>      return;
>    }
>    mError = new MediaError(this, aErrorCode, aErrorDetails);
>  
>    DispatchAsyncEvent(NS_LITERAL_STRING("error"));
> +  if (aErrorCode != MEDIA_ERR_ABORTED) {

This will prevent Error(MEDIA_ERR_ABORTED) from chaning networkState to NETWORK_IDLE when readyState > HAVE_NOTHING.

I think we should move the statement of #4461 into this function so we have only one place for error code handling (involing checking readyState and changing networkState).

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8792830 [details]
Bug 1303970: P1. MEDIA_ERR_ABORTED can be returned even if readyState is HAVE_NOTHING.

https://reviewboard.mozilla.org/r/79722/#review78710

::: dom/html/HTMLMediaElement.cpp:4471
(Diff revision 1)
>    NS_ASSERTION(aErrorCode == MEDIA_ERR_DECODE ||
>                 aErrorCode == MEDIA_ERR_NETWORK ||
>                 aErrorCode == MEDIA_ERR_ABORTED,
>                 "Only use MediaError codes!");
> -  NS_ASSERTION(mReadyState > HAVE_NOTHING,
> +  NS_ASSERTION(mReadyState > HAVE_NOTHING || aErrorCode == MEDIA_ERR_ABORTED,
>                 "Shouldn't be called when readyState is HAVE_NOTHING");

Fix the comment or just remove it which is not very informative.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8792831 [details]
Bug 1303970: P2. Fix "If the media data fetching process is aborted by the user steps".

https://reviewboard.mozilla.org/r/79726/#review78706

> This will prevent Error(MEDIA_ERR_ABORTED) from chaning networkState to NETWORK_IDLE when readyState > HAVE_NOTHING.
> 
> I think we should move the statement of #4461 into this function so we have only one place for error code handling (involing checking readyState and changing networkState).

done

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8792830 [details]
Bug 1303970: P1. MEDIA_ERR_ABORTED can be returned even if readyState is HAVE_NOTHING.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79722/diff/1-2/

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8792831 [details]
Bug 1303970: P2. Fix "If the media data fetching process is aborted by the user steps".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79726/diff/1-2/

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8792831 [details]
Bug 1303970: P2. Fix "If the media data fetching process is aborted by the user steps".

https://reviewboard.mozilla.org/r/79726/#review78714

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8792830 [details]
Bug 1303970: P1. MEDIA_ERR_ABORTED can be returned even if readyState is HAVE_NOTHING.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79722/diff/2-3/

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8792831 [details]
Bug 1303970: P2. Fix "If the media data fetching process is aborted by the user steps".

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79726/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/673807c7e76f
P1. MEDIA_ERR_ABORTED can be returned even if readyState is HAVE_NOTHING. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/e2e14420350f
P2. Fix "If the media data fetching process is aborted by the user steps". r=jwwang

Comment 14

[Jean-Yves Avenard [:jya]]

Patch P2 fixes a spec bug that got introduced in 51. It deserves a higher priority.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/673807c7e76f
https://hg.mozilla.org/mozilla-central/rev/e2e14420350f

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8792830 [details]
Bug 1303970: P1. MEDIA_ERR_ABORTED can be returned even if readyState is HAVE_NOTHING.

Approval Request Comment
[Feature/regressing bug #]: 1302632
[User impact if declined]: Assertions, incorrect events being fired making us non HTML5 compliant
[Describe test coverage new/current, TreeHerder]: In central, manual test. 
[Risks and why]: Low. We have removed an assertion and re-enabled the firing of the proper event when the user abort playback.
[String/UUID change made/needed]: None

Comment 17

[Gerry Chang [:gchang]]

Hi :jya,
Just for my clarification. Per comment #14, is the patch 2 included in the uplift request?

Comment 18

[Jean-Yves Avenard [:jya]]

(In reply to Gerry Chang [:gchang] from comment #17)
> Hi :jya,
> Just for my clarification. Per comment #14, is the patch 2 included in the
> uplift request?

yes... sorry for not indicating that earlier (I thought I did)

Comment 19

[Gerry Chang [:gchang]]

Hi :jya,
Do you need to create another uplift request for patch2?

Comment 20

[Jean-Yves Avenard [:jya]]

I don't no. The comment and description in the uplift request already mentioned the changes added by those two patches.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8792830 [details]
Bug 1303970: P1. MEDIA_ERR_ABORTED can be returned even if readyState is HAVE_NOTHING.

The patch fixes a intermittent-failure. Take it in 51 aurora for both patches.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f48be812c113
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ce626bad1bf