Bug 113492 – [Chromium] Bad cast from BitmapImageSingleFrameSkia to BitmapImage

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

createDragImageFromImage() casts an Image* to a BitmapImage* if Image::isBitmapImage() returns true. However, BitmapImageSingleFrameSkia::isBitmapImage() also returns true and BitmapImageSingleFrameSkia does not subclass BitmapImage.

Unluckily, the Image passed to createDragImageFromImage() is the result of ImageBuffer::copyImage() which is a BitmapImageSingleFrameSkia when using Skia, not a BitmapImage.

This leads to crashing in some of the drag'n drop test cases (e.g. editing/pasteboard/dataTransfer-setData-getData.html):
crash log for DumpRenderTree (pid 14229):
STDOUT: <empty>
STDERR: Received signal 11 <unknown> 000000000000
STDERR:  [0x000001813854] base::debug::StackTrace::StackTrace()
STDERR:  [0x000001813139] base::debug::(anonymous namespace)::StackDumpSignalHandler()
STDERR:  [0x7f504c3b1cb0] <unknown>
STDERR:  [0x00000068dada] WebCore::DeferredImageDecoder::frameSizeAtIndex()
STDERR:  [0x00000067bf67] WebCore::ImageSource::frameSizeAtIndex()
STDERR:  [0x00000067bf19] WebCore::ImageSource::size()
STDERR:  [0x000000636e23] WebCore::BitmapImage::updateSize()
STDERR:  [0x000000636ea8] WebCore::BitmapImage::sizeRespectingOrientation()
STDERR:  [0x000000629973] WebCore::createDragImageFromImage()
STDERR:  [0x000002b5cfeb] WebCore::Frame::dragImageForSelection()
STDERR:  [0x000002eac85b] WebCore::createDragImageForSelection()
STDERR:  [0x000002b38b96] WebCore::DragController::startDrag()
STDERR:  [0x000002b47ade] WebCore::EventHandler::handleDrag()
STDERR:  [0x000002b3bd0f] WebCore::EventHandler::handleMouseDraggedEvent()
STDERR:  [0x000002b3fcd3] WebCore::EventHandler::handleMouseMoveEvent()
STDERR:  [0x000002bee653] WebCore::EventHandler::passMouseMoveEventToSubframe()
STDERR:  [0x000002b3fb1e] WebCore::EventHandler::handleMouseMoveEvent()
STDERR:  [0x000002b3f2eb] WebCore::EventHandler::mouseMoved()
STDERR:  [0x00000051037f] WebKit::PageWidgetEventHandler::handleMouseMove()
STDERR:  [0x0000005100fc] WebKit::PageWidgetDelegate::handleInputEvent()
STDERR:  [0x0000004ba942] WebKit::WebViewImpl::handleInputEvent()
STDERR:  [0x0000017e0cd4] WebTestRunner::EventSender::doMouseMove()
STDERR:  [0x0000017e1e57] WebTestRunner::EventSender::replaySavedEvents()
STDERR:  [0x0000017e07e3] WebTestRunner::EventSender::mouseUp()
STDERR:  [0x0000017e6e53] WebTestRunner::CppBoundClass::MemberCallback<>::run()
STDERR:  [0x0000017da771] WebTestRunner::CppBoundClass::invoke()
STDERR:  [0x0000017da2c5] WebTestRunner::CppNPObject::invoke()
STDERR:  [0x00000276c247] WebCore::npObjectInvokeImpl()
STDERR:  [0x00000276c436] WebCore::npObjectMethodHandler()
STDERR:  [0x000001fcb3ae] v8::internal::HandleApiCallHelper<>()
STDERR:  [0x000001fc5ce7] v8::internal::Builtin_Impl_HandleApiCall()
STDERR:  [0x000001fc5cb8] v8::internal::Builtin_HandleApiCall()

[Chris Dumez]

Interestingly, I'm only able to reproduce the crash with the patch from Bug 113420. However, that patch does not change the behavior here so I think this is just (bad) luck.

[Chris Dumez]

I'm still unsure how to best fix it.

IMHO BitmapImageSingleFrameSkia::isBitmapImage() should probably not return true as BitmapImageSingleFrameSkia doesn't subclass BitmapImage.

Image::isBitmapImage() is often used for downcasting from Image to BitmapImage as it is done in createDragImageFromImage().

[Chris Dumez]

Adding Nico Weber in CC as it seems he wrote this section of createDragImageFromImage() via Bug 100200.

[Stephen White]

See also https://bugs.webkit.org/show_bug.cgi?id=66597, which tried to eliminate this class of bugs.  Looks like someone tripped on it again.

[Chris Dumez]

I confirmed that the Image in createDragImageFromImage() (from DragImageChromiumSkia.cpp) is really a BitmapImageSingleFrameSkia*, not a BitmapImage*. Therefore, the casting to BitmapImage* really is wrong.

The odd thing is that the code seems to currently work despite the bad cast (not sure exactly why). However, with the patch from Bug 113420 applied, it crashes consistently.

I looked at the patch from Bug 113420 to try to understand what has changed. One difference I could find is that it changes the size of the BitmapImageSingleFrameSkia class by converting one of its members from NativeImageSkia to RefPtr<NativeImageSkia>.

Again, I'm not sure how to handle the problem here. We should really cast to a BitmapImageSingleFrameSkia* instead of a BitmapImage*. However, if we do that, we lose the information we need about orientation as BitmapImageSingleFrameSkia does not provide sizeRespectingOrientation() or currentFrameOrientation(). BitmapImageSingleFrameSkia only holds the pixel data so I don't think we can get this information either, right?

[Tom Hudson]

"We should really cast to a BitmapImageSingleFrameSkia* instead of a BitmapImage*. However, if we do that, we lose the information we need about orientation as BitmapImageSingleFrameSkia does not provide sizeRespectingOrientation() or currentFrameOrientation()."

Yes. 66597 was trying to address this problem, but I moved off it (literally - now in England). If somebody knows how to handle he eight-zillion-build-system problem, I think the idea in 66597 is still sound, unless the SVG changes have introduced new dependencies on the BitmapImage members that aren't mirrored in BitmapImageSingleFrameSkia, which #5 suggests.

I want to get this 2+-year-old class of recurring problems fixed; I may have time next week to help.

[Chris Dumez]

Ok, so actually, I understand why the code worked now.

createDragImageFromImage() is called from differently places. Depending, where it is called from, the input Image may have a different type.

If the input image really is a BitmapImage, then the code would work. However, if the image came from ImageBuffer::copyImage(), then it is a BitmapImageSingleFrameSkia. And the code may crash or simply not do anything if you're lucky. This happens, for example, when you *select* elements and then drag them.

So, I think the patch from Bug 66597 would solve it as BitmapImageSingleFrameSkia::isBitmapImage() would no longer return true. The image would get rotated only if it is actually a BitmapImage, not a BitmapImageSingleFrameSkia. And this would be fine because we do not need to rotate the BitmapImageSingleFrameSkia anyway.

[Chris Dumez]

Created attachment 196097 [details]
Patch

Simpler proposal than in Bug 66597 that does not introduce a new base class. Since BitmapImage and BitmapImageSingleFrameSkia do not really share any code, adding a base class does not really useful.

[Stephen White]

Comment on attachment 196097 [details]
Patch

This seems like a nice simple fix, but I'm not sure what isBitmapKind() really means.  What is the contract you're agreeing to by returning true?  Is there a way we could capture that contract in the name?

[Tom Hudson]

Yeah, transparent naming is one of the things that my first attempts (pre-66597) to address this floundered on. WebKit apparently has a well-known isFoo() pattern for ad-hoc RTTI, but isXKind() is new, and I don't know how to introduce a new pattern like that without comments.

(hasBitmapSemantics()? ...Data()?  ...Contents()?)

[Chris Dumez]

I tend to agree that isBitmapKind() is not great. However, I couldn't come up with something better.

I took a closer look at the call sites to try to find a better name:
A. CachedImage::destroyDecodedData() wants to know if destroying the image may re-enter destroyDecodedData. Apparently, this was happening for SVGImage so someone added a m_image->isBitmapImage() check (Bug 22483).

Maybe this check could really be a !isSVGImage() instead, assuming the issue is still valid.

B. CachedImage::currentFrameKnownToBeOpaque() wants to know if there is data should be decoded. It basically calls nativeImageForCurrentFrame() before currentFrameKnownToBeOpaque() to make sure the frame is cached. This behavior really seems specific the BitmapImage class so I probably shouldn't have changed to isBitmapKind() there after all.

C. ImageQualityController::shouldPaintAtLowQuality() returns false for non-bitmap images. It basically wants to know if low quality scaling should be used and this only makes sense for bitmap images.

D. RenderLayerBacking::isDirectlyCompositedImage() wants to know if the "direct image compositing" code path can be used. With this code path, we set the image directly as the contents of the GraphicsLayer but this works only for bitmap images (Bug 46144).

So it seems like A could be replaced with !isSVGImage() check (maybe removed?). B change is not needed. C and D just want to know if the Image contains a Bitmap.

Maybe we can call this method hasBitmapData() like Tom suggested?

[Chris Dumez]

Created attachment 196224 [details]
Patch

Rename method to hasBitmapContent() and fix the callers.

[Stephen White]

I'm not sold on hasBitmapContent() either.  That sounds like you're going to call getBitmapContent() on it, which doesn't exist.  It still doesn't really tell me what it means to have bitmap content.  When another developer comes along, how do they know when to call hasBitmapContent()?  Especially when it has no effect on their non-Skia platform, for example.

Thinking a bit wider, I wonder how hard it would be to get rid of BitmapImageSingleFrameSkia altogether.

Could we just have ImageBuffer[Skia]::copyImage() construct a NativeImageSkia, from the given SkBitmap?  (shallow or deep, depending on the argument)  It looks like NativeImageSkia has shallow-copying capability now.  Then we create an actual BitmapImage from the NativeImagePtr.  Would that work?

[Chris Dumez]

Created attachment 196239 [details]
Quick prototype to remove BitmapImageSingleFrameSkia

[WebKit Review Bot]

Comment on attachment 196239 [details]
Quick prototype to remove BitmapImageSingleFrameSkia

Attachment 196239 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17337577

New failing tests:
canvas/philip/tests/2d.composite.uncovered.nocontext.source-in.html
canvas/philip/tests/2d.drawImage.canvas.html
canvas/philip/tests/2d.composite.canvas.destination-in.html
canvas/philip/tests/2d.composite.canvas.source-out.html
canvas/philip/tests/2d.composite.uncovered.nocontext.copy.html
css3/filters/effect-blur.html
canvas/philip/tests/2d.composite.canvas.copy.html
css3/filters/blur-filter-page-scroll-parents.html
canvas/philip/tests/2d.composite.canvas.source-in.html
canvas/philip/tests/2d.shadow.canvas.alpha.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
css3/filters/add-filter-rendering.html
canvas/philip/tests/2d.composite.canvas.source-atop.html
css3/filters/blur-filter-page-scroll.html
canvas/philip/tests/2d.shadow.canvas.transparent.2.html
canvas/philip/tests/2d.composite.uncovered.nocontext.destination-atop.html
canvas/philip/tests/2d.shadow.canvas.basic.html
canvas/philip/tests/2d.composite.uncovered.nocontext.source-out.html
canvas/philip/tests/2d.fillStyle.parse.current.removed.html
canvas/philip/tests/2d.composite.uncovered.nocontext.destination-in.html
compositing/masks/layer-mask-placement.html
css3/filters/blur-filter-page-scroll-self.html
css3/filters/css-opacity-with-drop-shadow.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.canvas.destination-over.html
canvas/philip/tests/2d.drawImage.self.1.html
css3/filters/crash-filter-change.html
canvas/philip/tests/2d.composite.canvas.lighter.html
canvas/philip/tests/2d.composite.canvas.xor.html
canvas/philip/tests/2d.composite.canvas.source-over.html

[WebKit Review Bot]

Created attachment 196252 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Chris Dumez]

Created attachment 196253 [details]
Patch to remove BitmapImageSingleFrameSkia

[Tom Hudson]

(In reply to comment #13)
> Could we just have ImageBuffer[Skia]::copyImage() construct a NativeImageSkia, from the given SkBitmap?  (shallow or deep, depending on the argument)  It looks like NativeImageSkia has shallow-copying capability now.  Then we create an actual BitmapImage from the NativeImagePtr.  Would that work?

I don't know if it's still relevant, but from comments 2 bugs ago the original purpose of BitmapImageSingleFrameSkia (several years ago) was to provide a significant performance improvement over BitmapImage, since we don't have the concept of multiple frames and so a lot of the overhead of managing them was unnecessary.

[Stephen White]

This looks like a great start.  However, could you leave the BitmapImage class where it is, in order to make reviewing this patch easier?  We can do the removal in another patch, since this makes it difficult to tell what the changes are.

[Stephen White]

(In reply to comment #18)
> (In reply to comment #13)
> > Could we just have ImageBuffer[Skia]::copyImage() construct a NativeImageSkia, from the given SkBitmap?  (shallow or deep, depending on the argument)  It looks like NativeImageSkia has shallow-copying capability now.  Then we create an actual BitmapImage from the NativeImagePtr.  Would that work?
> 
> I don't know if it's still relevant, but from comments 2 bugs ago the original purpose of BitmapImageSingleFrameSkia (several years ago) was to provide a significant performance improvement over BitmapImage, since we don't have the concept of multiple frames and so a lot of the overhead of managing them was unnecessary.

It should definitely be benchmarked, but I think part of the problem was that, at the time that comment was written, we didn't have a way to shallow-copy the SkBitmap into NativeImageSkia.  Now (for now == ~2.5 years ago) we do.

IIRC, canvas-to-canvas drawImage() exercises ImageBuffer::copy(), and we have a fair number of sites we can use to bench this change.

[Chris Dumez]

Created attachment 196392 [details]
Patch

No longer move BitmapImageSkia code to a separate cpp file.

[Stephen White]

Comment on attachment 196392 [details]
Patch

This looks great!  Thanks for the extra effort.  r=me

[WebKit Review Bot]

Comment on attachment 196392 [details]
Patch

Clearing flags on attachment: 196392

Committed r147583: <http://trac.webkit.org/changeset/147583>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.