Closed Bug 1301341 Opened 5 years ago Closed 5 years ago

Move some code to DecodeMetadataState

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

We are moving more and more code to state objects to improve the cohesion and responsibility of each state object.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8789301 - Flags: review?(kaku)
Attachment #8789302 - Flags: review?(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.
Attachment #8789301 - Flags: review?(kaku) → review+
Comment on attachment 8789302 [details]
Bug 1301341. Part 2 - Remove unused members.

https://reviewboard.mozilla.org/r/77578/#review76116
Attachment #8789302 - Flags: review?(kaku) → review+
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.
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/4197be267c95
https://hg.mozilla.org/mozilla-central/rev/50d2423ae823
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.