1312594 - src/currentSrc is cleared when an error occurred

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Attempting to troubleshoot bug 1311876.

One issue is that when the error event is fired, the element's src is cleared ; making it impossible to tell which URL caused the problem.

Adding the URL to the resource that caused the error would help.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1312594: Do not clear resource when error occurs.

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

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8804098 [details]
Bug 1312594: Do not clear resource when error occurs.

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

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8804098 [details]
Bug 1312594: Do not clear resource when error occurs.

https://reviewboard.mozilla.org/r/88242/#review87202

I know you are still working on this, so here are my review comments so far, so you may fix them in the next version.

::: dom/html/HTMLMediaElement.cpp:4444
(Diff revision 2)
>    if (mError) {
>      return;
>    }
> -  mError = new MediaError(this, aErrorCode, aErrorDetails);
>  
> +  // Get currentSrc

This comment is redundant I think.
Or did you intend to write more, e.g.: "Add currentSrc (if present) to error details"?

::: dom/html/HTMLMediaElement.cpp:4448
(Diff revision 2)
> +                          src.Length()
> +                            ? (src + NS_LITERAL_CSTRING(":") + aErrorDetails)
> +                            : nsCString(aErrorDetails));

Style: left-align '?' and ':' with 'src.Length()' above.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8804098 [details]
Bug 1312594: Do not clear resource when error occurs.

https://reviewboard.mozilla.org/r/88242/#review87202

> Style: left-align '?' and ':' with 'src.Length()' above.

I have taken the decision to ignore this coding-style "rule" from now on for two reasons:
1- I find it particularly hard to read, as it's not obvious at a glance that the block is related to the test above
2- clang-format in Mozilla mode adds an indentation. As my IDE reformat things as I type or when I copy/paste having to manually go against clang-format is a pain.

I could add a 3 in that no-one seems to follow the rule when it comes to test ? a : b, may as well change the rule.
It's more often than not written like throughout gecko code:
A ? B
  : C

Comment 5

[Jean-Yves Avenard [:jya]]

Changing the title of this bug as it's now clear to me that this is an invalid implementation of the spec.

When an error occurs, we call void HTMLMediaElement::DecodeError(const MediaResult& aError)
which https://dxr.mozilla.org/mozilla-central/rev/215f9686117673a2c914ed207bc7da9bb8d741ad/dom/html/HTMLMediaElement.cpp#4400

will clear mLoadingSrc
as such, currentSrc is cleared.

per spec, media.currentSrc is never cleared, except when the object is created.
Otherwise, it's only ever set:
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-load-algorithm

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8804098 [details]
> https://reviewboard.mozilla.org/r/88242/#review87202
> > Style: left-align '?' and ':' with 'src.Length()' above.
> I have taken the decision to ignore this coding-style "rule" from now on for
> two reasons: [...]

And I'll keep reporting this in reviews, as our team's "Vision and Culture" document says we should "Enforce Mozilla coding style".
(And I personally prefer the left-aligned style, it's consistent with binary operators.)

But it's true the operator-related guidelines are probably the least-followed ones, it's quite a mess out there. :-(
Maybe you should start discussing changing the guidelines, and/or using auto-formatting tools, in mozilla.dev.platform?

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8804098 [details]
Bug 1312594: Do not clear resource when error occurs.

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

Comment 8

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

Comment on attachment 8804098 [details]
Bug 1312594: Do not clear resource when error occurs.

https://reviewboard.mozilla.org/r/88242/#review87230

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99efd9af4a01
Do not clear resource when error occurs. r=jwwang

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/99efd9af4a01