Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Desktop] Crash in CanvasAsyncBlobCreator #10913

Closed
iefremov opened this issue Jul 24, 2020 · 5 comments · Fixed by brave/brave-core#6320
Closed

[Desktop] Crash in CanvasAsyncBlobCreator #10913

iefremov opened this issue Jul 24, 2020 · 5 comments · Fixed by brave/brave-core#6320
Labels
crash/webview Only tab webview crash. Browser doesn't crash OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA/No release-notes/include

Comments

@iefremov
Copy link
Contributor

It appears to me that static cast to LocalDomWindow in overriden canvas_async_blob_creator.cc is not safe (Document* document = To<LocalDOMWindow>(context)->document();), because context is not necessarily a window, it could be a worker. We can actually see V8OffscreenCanvas in the callstack that is used in workers.

So I suggest to change To to DynamicTo and also check other potentially dangerous callsites in Farbling infrastructure

[ 00 ] brave::BraveSessionCache::From(blink::Document&)
[ 01 ] blink::CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(scoped_refptr<blink::StaticBitmapImage>, blink::ImageEncodeOptions const*, blink::CanvasAsyncBlobCreator::ToBlobFunctionType, blink::V8BlobCallback*, base::TimeTicks, blink::ExecutionContext*, blink::ScriptPromiseResolver*)
[ 02 ] blink::CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(scoped_refptr<blink::StaticBitmapImage>, blink::ImageEncodeOptions const*, blink::CanvasAsyncBlobCreator::ToBlobFunctionType, base::TimeTicks, blink::ExecutionContext*, blink::ScriptPromiseResolver*)
[ 03 ] blink::CanvasRenderingContextHost::convertToBlob(blink::ScriptState*, blink::ImageEncodeOptions const*, blink::ExceptionState&)
[ 04 ] blink::V8OffscreenCanvas::ConvertToBlobMethodCallback(v8::FunctionCallbackInfo<v8::Value> const&)
[ 05 ] v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*)
[ 06 ] Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit

...

https://brave.sp.backtrace.io/p/brave/triage?aggregations=((guid%2Cunique)%2C(classifiers%2Chead))&fingerprint=0e19f930cd9b5a95610deadfcf5e934c7c08b3213d535412d5559179d3bad178

also https://brave.sp.backtrace.io/p/brave/debug?filters=(_deleted%3D0%2C(ver%2Cregex%2C%228%5B1%7C2%7C3%7C4%5D.*%22)%2Cptype%3Drenderer%2C(callstack%2Ccontains%2CCanvasAsyncBlobCreator))&debug=(%224bbf3%22,0,0)

@iefremov iefremov added crash/webview Only tab webview crash. Browser doesn't crash priority/P1 A very extremely bad problem. We might push a hotfix for it. OS/Desktop labels Jul 24, 2020
@iefremov
Copy link
Contributor Author

cc @bsclifton to triage
cc @pes10k @pilgrim-brave

@bsclifton bsclifton added priority/P2 A bad problem. We might uplift this to the next planned release. and removed priority/P1 A very extremely bad problem. We might push a hotfix for it. labels Aug 3, 2020
@iefremov
Copy link
Contributor Author

iefremov commented Aug 4, 2020

This one should be fixed with brave/brave-core#6320 @pilgrim-brave
Not sure if this bug and #10914 are same problem though

@pilgrim-brave
Copy link

I believe they are different problems. I mis-linked the PR. This crash is fixed now, #10914 is not yet.

@pilgrim-brave
Copy link

QA/No because it's handled by new automated tests which were part of the PR that fixed it.

@kjozwiak
Copy link
Member

kjozwiak commented Aug 6, 2020

Awesome, thanks @pilgrim-brave 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crash/webview Only tab webview crash. Browser doesn't crash OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA/No release-notes/include
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants