Summary: | REGRESSION (r260407): Over-release of NSGraphicsContext in WebKit::convertPlatformImageToBitmap() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | WebKit2 | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, nmouchtaris, thorton, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=210814 https://bugs.webkit.org/show_bug.cgi?id=211274 https://bugs.webkit.org/show_bug.cgi?id=211660 |
||||||||
Bug Depends on: | 208891 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2020-04-28 19:33:24 PDT
Created attachment 397917 [details]
Patch v1
Comment on attachment 397917 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=397917&action=review > Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:408 > - auto savedContext = adoptNS([NSGraphicsContext currentContext]); > + RetainPtr<NSGraphicsContext> savedContext = [NSGraphicsContext currentContext]; Do we have an RAII object to save NSGraphicsContext.currentContext? Seems like it might be useful. Comment on attachment 397917 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=397917&action=review >> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:408 >> + RetainPtr<NSGraphicsContext> savedContext = [NSGraphicsContext currentContext]; > > Do we have an RAII object to save NSGraphicsContext.currentContext? Seems like it might be useful. Looks like we have this in WebCore, but it's all CG-based: ALLOW_DEPRECATED_DECLARATIONS_BEGIN CGContextRef cgContext = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort]; ALLOW_DEPRECATED_DECLARATIONS_END CGContextStateSaver stateSaver(cgContext); Via drawCellFocusRingWithFrameAtTime() in Source/WebCore/platform/mac/ThemeMac.mm. Comment on attachment 397917 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=397917&action=review >>> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:408 >>> + RetainPtr<NSGraphicsContext> savedContext = [NSGraphicsContext currentContext]; >> >> Do we have an RAII object to save NSGraphicsContext.currentContext? Seems like it might be useful. > > Looks like we have this in WebCore, but it's all CG-based: > > ALLOW_DEPRECATED_DECLARATIONS_BEGIN > CGContextRef cgContext = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort]; > ALLOW_DEPRECATED_DECLARATIONS_END > > CGContextStateSaver stateSaver(cgContext); > > Via drawCellFocusRingWithFrameAtTime() in Source/WebCore/platform/mac/ThemeMac.mm. Instead of writing out RetainPtr, I suggest you just change the adoptNS() to retainPtr(). Comment on attachment 397917 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=397917&action=review >>>> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:408 >>>> + RetainPtr<NSGraphicsContext> savedContext = [NSGraphicsContext currentContext]; >>> >>> Do we have an RAII object to save NSGraphicsContext.currentContext? Seems like it might be useful. >> >> Looks like we have this in WebCore, but it's all CG-based: >> >> ALLOW_DEPRECATED_DECLARATIONS_BEGIN >> CGContextRef cgContext = (CGContextRef)[[NSGraphicsContext currentContext] graphicsPort]; >> ALLOW_DEPRECATED_DECLARATIONS_END >> >> CGContextStateSaver stateSaver(cgContext); >> >> Via drawCellFocusRingWithFrameAtTime() in Source/WebCore/platform/mac/ThemeMac.mm. > > Instead of writing out RetainPtr, I suggest you just change the adoptNS() to retainPtr(). Thanks, I had forgotten about that helper. Created attachment 397918 [details]
Patch for landing
I think Darin might have been referring to LocalCurrentGraphicsContext? Comment on attachment 397918 [details]
Patch for landing
Setting cq+ with win bot orange as this code isn't used on Windows.
Committed r260864: <https://trac.webkit.org/changeset/260864> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397918 [details]. (In reply to Tim Horton from comment #7) > I think Darin might have been referring to LocalCurrentGraphicsContext? It does seem like LocalCurrentGraphicsContext would be usable here with a tiny bit of refactoring. |