Bug 200185 – ShareableBitmap::createGraphicsContext() should return nullptr when CGBitmapContextCreateWithData returns nil

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Ryosuke Niwa]

We should be returning nullptr instead of creating GraphicsContext with null platform graphics context when CGBitmapContextCreateWithData returns nil.

[Ryosuke Niwa]

Created attachment 375013 [details]
Fixes the bug

[Ryosuke Niwa]

Committed r247921: <https://trac.webkit.org/changeset/247921>

[Radar WebKit Bug Importer]

<rdar://problem/53679029>

[Said Abou-Hallawa]

Comment on attachment 375013 [details]
Fixes the bug

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

> Source/WebKit/Shared/ContextMenuContextData.cpp:71
> +    auto graphicsContext = m_controlledImage->createGraphicsContext();
> +    if (!graphicsContext)
> +        return;
> +    graphicsContext->drawImage(*image, IntPoint());

These four lines can be shorten to two:

if (auto graphicsContext = m_controlledImage->createGraphicsContext())
    graphicsContext->drawImage(*image, IntPoint());

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1100
> +    auto graphicsContext = bitmap->createGraphicsContext();
> +    if (graphicsContext)
> +        graphicsContext->drawImage(image, IntPoint());

Getting graphicsContext can be moved to the if-statement.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:283
> +        if (context)
> +            drawInContext(*context, backImage.get());

The check for "if (context)" should be moved above few lines immediately after assigning the context. There is no need to calculate backImage if context is null.

> Source/WebKit/Shared/cg/ShareableBitmapCG.cpp:97
>      ASSERT(bitmapContext.get());

I do not think we need this assertion after adding the above if-statement.

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:188
>      auto graphicsContext = webImage->bitmap().createGraphicsContext();
> +    if (!graphicsContext)
> +        return nullptr;
> +
>      graphicsContext->drawImage(bitmapImage, {{ }, size});

These four lines can be shortened to two if use "if (auto graphicsContext = ...)"

> Source/WebKit/WebProcess/Plugins/PluginProxy.cpp:216
>          auto graphicsContext = m_backingStore->createGraphicsContext();
> -        graphicsContext->applyDeviceScaleFactor(contentsScaleFactor());
> -        graphicsContext->setCompositeOperation(CompositeCopy);
> +        if (graphicsContext) {

Calculating graphicsContext can be moved to the if-statement.

> Source/WebKit/WebProcess/Plugins/PluginProxy.cpp:728
>          // Blit the plug-in backing store into our own backing store.
>          auto graphicsContext = m_backingStore->createGraphicsContext();
> -        graphicsContext->applyDeviceScaleFactor(contentsScaleFactor());
> -        graphicsContext->setCompositeOperation(CompositeCopy);
> -        m_pluginBackingStore->paint(*graphicsContext, contentsScaleFactor(), paintedRect.location(), paintedRect);
> +        if (graphicsContext) {

Ditto.

> Source/WebKit/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:750
>      auto bitmap = ShareableBitmap::createShareable(backingStoreSize, { });

Don't we need check whether bitmap is equal to nullptr or not? We check for that in convertCGImageToBitmap().

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1245
>      auto bitmap = ShareableBitmap::createShareable(backingStoreSize, { });

Ditto.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4789
> +        if (graphicsContext) {

It is strange that you decided to continue in this function if graphicsContext is nullptr instead of returning. Above this code we return if (!bitmap). What is the point in continuing this function If graphicsContext is nullptr?

> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp:744
> +        if (graphicsContext)
> +            m_webPage.drawRect(*graphicsContext, rect);
>          updateInfo.updateRects.append(rect);

Why do we append rect to updateRects if graphicsContext is nullptr? We are not drawing on this rect in this case.

[Ryosuke Niwa]

Please feel free to post a cleanup patch. I don't know enough about this code to follow up in a meaningful way. I was just trying to fix a crash.