1301341 - Move some code to DecodeMetadataState

Status: RESOLVED FIXED

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

Description

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

We are moving more and more code to state objects to improve the cohesion and responsibility of each state object.

Comment 1

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

Attachment: Bug 1301341. Part 1 - Move some code to DecodeMetadataState.

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

Comment 2

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

Attachment: Bug 1301341. Part 2 - Remove unused members.

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

Comment 3

[Tzuhao Kuo [:kaku]]

Comment on attachment 8789301 [details]
Bug 1301341. Part 1 - Move some code to DecodeMetadataState.

https://reviewboard.mozilla.org/r/77576/#review75914

::: dom/media/MediaDecoderStateMachine.cpp:294
(Diff revision 1)
> +    mMetadataRequest.Complete();
> +
> +    if (mPendingDormant) {
> +      // No need to store mQueuedSeek because we are at position 0.
> +      SetState(DECODER_STATE_DORMANT);
> +      return;

Is it because that this object won't be reused again so that you don't reset the mPendingDormant before returning? If that case, how about we at least have a comment here?

::: dom/media/MediaDecoderStateMachine.cpp:314
(Diff revision 1)
> +          NS_ENSURE_TRUE_VOID(!master->IsShutdown());
> +          TimeUnit unadjusted = master->mInfo.mUnadjustedMetadataEndTime.ref();
> +          TimeUnit adjustment = master->mReader->StartTime();
> +          master->mInfo.mMetadataDuration.emplace(unadjusted - adjustment);
> +          master->RecomputeDuration();
> +        }, [] () { NS_WARNING("Adjusting metadata end time failed"); }

I think you would like to use SWARN here.

Comment 4

[Tzuhao Kuo [:kaku]]

Comment on attachment 8789302 [details]
Bug 1301341. Part 2 - Remove unused members.

https://reviewboard.mozilla.org/r/77578/#review76116

Comment 5

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

Comment on attachment 8789301 [details]
Bug 1301341. Part 1 - Move some code to DecodeMetadataState.

https://reviewboard.mozilla.org/r/77576/#review76152

::: dom/media/MediaDecoderStateMachine.cpp:294
(Diff revision 1)
> +    mMetadataRequest.Complete();
> +
> +    if (mPendingDormant) {
> +      // No need to store mQueuedSeek because we are at position 0.
> +      SetState(DECODER_STATE_DORMANT);
> +      return;

Yes. See the comment of SetState():

// Note this function will delete the current state object. Don't access members to avoid UAF after this call.

Accessing mPendingDormant after SetState() will cause UAF.

Comment 6

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

Comment on attachment 8789301 [details]
Bug 1301341. Part 1 - Move some code to DecodeMetadataState.

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

Comment 7

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

Comment on attachment 8789302 [details]
Bug 1301341. Part 2 - Remove unused members.

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

Comment 8

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

Thanks for the review!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4197be267c95
Part 1 - Move some code to DecodeMetadataState. r=kaku
https://hg.mozilla.org/integration/autoland/rev/50d2423ae823
Part 2 - Remove unused members. r=kaku

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4197be267c95
https://hg.mozilla.org/mozilla-central/rev/50d2423ae823