Bug 211160 – REGRESSION (r260407): Over-release of NSGraphicsContext in WebKit::convertPlatformImageToBitmap()

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Over-release of NSGraphicsContext in WebKit::convertPlatformImageToBitmap().

Occurred in r260407 for Bug 208891:

    auto savedContext = adoptNS([NSGraphicsContext currentContext]);

[David Kilzer (:ddkilzer)]

Created attachment 397917 [details]
Patch v1

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

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().

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 397918 [details]
Patch for landing

[Tim Horton]

I think Darin might have been referring to LocalCurrentGraphicsContext?

[David Kilzer (:ddkilzer)]

Comment on attachment 397918 [details]
Patch for landing

Setting cq+ with win bot orange as this code isn't used on Windows.

[EWS]

Committed r260864: <https://trac.webkit.org/changeset/260864>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397918 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62573162>

[Darin Adler]

(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.