We should be returning nullptr instead of creating GraphicsContext with null platform graphics context when CGBitmapContextCreateWithData returns nil.
Created attachment 375013 [details] Fixes the bug
Committed r247921: <https://trac.webkit.org/changeset/247921>
<rdar://problem/53679029>
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.
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.