search

brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.64k stars 2.3k forks source link

Playlist crashes when playing large/long-running videos when they're cached #34614

Closed stephendonner closed 7 months ago

stephendonner commented 10 months ago

Description

Playlist crashes when switching between large/long-running videos (cached and uncached)

Steps to Reproduce

  1. install 1.62.89
  2. launch Brave
  3. set brave://flags/#playlist to Enabled
  4. click Relaunch
  5. load a bunch of large/long-running vidoes (4+ hours or more, each, typically)
  6. queue at least one -- if not 3 or 4 -- for offline-media playback via the context-menu
  7. play any video
  8. now, switch between videos

NOTE: It appears to be the larger videos (which are slower to cache offline) which are still downloading that might be the problem, here - but I haven't fully nailed it down

Actual result:

💥

playlist-crasher

Crash ID: 6c330f00-f35a-140b-0000-000000000000

[ 00 ] partition_alloc::internal::OnNoMemoryInternal(unsigned long) ( oom.cc:58 )
[ 01 ] partition_alloc::TerminateBecauseOutOfMemory(unsigned long) ( oom.cc:65 )
[ 02 ] partition_alloc::internal::OnNoMemory(unsigned long) ( oom.cc:75 )
[ 03 ] partition_alloc::internal::PartitionExcessiveAllocationSize(unsigned long) ( partition_oom.cc:19 )
[ 04 ] partition_alloc::internal::(anonymous namespace)::PartitionDirectMap(partition_alloc::PartitionRoot*, partition_alloc::internal::AllocFlags, unsigned long, unsigned long) ( partition_bucket.cc:228 )
[ 05 ] partition_alloc::internal::PartitionBucket::SlowPathAlloc(partition_alloc::PartitionRoot*, partition_alloc::internal::AllocFlags, unsigned long, unsigned long, bool*) ( partition_bucket.cc:1340 )
[ 06 ] void* partition_alloc::PartitionRoot::AllocInternalNoHooks<(partition_alloc::internal::AllocFlags)16>(unsigned long, unsigned long) ( partition_root.h:1191 )
[ 07 ] void* partition_alloc::PartitionRoot::AllocInternal<(partition_alloc::internal::AllocFlags)16>(unsigned long, unsigned long, char const*) ( partition_root.h:1953 )
[ 08 ] void* partition_alloc::PartitionRoot::AllocInline<(partition_alloc::internal::AllocFlags)16>(unsigned long, char const*) ( partition_root.h:467 )
[ 09 ] allocator_shim::internal::PartitionMalloc(allocator_shim::AllocatorDispatch const*, unsigned long, void*) ( allocator_shim_default_dispatch_to_partition_alloc.cc:240 )
[ 10 ] base::allocator::dispatcher::internal::DispatcherImpl<base::PoissonAllocationSampler>::AllocFn(allocator_shim::AllocatorDispatch const*, unsigned long, void*) ( dispatcher_internal.h:113 )
[ 11 ] ShimCppNew ( allocator_shim.cc:188 )
[ 12 ] operator new(unsigned long) ( allocator_shim_override_cpp_symbols.h:35 )
[ 13 ] std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>::append(unsigned long, char) ( new:272 )
[ 14 ] std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>::resize(unsigned long, char) ( string:3205 )
[ 15 ] std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>::resize(unsigned long) ( string:1177 )
[ 16 ] base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*)::$_0::operator()(unsigned long) const ( file_util.cc:315 )
[ 17 ] decltype(std::declval<base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*)::$_0 const&>()(std::declval<unsigned long>())) std::__Cr::__invoke<base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*)::$_0 const&, unsigned long>(base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*)::$_0 const&, unsigned long&&) ( invoke.h:344 )
[ 18 ] std::__Cr::invoke_result<base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*)::$_0 const&, unsigned long>::type std::__Cr::invoke<base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*)::$_0 const&, unsigned long>(base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*)::$_0 const&, unsigned long&&) ( invoke.h:30 )
[ 19 ] base::span<unsigned char, 18446744073709551615ul, unsigned char*> absl::functional_internal::InvokeObject<base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*)::$_0, base::span<unsigned char, 18446744073709551615ul, unsigned char*>, unsigned long>(absl::functional_internal::VoidPtr, absl::functional_internal::ForwardT<unsigned long>::type) ( function_ref.h:78 )
[ 20 ] absl::FunctionRef<base::span<unsigned char, 18446744073709551615ul, unsigned char*> (unsigned long)>::operator()(unsigned long) const ( function_ref.h:132 )
[ 21 ] base::FunctionRef<base::span<unsigned char, 18446744073709551615ul, unsigned char*> (unsigned long)>::operator()(unsigned long) const ( function_ref.h:90 )
[ 22 ] base::(anonymous namespace)::ReadStreamToSpanWithMaxSize(__sFILE*, unsigned long, base::FunctionRef<base::span<unsigned char, 18446744073709551615ul, unsigned char*> (unsigned long)>) ( file_util.cc:0 )
[ 23 ] base::ReadStreamToStringWithMaxSize(__sFILE*, unsigned long, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*) ( file_util.cc:313 )
[ 24 ] base::ReadFileToStringWithMaxSize(base::FilePath const&, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*, unsigned long) ( file_util.cc:362 )
[ 25 ] base::ReadFileToString(base::FilePath const&, std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>*) ( file_util.cc:348 )
[ 26 ] playlist::(anonymous namespace)::ReadFileToString(base::FilePath const&) ( playlist_data_source.cc:32 )
[ 27 ] base::OnceCallback<scoped_refptr<base::RefCountedBytes> ()>::Run() && ( callback.h:154 )
[ 28 ] void base::internal::ReturnAsParamAdapter<scoped_refptr<base::RefCountedBytes>>(base::OnceCallback<scoped_refptr<base::RefCountedBytes> ()>, std::__Cr::unique_ptr<scoped_refptr<base::RefCountedBytes>, std::__Cr::default_delete<scoped_refptr<base::RefCountedBytes>>>*) ( post_task_and_reply_with_result_internal.h:23 )
[ 29 ] base::OnceCallback<void ()>::Run() && ( callback.h:154 )
[ 30 ] base::internal::PostTaskAndReplyRelay::RunTaskAndPostReply(base::internal::PostTaskAndReplyRelay) ( post_task_and_reply_impl.h:45 )
[ 31 ] void base::internal::FunctorTraits<void (*)(base::internal::PostTaskAndReplyRelay), void>::Invoke<void (*)(base::internal::PostTaskAndReplyRelay), base::internal::PostTaskAndReplyRelay>(void (*&&)(base::internal::PostTaskAndReplyRelay), base::internal::PostTaskAndReplyRelay&&) ( bind_internal.h:631 )
[ 32 ] void base::internal::InvokeHelper<false, void, 0ul>::MakeItSo<void (*)(base::internal::PostTaskAndReplyRelay), std::__Cr::tuple<base::internal::PostTaskAndReplyRelay>>(void (*&&)(base::internal::PostTaskAndReplyRelay), std::__Cr::tuple<base::internal::PostTaskAndReplyRelay>&&) ( bind_internal.h:868 )
[ 33 ] void base::internal::Invoker<base::internal::BindState<void (*)(base::internal::PostTaskAndReplyRelay), base::internal::PostTaskAndReplyRelay>, void ()>::RunImpl<void (*)(base::internal::PostTaskAndReplyRelay), std::__Cr::tuple<base::internal::PostTaskAndReplyRelay>, 0ul>(void (*&&)(base::internal::PostTaskAndReplyRelay), std::__Cr::tuple<base::internal::PostTaskAndReplyRelay>&&, std::__Cr::integer_sequence<unsigned long, 0ul>) ( bind_internal.h:968 )
[ 34 ] base::internal::Invoker<base::internal::BindState<void (*)(base::internal::PostTaskAndReplyRelay), base::internal::PostTaskAndReplyRelay>, void ()>::RunOnce(base::internal::BindStateBase*) ( bind_internal.h:919 )
[ 35 ] base::TaskAnnotator::RunTaskImpl(base::PendingTask&) ( callback.h:154 )
[ 36 ] void base::TaskAnnotator::RunTask<base::internal::TaskTracker::RunTaskImpl(base::internal::Task&, base::TaskTraits const&, base::internal::TaskSource*, base::SequenceToken const&)::$_0>(perfetto::StaticString, base::PendingTask&, base::internal::TaskTracker::RunTaskImpl(base::internal::Task&, base::TaskTraits const&, base::internal::TaskSource*, base::SequenceToken const&)::$_0&&) ( task_annotator.h:89 )
[ 37 ] base::internal::TaskTracker::RunTaskImpl(base::internal::Task&, base::TaskTraits const&, base::internal::TaskSource*, base::SequenceToken const&) ( task_tracker.cc:644 )
[ 38 ] base::internal::TaskTracker::RunSkipOnShutdown(base::internal::Task&, base::TaskTraits const&, base::internal::TaskSource*, base::SequenceToken const&) ( task_tracker.cc:629 )
[ 39 ] base::internal::WorkerThread::RunWorker() ( task_tracker.cc:659 )
[ 40 ] base::internal::WorkerThread::RunPooledWorker() ( worker_thread.cc:359 )
[ 41 ] base::internal::WorkerThread::ThreadMain() ( worker_thread.cc:339 )
[ 42 ] base::(anonymous namespace)::ThreadFunc(void*) ( platform_thread_posix.cc:101 )
[ 43 ] _pthread_start
[ 44 ] thread_start

Expected result:

No crash

Reproduces how often:

100% with: multiple large files, at least stored for offline, media playing, and switching betweeen them

Brave version (brave://version info)

Brave | 1.62.89 Chromium: 120.0.6099.35 (Official Build) nightly (x86_64)
-- | --
Revision | f68a18538c0af7dba0f91c6176be4c1eee17101c
OS | macOS Version 11.7.10 (Build 20G1427)

Version/Channel Information:

Other Additional Information:

Miscellaneous Information:

/cc @sangwoo108 @rebron @brave/qa-team

stephendonner commented 10 months ago

One of the test files/URLs is https://www.youtube.com/watch?v=kBW-oDpamZg

Looks like it's actually the only cached one, but it's a hefty 3.19 GB file

Screen Shot 2023-11-29 at 4 50 44 AM
stephendonner commented 10 months ago

...which makes sense, since this stack is in out-of-memory (OOM) land.

stephendonner commented 10 months ago

I Removed from playlist the file, and all is well (no crashing when switching between media):

example example
Screen Shot 2023-11-29 at 4 53 59 AM Screen Shot 2023-11-29 at 4 54 06 AM
sangwoo108 commented 10 months ago

I'm hoping I could get some help about this. When playing a video, we're read the video to string and pass it to webui from c++ layer(PlaylistDataSource). I guess this could be what cased OOM. I'm not sure what else we can try.

cc. @simonhong @petemill @fallaciousreasoning

fallaciousreasoning commented 10 months ago

The reading the video to a string is probably the cause - we'll be downloading the entire video to memory. Maybe we can stream it to disk?

sangwoo108 commented 10 months ago

Maybe we can stream it to disk?

Yeah, that sounds great. But I'm not sure how I can achieve that 😂. Should I slice the video into small chunks of bytes and gather them with MedaiSource API? Or is there easy way?

simonhong commented 10 months ago

Just curious why we don't play with local media file path and instead passing it via string? It's bcause of chrome-untrusted://? NVM Data source should pass that file's data :) As we're using custom chrome-untrusted://playlist-data/... url for playlist data, we have its own data source.

I think it's worth to investigate how file:// scheme load big size media file. Maybe loading url something like /Users/simon/Library/ApplicationSupport/BraveSoftware/Brave-Browser-Beta//Default/playlist/60A2BE63931C52698229F7F0C430D243/media_file.mp4 in omnibox will play it well?

Or how about trying local file path instead of custom media path if it's possible from untrusted webui?

fallaciousreasoning commented 10 months ago

I don't know how much control we have over the stream, but if we have control over a HTTP response we can do something like this:

/* super pseudo codey - I'm sure there's a built in Chromium way to just write small chunks to the HTTP stream */

auto http_response = /* whatever the chromium type is */

constexpr size_t chunk_size = 1024;
while (auto chunk = source.read(chunk_size)) {
    http_response.write(chunk);
    /* sleep or whatever so we don't block */ 
}
sangwoo108 commented 7 months ago

Maybe loading url something like /Users/simon/Library/ApplicationSupport/BraveSoftware/Brave-Browser-Beta//Default/playlist/60A2BE63931C52698229F7F0C430D243/media_file.mp4 in omnibox will play it well?

it might not be allowed due to the CSP.

local file path instead of custom media path if it's possible from untrusted webui?

Also this seems to be difficult without explicit user interaction via <input type=file for security/privacy reason.

So, not sure if it works, but I guess we should do something like from PlaylistDataSource

  1. chrome://playlist-data/{id}/media/ should return the size of chunks and media type
  2. `chrome://playlist-data/{id}/media/chunk/{number} should return the requested chunk.
  3. WebUI should get size of chunks first and iterate chunks to add buffers to the MediaSource.

Update:

It seems that Youtube videos(mp4) we caches are not in format that can be used for MediaSource. So I'm going to try gather chunks into byte array(not MediaSource's source buffer), and set it to video tag's src attribute.

Even if the byte array causes OOM, only renderer would crash. I think that's better. But eventually, we'll need to work on caching MediaSource object as is.

[35205:259:0220/185131.533058:ERROR:box_definitions.cc(2063)] Failure parsing MP4: Detected unfragmented MP4. Media Source Extensions require ISO BMFF moov to contain mvex to indicate that Movie Fragments are to be expected.
sangwoo108 commented 7 months ago

Updated issue title

petemill commented 7 months ago

It would be great to support http range requests and just get the video player itself to support the chunking. Added benefit would be efficient seeking (only downloading the part of the video that's being watched).

@sangwoo108 and I took a look at URLDataSource and it seems we don't get the request headers (or ability to set response headers). I wonder if we can use a Throttle, which does have access to the full request data in order to set the appropriate range request and response headers? https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests

sangwoo108 commented 7 months ago

It would be great to support http range requests and just get the video player itself to support the chunking.

This seems to be similar to what @fallaciousreasoning suggested. Maybe we make use of the throttle or need to plumb new API from the web_ui_url_loader_factory, which calls the WebUIDataSource's methods.

sangwoo108 commented 7 months ago

At the moment, it seems the blink::URLLoaderThrottles are attached to main resource requests(from brave_content_browser_client). So requests for sub resources are not going through URLLoaderThrottles. I might be missing because there're other types of the throttle(e.g. content::NavigationThrottle). I will keep trying find out a better way.

sangwoo108 commented 7 months ago

And one more thing worth mentioning is that WebUIUrlLoaderFactory rejects the HTTP range request for simplicity. https://source.chromium.org/chromium/chromium/src/+/main:content/browser/webui/web_ui_url_loader_factory.cc;l=222;drc=c686e8f4fd379312469fe018f5c390e9c8f20d0d .

So we might need to start from here.

Maybe it means it doesn't support multi ranges in one header, but still supports range.

sangwoo108 commented 7 months ago

Yay, I think we're close to it. Succeeded in playing 24hr long video

image
stephendonner commented 7 months ago

That's...pretty "reasonable."

😆 - nice work as always!

bsclifton commented 7 months ago

@sangwoo108 is the WIP for this one https://github.com/brave/brave-core/pull/22304?

sangwoo108 commented 7 months ago

Yes, that's right.

stephendonner commented 6 months ago

Verified PASSED using

Brave   1.65.83 Chromium: 123.0.6312.46 (Official Build) beta (x86_64) 
Revision    1ea667c8c59a4a4a327579974ec11149a738e750
OS  macOS Version 11.7.10 (Build 20G1427)

Steps:

  1. installed 1.65.83
  2. launched Brave
  3. enabled playlist via brave://flags
  4. loaded youtube.com
  5. played/queued a few videos
  6. added them to playlist via the icon in the URL bar
  7. ensured all were Kept for offline playing
  8. switched between them
  9. played a video
  10. while the video was playing, removed its offline data
  11. switched between videos
  12. returned to the original video, continued playing it
  13. removed other videos from the playlist

Ensured no crashes doing the above, with rather-large media files (known to have crashed, before)

no-crash-playlist

stephendonner commented 6 months ago

Verified PASSED using

Brave | 1.65.88 Chromium: 123.0.6312.58 (Official Build) beta (64-bit)
-- | --
Revision | e260e641e7db9e88b9b51112f4f00febf5a2f5eb
OS | Windows 10 Version 22H2 (Build 19045.4170)

Confirmed no crashes, following my original steps in https://github.com/brave/brave-browser/issues/34614#issuecomment-2004913389

https://github.com/brave/brave-browser/assets/387249/4de97147-0780-42ae-9680-e2d0f332dc29