Bug 113492 - [Chromium] Bad cast from BitmapImageSingleFrameSkia to BitmapImage
Summary: [Chromium] Bad cast from BitmapImageSingleFrameSkia to BitmapImage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 113420
  Show dependency treegraph
 
Reported: 2013-03-28 02:38 PDT by Chris Dumez
Modified: 2013-04-03 14:07 PDT (History)
16 users (show)

See Also:


Attachments
Patch (7.72 KB, patch)
2013-04-02 01:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.01 KB, patch)
2013-04-02 14:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Quick prototype to remove BitmapImageSingleFrameSkia (5.35 KB, patch)
2013-04-02 15:32 PDT, Chris Dumez
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64 (387.83 KB, application/zip)
2013-04-02 16:41 PDT, WebKit Review Bot
no flags Details
Patch to remove BitmapImageSingleFrameSkia (62.86 KB, patch)
2013-04-02 16:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (20.16 KB, patch)
2013-04-03 12:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-03-28 02:38:56 PDT
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()
Comment 1 Chris Dumez 2013-03-28 02:40:55 PDT
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.
Comment 2 Chris Dumez 2013-03-28 02:45:17 PDT
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().
Comment 3 Chris Dumez 2013-03-28 04:01:08 PDT
Adding Nico Weber in CC as it seems he wrote this section of createDragImageFromImage() via Bug 100200.
Comment 4 Stephen White 2013-03-28 09:22:07 PDT
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.
Comment 5 Chris Dumez 2013-04-01 14:21:33 PDT
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?
Comment 6 Tom Hudson 2013-04-01 22:45:56 PDT
"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.
Comment 7 Chris Dumez 2013-04-02 00:47:26 PDT
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.
Comment 8 Chris Dumez 2013-04-02 01:17:29 PDT
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.
Comment 9 Stephen White 2013-04-02 08:07:18 PDT
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?
Comment 10 Tom Hudson 2013-04-02 08:51:41 PDT
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()?)
Comment 11 Chris Dumez 2013-04-02 14:04:42 PDT
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?
Comment 12 Chris Dumez 2013-04-02 14:26:03 PDT
Created attachment 196224 [details]
Patch

Rename method to hasBitmapContent() and fix the callers.
Comment 13 Stephen White 2013-04-02 14:40:29 PDT
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?
Comment 14 Chris Dumez 2013-04-02 15:32:32 PDT
Created attachment 196239 [details]
Quick prototype to remove BitmapImageSingleFrameSkia
Comment 15 WebKit Review Bot 2013-04-02 16:41:01 PDT
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
Comment 16 WebKit Review Bot 2013-04-02 16:41:05 PDT
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
Comment 17 Chris Dumez 2013-04-02 16:41:53 PDT
Created attachment 196253 [details]
Patch to remove BitmapImageSingleFrameSkia
Comment 18 Tom Hudson 2013-04-02 23:04:00 PDT
(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.
Comment 19 Stephen White 2013-04-03 11:45:59 PDT
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.
Comment 20 Stephen White 2013-04-03 12:13:53 PDT
(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.
Comment 21 Chris Dumez 2013-04-03 12:39:18 PDT
Created attachment 196392 [details]
Patch

No longer move BitmapImageSkia code to a separate cpp file.
Comment 22 Stephen White 2013-04-03 13:33:33 PDT
Comment on attachment 196392 [details]
Patch

This looks great!  Thanks for the extra effort.  r=me
Comment 23 WebKit Review Bot 2013-04-03 14:06:56 PDT
Comment on attachment 196392 [details]
Patch

Clearing flags on attachment: 196392

Committed r147583: <http://trac.webkit.org/changeset/147583>
Comment 24 WebKit Review Bot 2013-04-03 14:07:05 PDT
All reviewed patches have been landed.  Closing bug.