1290028 - Remove the check for IsShutdown() from MediaDecoder::OwnerHasError()

Status: RESOLVED FIXED

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

Description

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

https://hg.mozilla.org/mozilla-central/file/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/media/mediasource/SourceBuffer.cpp#l499

|mMediaSource->GetDecoder() == nullptr| covers the shutdown case. We can remove the IsShutdown() check so the meaning of MediaDecoder::OwnerHasError() is less confusing.

Comment 1

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

Attachment: Bug 1290028 - Remove the check for IsShutdown() from MediaDecoder::OwnerHasError().

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

Comment 2

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

Comment on attachment 8776815 [details]
Bug 1290028 - Remove the check for IsShutdown() from MediaDecoder::OwnerHasError().

https://reviewboard.mozilla.org/r/68484/#review65566

r+ with a bit more documentation and sanity checking:

::: dom/media/MediaDecoder.h:242
(Diff revision 1)
>    // Return true if the MediaDecoderOwner's error attribute is not null.
> -  // If the MediaDecoder is shutting down, OwnerHasError will return true.
>    bool OwnerHasError() const;

...

::: dom/media/MediaDecoder.cpp:1067
(Diff revision 1)
>  
>  bool
>  MediaDecoder::OwnerHasError() const
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  return IsShutdown() || mOwner->HasError();
> +  return GetOwner()->HasError();

I see why you can do this here: IsShutdown() is implictly checked by the caller before calling OwnerHasError(), meaning GetOwner() cannot return a nullptr.
However that is not obvious, and from just glancing at this code fragment it seems you just blindly dereference a pointer without checking it!
So I think it would be worth:
- Documenting that OwnerHasError should only be called when we're not in shutdown.
- Adding a MOZ_ASSERT(GetOwner()) to catch misuses in the future, without sacrificing releases' speed.

Thinking about it a bit more: Since you assume we are not in shutdown, why do you even call GetOwner() instead of just using mOwner? This would save an unnecessary IsShutdown() call.
(In this case the MOZ_ASSERT could just check for !IsShutdown().)

Comment 3

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

https://reviewboard.mozilla.org/r/68484/#review65566

> I see why you can do this here: IsShutdown() is implictly checked by the caller before calling OwnerHasError(), meaning GetOwner() cannot return a nullptr.
> However that is not obvious, and from just glancing at this code fragment it seems you just blindly dereference a pointer without checking it!
> So I think it would be worth:
> - Documenting that OwnerHasError should only be called when we're not in shutdown.
> - Adding a MOZ_ASSERT(GetOwner()) to catch misuses in the future, without sacrificing releases' speed.
> 
> Thinking about it a bit more: Since you assume we are not in shutdown, why do you even call GetOwner() instead of just using mOwner? This would save an unnecessary IsShutdown() call.
> (In this case the MOZ_ASSERT could just check for !IsShutdown().)

Thanks! I will address the review comments by:
1. Assert !IsShutdown() to make it obvious that this function should be only called before Shutdown().
2. replace GetOwner() with mOwner which is more efficient.

Comment 4

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

Comment on attachment 8776815 [details]
Bug 1290028 - Remove the check for IsShutdown() from MediaDecoder::OwnerHasError().

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1877c26c7e86
Remove the check for IsShutdown() from MediaDecoder::OwnerHasError(). r=gerald

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1877c26c7e86