Bug 218924 - [GPUProcess] Add basic GPUProcess crash handling for canvas
Summary: [GPUProcess] Add basic GPUProcess crash handling for canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-13 15:13 PST by Chris Dumez
Modified: 2020-11-16 16:44 PST (History)
6 users (show)

See Also:


Attachments
Patch (20.27 KB, patch)
2020-11-13 15:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (20.24 KB, patch)
2020-11-13 16:14 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-11-13 15:13:27 PST
Add basic GPUProcess crash handling for canvas.
Comment 1 Chris Dumez 2020-11-13 15:22:46 PST
Created attachment 414098 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-11-13 15:59:44 PST
Comment on attachment 414098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414098&action=review

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:83
> +    float resolutionScale() const override { return m_resolutionScale; }

final?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:66
> +    // Register itself as a MessageReceiver in the GPUProcessConnection.

Comment doesn't add anything.

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:70
> +    IPC::MessageReceiverMap& messageReceiverMap = connection.messageReceiverMap();

auto?

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:74
> +    send(Messages::GPUConnectionToWebProcess::CreateRenderingBackend(m_renderingBackendIdentifier), 0);

Comment doesn't add anything.

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:101
> +    connectToGPUProcess();
> +
> +    for (auto& pair : m_remoteResourceCacheProxy.imageBuffers()) {
> +        if (auto& baseImageBuffer = pair.value) {
> +            if (is<AcceleratedRemoteImageBufferProxy>(*baseImageBuffer))
> +                recreateImageBuffer(*this, downcast<AcceleratedRemoteImageBufferProxy>(*baseImageBuffer), pair.key, m_renderingBackendIdentifier);
> +            else
> +                recreateImageBuffer(*this, downcast<UnacceleratedRemoteImageBufferProxy>(*baseImageBuffer), pair.key, m_renderingBackendIdentifier);
> +        }
> +    }

It's weird to see this code in a "didClose" function. It would be nicer to move this to a "reestablishGPUConnection" function.
Comment 3 Chris Dumez 2020-11-13 16:14:15 PST
Created attachment 414103 [details]
Patch
Comment 4 EWS 2020-11-13 18:11:53 PST
Committed r269809: <https://trac.webkit.org/changeset/269809>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414103 [details].
Comment 5 Radar WebKit Bug Importer 2020-11-13 18:12:20 PST
<rdar://problem/71392812>
Comment 6 Darin Adler 2020-11-15 13:17:09 PST
Comment on attachment 414103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=414103&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/GPUProcess.mm:416
> +        RetainPtr<CGContextRef> context = CGBitmapContextCreate(rgba, viewWidthInPixels, viewHeightInPixels, 8, 4 * viewWidthInPixels, colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big);

Missing adoptCF here, so this bitmap context will leak.
Comment 7 Chris Dumez 2020-11-16 08:54:54 PST
(In reply to Darin Adler from comment #6)
> Comment on attachment 414103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414103&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/GPUProcess.mm:416
> > +        RetainPtr<CGContextRef> context = CGBitmapContextCreate(rgba, viewWidthInPixels, viewHeightInPixels, 8, 4 * viewWidthInPixels, colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big);
> 
> Missing adoptCF here, so this bitmap context will leak.

I had copied from an existing screenshot tests. Will fix existing tests and new tests via https://bugs.webkit.org/show_bug.cgi?id=218988. Thanks.