Use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived()

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

No description provided.
Assignee: nobody → jwwang
Blocks: 1355409
Priority: -- → P3
Attachment #8925411 - Flags: review?(bechen)
Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/196532/#review201746
Attachment #8925411 - Flags: review?(bechen) → review+
Attachment #8925411 - Flags: review?(gsquelart)
Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/196532/#review201766

::: dom/media/ChannelMediaResource.cpp:443
(Diff revision 1)
>  {
>    Closure* closure = static_cast<Closure*>(aClosure);
>    closure->mResource->mCacheStream.NotifyDataReceived(
> -    closure->mLoadID, aCount, aFromSegment);
> +    closure->mLoadID,
> +    aCount,
> +    reinterpret_cast<const unsigned char*>(aFromSegment));

'unsigned char' -> 'uint8_t', to match the NotifyDataReceived argument.
Attachment #8925411 - Flags: review?(gsquelart) → review+
Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/196532/#review201766

> 'unsigned char' -> 'uint8_t', to match the NotifyDataReceived argument.

That was done intentionally. AFAIK, uint8_t doesn't always equal 'unsigned char' on all platforms and casting char* to uint8_t* will break strict aliasing if sizeof(uint8_t) != sizeof(char).
I guess strict aliasing is not an issue here since we never dereference |aData|.
Comment on attachment 8925411 [details]
Bug 1414709 - use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived().

https://reviewboard.mozilla.org/r/196532/#review201766

> That was done intentionally. AFAIK, uint8_t doesn't always equal 'unsigned char' on all platforms and casting char* to uint8_t* will break strict aliasing if sizeof(uint8_t) != sizeof(char).

I thought aliasing between any type and `char` was supported?
And then you call a function that takes `const uint8_t*`, so there's still a potential cast from `const unsigned char*` to `const uint8_t*` (if they are in fact different).
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5915590f9500
use Span<> to replace low level pointer arithmetic in MediaCacheStream::NotifyDataReceived(). r=bechen,gerald
https://hg.mozilla.org/mozilla-central/rev/5915590f9500
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.