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):
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()
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.
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().
Adding Nico Weber in CC as it seems he wrote this section of createDragImageFromImage() via Bug 100200.
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.
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?
"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.
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.
Created attachment 196097 [details]
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.
Comment on attachment 196097 [details]
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?
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()?)
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?
Created attachment 196224 [details]
Rename method to hasBitmapContent() and fix the callers.
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?
Created attachment 196239 [details]
Quick prototype to remove BitmapImageSingleFrameSkia
Comment on attachment 196239 [details]
Quick prototype to remove BitmapImageSingleFrameSkia
Attachment 196239 [details] did not pass chromium-ews (chromium-xvfb):
New failing tests:
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
Created attachment 196253 [details]
Patch to remove BitmapImageSingleFrameSkia
(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.
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.
(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.
Created attachment 196392 [details]
No longer move BitmapImageSkia code to a separate cpp file.
Comment on attachment 196392 [details]
This looks great! Thanks for the extra effort. r=me
Comment on attachment 196392 [details]
Clearing flags on attachment: 196392
Committed r147583: <http://trac.webkit.org/changeset/147583>
All reviewed patches have been landed. Closing bug.