1378518 - InvalidArrayIndex_CRASH [@ mozilla::MediaCache::NoteSeek]

Reporter

Description

•

7 years ago

Attached video test_case.webm — Details

To reproduce this issue test_case.html and test_case.webm *MUST* be served from a web server not the filesystem.

==4088==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x00000051b706 bp 0x7f498268aa80 sp 0x7f498268a920 T106)
==4088==The signal is caused by a WRITE memory access.
==4088==Hint: address points to the zero page.
    #0 0x51b705 in MOZ_CrashPrintf mfbt/Assertions.cpp:63:3
    #1 0x7f49aa04aa7f in InvalidArrayIndex_CRASH(unsigned long, unsigned long) xpcom/ds/nsTArray.cpp:26:3
    #2 0x7f49aeff30e5 in ElementAt obj-firefox/dist/include/nsTArray.h:1048:7
    #3 0x7f49aeff30e5 in operator[] obj-firefox/dist/include/nsTArray.h:1086
    #4 0x7f49aeff30e5 in mozilla::MediaCache::NoteSeek(mozilla::MediaCacheStream*, long) dom/media/MediaCache.cpp:1778
    #5 0x7f49aeff72e3 in mozilla::MediaCacheStream::Seek(int, long) dom/media/MediaCache.cpp:2278:16
    #6 0x7f49aeff8c10 in mozilla::MediaCacheStream::ReadAt(long, char*, unsigned int, unsigned int*) dom/media/MediaCache.cpp:2422:17
    #7 0x7f49af1e104d in mozilla::ChannelMediaResource::ReadAt(long, char*, unsigned int, unsigned int*) dom/media/MediaResource.cpp:730:30
    #8 0x7f49af1e6d28 in UncachedReadAt dom/media/MediaResource.cpp:1916:32
    #9 0x7f49af1e6d28 in mozilla::MediaResourceIndex::CacheOrReadAt(long, char*, unsigned int, unsigned int*) dom/media/MediaResource.cpp:1885
    #10 0x7f49af1e60c3 in mozilla::MediaResourceIndex::ReadAt(long, char*, unsigned int, unsigned int*) dom/media/MediaResource.cpp:1758:10
    #11 0x7f49af1e5336 in mozilla::MediaResourceIndex::Read(char*, unsigned int, unsigned int*) dom/media/MediaResource.cpp:1568:17
    #12 0x7f49af715b26 in mozilla::webmdemux_read(void*, unsigned long, void*) dom/media/webm/WebMDemuxer.cpp:75:29
    #13 0x7f49b243f5d6 in ne_io_read media/libnestegg/src/nestegg.c:613:10
    #14 0x7f49b243f5d6 in ne_bare_read_vint media/libnestegg/src/nestegg.c:654
    #15 0x7f49b24389da in ne_read_id media/libnestegg/src/nestegg.c:686:10
    #16 0x7f49b24389da in ne_peek_element media/libnestegg/src/nestegg.c:965
    #17 0x7f49b24389da in ne_read_element media/libnestegg/src/nestegg.c:988
    #18 0x7f49b24389da in nestegg_read_packet media/libnestegg/src/nestegg.c:2744
    #19 0x7f49af722a03 in mozilla::WebMDemuxer::DemuxPacket(mozilla::TrackInfo::TrackType, RefPtr<mozilla::NesteggPacketHolder>&) dom/media/webm/WebMDemuxer.cpp:910:11
    #20 0x7f49af7225c3 in mozilla::WebMDemuxer::NextPacket(mozilla::TrackInfo::TrackType, RefPtr<mozilla::NesteggPacketHolder>&) dom/media/webm/WebMDemuxer.cpp:890:19
    #21 0x7f49af71d230 in mozilla::WebMDemuxer::GetNextPacket(mozilla::TrackInfo::TrackType, mozilla::MediaRawDataQueue*) dom/media/webm/WebMDemuxer.cpp:580:17
    #22 0x7f49af728363 in mozilla::WebMTrackDemuxer::NextSample(RefPtr<mozilla::MediaRawData>&) dom/media/webm/WebMDemuxer.cpp:1116:10
    #23 0x7f49af726c47 in mozilla::WebMTrackDemuxer::SetNextKeyFrameTime() dom/media/webm/WebMDemuxer.cpp:1184:28
    #24 0x7f49af7297f9 in mozilla::WebMTrackDemuxer::Reset() dom/media/webm/WebMDemuxer.cpp:1224:5
    #25 0x7f49af198c2a in operator() dom/media/MediaFormatReader.cpp:931:62
    #26 0x7f49af198c2a in mozilla::detail::RunnableFunction<mozilla::MediaFormatReader::DemuxerProxy::Wrapper::Reset()::{lambda()#1}>::Run() obj-firefox/dist/include/nsThreadUtils.h:507
    #27 0x7f49aa0f9584 in mozilla::TaskQueue::Runner::Run() xpcom/threads/TaskQueue.cpp:246:12
    #28 0x7f49aa12ac48 in nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp:225:14
    #29 0x7f49aa12b38c in non-virtual thunk to nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp:154:15
    #30 0x7f49aa12122a in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1437:14
    #31 0x7f49aa1273c8 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:489:10
    #32 0x7f49aaf0ab00 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:339:20
    #33 0x7f49aae66150 in RunInternal ipc/chromium/src/base/message_loop.cc:320:10
    #34 0x7f49aae66150 in RunHandler ipc/chromium/src/base/message_loop.cc:313
    #35 0x7f49aae66150 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:293
    #36 0x7f49aa118eed in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:506:11
    #37 0x7f49c43ad423 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:216:5
    #38 0x7f49c79c26b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
    #39 0x7f49c6a323dc in clone /build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV mfbt/Assertions.cpp:63:3 in MOZ_CrashPrintf
Thread T106 (MediaPD~oder #2) created by T100 (MediaPl~back #1) here:
    #0 0x4a3dc6 in __interceptor_pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:245:3
    #1 0x7f49c43aa1c9 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:457:14
    #2 0x7f49c43a9dde in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:548:12
    #3 0x7f49aa11b44e in nsThread::Init(nsACString const&) xpcom/threads/nsThread.cpp:688:8
    #4 0x7f49aa12657f in nsThreadManager::NewNamedThread(nsACString const&, unsigned int, nsIThread**) xpcom/threads/nsThreadManager.cpp:273:22
    #5 0x7f49aa1293fb in NS_NewNamedThread xpcom/threads/nsThreadUtils.cpp:113:45
    #6 0x7f49aa1293fb in nsThreadPool::PutEvent(already_AddRefed<nsIRunnable>, unsigned int) xpcom/threads/nsThreadPool.cpp:107
    #7 0x7f49aa12b689 in nsThreadPool::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) xpcom/threads/nsThreadPool.cpp:274:5
    #8 0x7f49aa101fd1 in mozilla::SharedThreadPool::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) obj-firefox/dist/include/mozilla/SharedThreadPool.h:71:68
    #9 0x7f49aa0f82fe in mozilla::TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>&, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) xpcom/threads/TaskQueue.cpp:128:26
    #10 0x7f49aa102df2 in mozilla::TaskQueue::Dispatch(already_AddRefed<nsIRunnable>, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) obj-firefox/dist/include/mozilla/TaskQueue.h:71:21
    #11 0x7f49af17355e in mozilla::AutoTaskQueue::Dispatch(already_AddRefed<nsIRunnable>, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) obj-firefox/dist/include/AutoTaskQueue.h:43:17
    #12 0x7f49af18f88e in mozilla::MediaFormatReader::DemuxerProxy::Wrapper::Reset() dom/media/MediaFormatReader.cpp:929:17
    #13 0x7f49af12c12e in mozilla::MediaFormatReader::DecoderData::ResetDemuxer() dom/media/MediaFormatReader.h:335:22
    #14 0x7f49af147748 in mozilla::MediaFormatReader::ResetDecode(mozilla::EnumSet<mozilla::TrackInfo::TrackType>) dom/media/MediaFormatReader.cpp:2537:12
    #15 0x7f49af0cd0ab in applyImpl<mozilla::MediaDecoderReader, nsresult (mozilla::MediaDecoderReader::*)(mozilla::EnumSet<mozilla::TrackInfo::TrackType>), StoreCopyPassByConstLRef<mozilla::EnumSet<mozilla::TrackInfo::TrackType> > , 0> obj-firefox/dist/include/nsThreadUtils.h:1138:12
    #16 0x7f49af0cd0ab in apply<mozilla::MediaDecoderReader, nsresult (mozilla::MediaDecoderReader::*)(mozilla::EnumSet<mozilla::TrackInfo::TrackType>)> obj-firefox/dist/include/nsThreadUtils.h:1144
    #17 0x7f49af0cd0ab in mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::MediaDecoderReader> const, nsresult (mozilla::MediaDecoderReader::*)(mozilla::EnumSet<mozilla::TrackInfo::TrackType>), true, (mozilla::RunnableKind)0, mozilla::EnumSet<mozilla::TrackInfo::TrackType> >::Run() obj-firefox/dist/include/nsThreadUtils.h:1187
    #18 0x7f49aa107768 in mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() obj-firefox/dist/include/mozilla/TaskDispatcher.h:209:37
    #19 0x7f49aa0f9584 in mozilla::TaskQueue::Runner::Run() xpcom/threads/TaskQueue.cpp:246:12
    #20 0x7f49aa12ac48 in nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp:225:14
    #21 0x7f49aa12b38c in non-virtual thunk to nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp:154:15
    #22 0x7f49aa12122a in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1437:14
    #23 0x7f49aa1273c8 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:489:10
    #24 0x7f49aaf0ab00 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:339:20
    #25 0x7f49aae66150 in RunInternal ipc/chromium/src/base/message_loop.cc:320:10
    #26 0x7f49aae66150 in RunHandler ipc/chromium/src/base/message_loop.cc:313
    #27 0x7f49aae66150 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:293
    #28 0x7f49aa118eed in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:506:11
    #29 0x7f49c43ad423 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:216:5
    #30 0x7f49c79c26b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)

Thread T100 (MediaPl~back #1) created by T0 here:
    #0 0x4a3dc6 in __interceptor_pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:245:3
    #1 0x7f49c43aa1c9 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:457:14
    #2 0x7f49c43a9dde in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:548:12
    #3 0x7f49aa11b44e in nsThread::Init(nsACString const&) xpcom/threads/nsThread.cpp:688:8
    #4 0x7f49aa12657f in nsThreadManager::NewNamedThread(nsACString const&, unsigned int, nsIThread**) xpcom/threads/nsThreadManager.cpp:273:22
    #5 0x7f49aa1293fb in NS_NewNamedThread xpcom/threads/nsThreadUtils.cpp:113:45
    #6 0x7f49aa1293fb in nsThreadPool::PutEvent(already_AddRefed<nsIRunnable>, unsigned int) xpcom/threads/nsThreadPool.cpp:107
    #7 0x7f49aa12b689 in nsThreadPool::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) xpcom/threads/nsThreadPool.cpp:274:5
    #8 0x7f49aa101fd1 in mozilla::SharedThreadPool::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) obj-firefox/dist/include/mozilla/SharedThreadPool.h:71:68
    #9 0x7f49aa0f82fe in mozilla::TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>&, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) xpcom/threads/TaskQueue.cpp:128:26
    #10 0x7f49aa102df2 in mozilla::TaskQueue::Dispatch(already_AddRefed<nsIRunnable>, mozilla::AbstractThread::DispatchFailureHandling, mozilla::AbstractThread::DispatchReason) obj-firefox/dist/include/mozilla/TaskQueue.h:71:21
    #11 0x7f49aa10734c in mozilla::AutoTaskDispatcher::DispatchTaskGroup(mozilla::UniquePtr<mozilla::AutoTaskDispatcher::PerThreadTaskGroup, mozilla::DefaultDelete<mozilla::AutoTaskDispatcher::PerThreadTaskGroup> >) obj-firefox/dist/include/mozilla/TaskDispatcher.h:261:13
    #12 0x7f49aa1058fc in mozilla::AutoTaskDispatcher::~AutoTaskDispatcher() obj-firefox/dist/include/mozilla/TaskDispatcher.h:91:7
    #13 0x7f49aa1056b0 in reset obj-firefox/dist/include/mozilla/Maybe.h:446:17
    #14 0x7f49aa1056b0 in mozilla::EventTargetWrapper::FireTailDispatcher() xpcom/threads/AbstractThread.cpp:95
    #15 0x7f49aa1088b2 in applyImpl<mozilla::EventTargetWrapper, void (mozilla::EventTargetWrapper::*)()> obj-firefox/dist/include/nsThreadUtils.h:1138:12
    #16 0x7f49aa1088b2 in apply<mozilla::EventTargetWrapper, void (mozilla::EventTargetWrapper::*)()> obj-firefox/dist/include/nsThreadUtils.h:1144
    #17 0x7f49aa1088b2 in mozilla::detail::RunnableMethodImpl<mozilla::EventTargetWrapper*, void (mozilla::EventTargetWrapper::*)(), true, (mozilla::RunnableKind)0>::Run() obj-firefox/dist/include/nsThreadUtils.h:1187
    #18 0x7f49a9f9c543 in mozilla::CycleCollectedJSContext::ProcessStableStateQueue() xpcom/base/CycleCollectedJSContext.cpp:309:12
    #19 0x7f49ab968eed in XPCJSContext::AfterProcessTask(unsigned int) js/xpconnect/src/XPCJSContext.cpp:1007:30
    #20 0x7f49aa121756 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1453:24
    #21 0x7f49aa1273c8 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:489:10
    #22 0x7f49aaf09761 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:97:21
    #23 0x7f49aae66150 in RunInternal ipc/chromium/src/base/message_loop.cc:320:10
    #24 0x7f49aae66150 in RunHandler ipc/chromium/src/base/message_loop.cc:313
    #25 0x7f49aae66150 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:293
    #26 0x7f49b051210f in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:156:27
    #27 0x7f49b4572291 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:287:30
    #28 0x7f49b47411a4 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4589:22
    #29 0x7f49b4742d10 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4772:8
    #30 0x7f49b4744061 in XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4867:21
    #31 0x4eb613 in do_main browser/app/nsBrowserApp.cpp:237:22
    #32 0x4eb613 in main browser/app/nsBrowserApp.cpp:310
    #33 0x7f49c694b82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291

Tyson Smith [:tsmith]

Reporter

Comment 1

•

7 years ago

Attached file test_case.html — Details

Ralph Giles (:rillian)

Comment 2

•

7 years ago

Matthew, looks like nestegg is triggering an assert in the MediaCache. Do you have time to look at this?

Flags: needinfo?(kinetik)

Ralph Giles (:rillian)

Comment 3

•

7 years ago

Ah, could be a straight-up MediaCache bug. I misread the assert: it looks like the BlockList::mBlocks nsTArray is racy or corrupt.

Ralph Giles (:rillian)

Comment 4

•

7 years ago

Gerald, this is the MediaCache crash I mentioned.

Flags: needinfo?(gsquelart)

Ryan VanderMeulen [:RyanVM]

Comment 5

•

7 years ago

INFO: Last good revision: 89d904147926f4b1d66e9e16972b5bc32ba58a42
INFO: First bad revision: 744f286690d7931afcde3eec90490fe4e6279885
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=89d904147926f4b1d66e9e16972b5bc32ba58a42&tochange=744f286690d7931afcde3eec90490fe4e6279885

Though I suppose bisection on another platform would yield something maybe more interesting. Oh well.

Blocks: 1369932

status-firefox54: --- → unaffected

status-firefox55: --- → unaffected

status-firefox56: --- → affected

status-firefox-esr52: --- → unaffected

Flags: in-testsuite?

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

Assignee

Comment 6

•

7 years ago

The file is 1,477,347 bytes long, and after reading up to the end, there is a further read at offset 239,399,576,240,742! (0xD9BB8EB38266)

Somehow MediaCache can't handle this offset, or trusts that the reader shouldn't request bytes outside of the cache and therefore just attempts to read the non-existent block?
I'll investigate and should be able to fix it.

Assignee: nobody → gsquelart

Flags: needinfo?(gsquelart)

Matthew Gregan [:kinetik]

Updated

•

7 years ago

Blocks: 1379039

Matthew Gregan [:kinetik]

Comment 7

•

7 years ago

(In reply to Ralph Giles (:rillian) | needinfo me from comment #2)
> Matthew, looks like nestegg is triggering an assert in the MediaCache. Do
> you have time to look at this?

I spun off bug 1379039 for the nestegg side of this bug.

Matthew Gregan [:kinetik]

Updated

•

7 years ago

Flags: needinfo?(kinetik)

Comment hidden (mozreview-request)

Retrieve pref from MediaPrefs, which is more efficient than
Preferences::GetInt().

Also refactored computations to avoid unnecessary type conversions.

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

Comment hidden (mozreview-request)

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

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

Assignee

Comment 10

•

7 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b949c3385c0018236415cc26b084eb08f437d75a

JW Wang [:jwwang] [:jw_wang]

Comment 11

•

7 years ago

mozreview-review

Comment on attachment 8884138 [details]
Bug 1378518 - Better MediaCache GetMaxBlocks() computation -

https://reviewboard.mozilla.org/r/155058/#review160130

::: dom/media/MediaCache.cpp:742
(Diff revision 1)
>    }
>  }
>  
>  static int32_t GetMaxBlocks()
>  {
>    // We look up the cache size every time. This means dynamic changes

Remove this comment because MediaPrefs doesn't support dynamic prefs changes.

::: dom/media/MediaCache.cpp:750
(Diff revision 1)
> -  int32_t cacheSize = Preferences::GetInt("media.cache_size", 500*1024);
> -  int64_t maxBlocks = static_cast<int64_t>(cacheSize)*1024/MediaCache::BLOCK_SIZE;
> -  maxBlocks = std::max<int64_t>(maxBlocks, 1);
> -  return int32_t(std::min<int64_t>(maxBlocks, INT32_MAX));
> +    std::min(MediaPrefs::MediaCacheSizeKb(), uint32_t(INT32_MAX) * 2);
> +  // Ensure we can divide BLOCK_SIZE by 1024.
> +  static_assert(MediaCache::BLOCK_SIZE % 1024 == 0,
> +                "BLOCK_SIZE should be a multiple of 1024");
> +  // Ensure BLOCK_SIZE/1024 is at least 2.
> +  static_assert(MediaCache::BLOCK_SIZE % 2048 == 0,

It is not useful for BLOCK_SIZE to be something like 1024 * 3. We should just assert BLOCK_SIZE is a multiple of 2048 so you can remove the |MediaCache::BLOCK_SIZE % 1024 == 0| assertion.

::: dom/media/MediaCache.cpp:761
(Diff revision 1)
> +  // cacheSizeKb * 1024 / BLOCK_SIZE == cacheSizeKb / (BLOCK_SIZE / 1024),
> +  // but the latter formula avoids a potential overflow from `* 1024`.
> +  // And because BLOCK_SIZE/1024 is at least 2, the maximum cache size
> +  // INT32_MAX*2 will give a maxBlocks that can fit in an int32_t.
> +  int32_t maxBlocks =
> +    int32_t(cacheSizeKb / (int32_t(MediaCache::BLOCK_SIZE) / 1024));

It would be clearer to define a constexpr int32_t BlockSizeKB = MediaCache::BLOCK_SIZE / 1024 which also can be better optimized by the compiler.

Attachment #8884138 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 12

•

7 years ago

mozreview-review

Comment on attachment 8884139 [details]
Bug 1378518 - Use offset-to-block utilities to check MediaCache offset ranges -

https://reviewboard.mozilla.org/r/155060/#review160134

::: dom/media/MediaCache.cpp:792
(Diff revision 1)
> +{
> +  return IsOffsetAllowed(aOffset) ? OffsetToBlockIndexUnchecked(aOffset) : -1;
> +}
> +
> +// Convert 64-bit offset to 32-bit offset inside a block.
> +// Cannot fail (even if offset is outside allowed range).

This can't fail as long as |BLOCK_SIZE<=INT32_MAX+1| is held which is already the case, right?

Attachment #8884139 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 13

•

7 years ago

Btw, why don't we just use CheckedInt to handle overflow and avoid the clever code?

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

Assignee

Comment 14

•

7 years ago

mozreview-review-reply

Comment on attachment 8884138 [details]
Bug 1378518 - Better MediaCache GetMaxBlocks() computation -

https://reviewboard.mozilla.org/r/155058/#review160130

> Remove this comment because MediaPrefs doesn't support dynamic prefs changes.

As discussed, MediaPrefs actually handle dynamic changes, so I'll keep the comment.

> It is not useful for BLOCK_SIZE to be something like 1024 * 3. We should just assert BLOCK_SIZE is a multiple of 2048 so you can remove the |MediaCache::BLOCK_SIZE % 1024 == 0| assertion.

Oops, the 2nd test should have been `BLOCK_SIZE >= 2048`.

I wanted to separate both tests, as they have different goals: The first one showing we can divide BLOCK_SIZE by 1024 without loss, the second one showing that BLOCK_SIZE/1024>=2.

So I'll fix my 2nd test to be `BLOCK_SIZE / 1024 >= 2`, which is even clearer.

> It would be clearer to define a constexpr int32_t BlockSizeKB = MediaCache::BLOCK_SIZE / 1024 which also can be better optimized by the compiler.

I'm pretty sure any compiler would optimize `BLOCK_SIZE/1024` by calculating it at compile-time and dividing by that value.
But I agree it may be a bit more obvious to separate that constexpr calculation, thanks for the suggestion.

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

Assignee

Comment 15

•

7 years ago

mozreview-review-reply

Comment on attachment 8884139 [details]
Bug 1378518 - Use offset-to-block utilities to check MediaCache offset ranges -

https://reviewboard.mozilla.org/r/155060/#review160134

> This can't fail as long as |BLOCK_SIZE<=INT32_MAX+1| is held which is already the case, right?

Correct, but I wanted to emphasize that this function *could* have checked that the offset was in the allowed range and *could* have returned an error code in that case; instead there is no need to check the return value.
I'll clarify the comment.

Comment hidden (mozreview-request)

Comment on attachment 8884138 [details]
Bug 1378518 - Better MediaCache GetMaxBlocks() computation -

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

Comment hidden (mozreview-request)

Comment on attachment 8884139 [details]
Bug 1378518 - Use offset-to-block utilities to check MediaCache offset ranges -

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

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

Assignee

Comment 18

•

7 years ago

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #13)
> Btw, why don't we just use CheckedInt to handle overflow and avoid the
> clever code?

I had the "gut feeling" that:
1. it would be easy enough to add one set of manually-checked functions,
2. CheckedInt would be slower at run-time,
3. CheckedInt would have meant more complex code changes throughout MediaCache.cpp: E.g., each `x / BLOCK_SIZE` would have had to be changed to a CheckedInt declaration, a test, and finally a value() extraction.
4. The new functions now document the intent of all `x / BLOCK_SIZE` statements (converting offsets to block indices) while doing the extra checks needed.


Thank you for the review & suggestions.

Pulsebot

Comment 19

•

7 years ago

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/713c1616eb5c
Better MediaCache GetMaxBlocks() computation - r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ce6e74eb7da1
Use offset-to-block utilities to check MediaCache offset ranges - r=jwwang

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 20

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/713c1616eb5c
https://hg.mozilla.org/mozilla-central/rev/ce6e74eb7da1

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox56: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla56

test_case.webm 7 years ago Tyson Smith [:tsmith] 1.41 MB, video/webm		Details
test_case.html 7 years ago Tyson Smith [:tsmith] 178 bytes, text/html		Details
Bug 1378518 - Better MediaCache GetMaxBlocks() computation - 7 years ago Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1378518 - Use offset-to-block utilities to check MediaCache offset ranges - 7 years ago Gerald Squelart (he/him) (not at Mozilla since 2022-09-15) 59 bytes, text/x-review-board-request	jwwang : review+	Details