-
Notifications
You must be signed in to change notification settings - Fork 16.9k
fix: prevent crash when GPU context unavailable in sharedTexture #49518
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to log on every line here.
| if (!factory) { | ||
| LOG(ERROR) << "ImageTransportFactory is null"; | ||
| return nullptr; | ||
| } | ||
|
|
||
| auto* context_factory = factory->GetContextFactory(); | ||
| if (!context_factory) { | ||
| LOG(ERROR) << "ContextFactory is null"; | ||
| return nullptr; | ||
| } | ||
|
|
||
| scoped_refptr<viz::RasterContextProvider> provider = | ||
| context_factory->SharedMainThreadRasterContextProvider(); | ||
| if (!provider) { | ||
| LOG(ERROR) << "RasterContextProvider is null"; | ||
| return nullptr; | ||
| } | ||
|
|
||
| return provider->SharedImageInterface(); | ||
| } else { | ||
| return blink::SharedGpuContext::SharedImageInterfaceProvider() | ||
| ->SharedImageInterface(); | ||
| auto* provider = blink::SharedGpuContext::SharedImageInterfaceProvider(); | ||
| if (!provider) { | ||
| LOG(ERROR) | ||
| << "SharedImageInterfaceProvider is null, GPU context unavailable"; | ||
| return nullptr; | ||
| } | ||
|
|
||
| F440 | return provider->SharedImageInterface(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same like above.
|
|
||
| std::string GetBase64StringFromSyncToken(gpu::SyncToken& sync_token) { | ||
| if (!sync_token.verified_flush()) { | ||
| auto* sii = GetSharedImageInterface(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of logging and silently return, is it possible to pass sii and context support as parameter in, instead of GetShareImageInterface, GetContextSupport in place? Then we can do checks earlier in outer entry functions with throwable javascript error.
| // Texture id for printing warnings like GC check. | ||
| std::string id; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove spaces (You might want to run npm run lint to find lint errors locally before failing the CI)
| if (!sii) { | ||
| LOG(WARNING) | ||
| << "Cannot update release sync token, SharedImageInterface is null"; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, can we pass sii in?
| base::AutoLock locker(release_sync_token_lock_); | ||
|
|
||
| auto* sii = GetSharedImageInterface(); | ||
| if (!sii) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
| if (release_callback) { | ||
| GetContextSupport()->SignalSyncToken(release_sync_token, | ||
| std::move(release_callback)); | ||
| auto* context_support = GetContextSupport(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
|
Thanks for fixing this! |
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
| if (!factory) { | ||
| LOG(ERROR) << "ImageTransportFactory is null"; | ||
| return nullptr; | ||
| } | ||
|
|
||
| auto* context_factory = factory->GetContextFactory(); | ||
| if (!context_factory) { | ||
| LOG(ERROR) << "ContextFactory is null"; | ||
| return nullptr; | ||
| } | ||
|
|
||
| scoped_refptr<viz::RasterContextProvider> provider = | ||
| context_factory->SharedMainThreadRasterContextProvider(); | ||
| if (!provider) { | ||
| LOG(ERROR) << "RasterContextProvider is null"; | ||
| return nullptr; | ||
| } | ||
|
|
||
| return provider->SharedImageInterface(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same logging comments here as above
95b0bff to
8f73cfd
Compare
Description of Change
This PR fixes a critical crash in the
sharedTexture(introduced in #47317) module related to improper lifecycle management of the graphics context provider.The crash happens in the
ImportedSharedTexturedestructor when it callsSetupReleaseSyncTokenCallback(), which in turn callsGetContextSupport(). In renderer processes,GetContextSupport()attempts to dereference aWeakPtr<blink::WebGraphicsContext3DProviderWrapper>without first checking if the pointer is valid. When the GPU context is unavailable (e.g., during shutdown or GPU device loss), thisWeakPtris null, causing a CHECK failure.The fix introduces a safety check in
GetContextSupport()to verify the availability of the context provider before usage. Additionally, the destruction sequence has been updated to handle context loss gracefully, preventing theEXCEPTION_BREAKPOINTtriggered bybase::WeakPtr's internal checks.Stack Trace Included:
1
2
Checklist
npm testpassesRelease Notes
Notes: Fixed crash in sharedTexture module when GPU context becomes unavailable.