We never pass anything other than NS_SEEK_SET to MediaCacheStream::Seek()

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

Assignee

Updated

2 years ago
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Assignee

Updated

2 years ago
Attachment #8926684 - Flags: review?(bechen)
Attachment #8926685 - Flags: review?(bechen)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8926684 [details]
Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

https://reviewboard.mozilla.org/r/197926/#review203114

::: dom/media/MediaCache.cpp:31
(Diff revision 1)
>  #include "nsIPrincipal.h"
> -#include "nsISeekableStream.h"
>  #include "nsPrintfCString.h"
>  #include "nsProxyRelease.h"
>  #include "nsThreadUtils.h"
>  #include "prio.h"

Could we also remove the prio.h?
Attachment #8926684 - Flags: review?(bechen) → review+

Comment 4

2 years ago
mozreview-review
Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

https://reviewboard.mozilla.org/r/197928/#review203118
Attachment #8926685 - Flags: review?(bechen) → review+
Assignee

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8926684 [details]
Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

https://reviewboard.mozilla.org/r/197926/#review203114

> Could we also remove the prio.h?

I will do that in a new bug.
Assignee

Updated

2 years ago
Attachment #8926684 - Flags: review?(gsquelart)
Attachment #8926685 - Flags: review?(gsquelart)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8926684 [details]
Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

https://reviewboard.mozilla.org/r/197926/#review203126

LGTM.
Note that this is similar to bug 1374173, which was backed out for intermittent failures! But it was probably because of the other part of that bug, so I think this one should be fine; but please keep an eye on build results.

::: dom/media/MediaCache.cpp:2425
(Diff revision 1)
> -  default:
> -    NS_ERROR("Unknown whence");
>      return NS_ERROR_FAILURE;
>    }
> -
> +  int64_t oldOffset = mStreamOffset;
> +  int64_t newOffset = aOffset;

You don't really need `newOffset` anymore, you could just use `aOffset` throughout.

::: dom/media/MediaCache.cpp:2426
(Diff revision 1)
>    if (!IsOffsetAllowed(newOffset)) {
>      return NS_ERROR_FAILURE;
>    }

Minor optimization: Move this test&return before taking the monitor.
Attachment #8926684 - Flags: review?(gsquelart) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

https://reviewboard.mozilla.org/r/197928/#review203132

::: dom/media/MediaCache.cpp:2421
(Diff revision 1)
>  MediaCacheStream::Seek(int64_t aOffset)
>  {
> -  NS_ASSERTION(!NS_IsMainThread(), "Don't call on main thread");
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  mMediaCache->GetReentrantMonitor().AssertCurrentThreadIn();
>  
> -  ReentrantMonitorAutoEnter mon(mMediaCache->GetReentrantMonitor());
> +  if (mClosed || !IsOffsetAllowed(aOffset)) {

As per my comment in the previous patch, the `IsOffsetAllowed` test could be done before taking the monitor. But it's very minor, so totally optional.
Attachment #8926685 - Flags: review?(gsquelart) → review+

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8926684 [details]
Bug 1415766. P1 - we never pass anything other than NS_SEEK_SET to Seek().

https://reviewboard.mozilla.org/r/197926/#review203126

> You don't really need `newOffset` anymore, you could just use `aOffset` throughout.

I see you've done that in the next patch, so nevermind. :-)
Assignee

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

https://reviewboard.mozilla.org/r/197928/#review203132

> As per my comment in the previous patch, the `IsOffsetAllowed` test could be done before taking the monitor. But it's very minor, so totally optional.

#2419 now asserts the monitor is already taken without taking it.
Assignee

Comment 10

2 years ago
Thanks for the reviews!

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8926685 [details]
Bug 1415766. P2 - move Seek() to private and tighten up some assertions.

https://reviewboard.mozilla.org/r/197928/#review203132

> #2419 now asserts the monitor is already taken without taking it.

Oops, didn't notice, sorry. All good, then!
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/753f3591930d
P1 - we never pass anything other than NS_SEEK_SET to Seek(). r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/b12b1f6beb49
P2 - move Seek() to private and tighten up some assertions. r=bechen,gerald

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/753f3591930d
https://hg.mozilla.org/mozilla-central/rev/b12b1f6beb49
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.