Bug 44628 - [Skia] canvas/philip/tests/2d.imageData.get.source.outside.html crashes test_shell in Chromium Linux debug
Summary: [Skia] canvas/philip/tests/2d.imageData.get.source.outside.html crashes test_...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 20553
  Show dependency treegraph
 
Reported: 2010-08-25 11:31 PDT by Dominic Cooney
Modified: 2013-04-09 12:53 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2010-08-25 11:46 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2010-08-25 14:07 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch with early return. (1.60 KB, patch)
2010-08-30 15:03 PDT, Dominic Cooney
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2010-08-25 11:31:08 PDT
canvas/philip/tests/2d.imageData.get.source.outside.html asserts in test_shell in Chromium Linux debug:

        StackTrace::StackTrace() [0x590d5e]                                     
        logging::LogMessage::~LogMessage() [0x536799]                           
        SkDebugf_FileLine() [0x4c06d5]                                          
        SkBitmap::getAddr32() [0x468725]                                        
        WebCore::getImageData<>() [0xd09daa]                                    
        WebCore::ImageBuffer::getUnmultipliedImageData() [0xd09617]             
        WebCore::CanvasRenderingContext2D::getImageData() [0x10fd10a]           
        WebCore::CanvasRenderingContext2DInternal::getImageDataCallback() [0x130a68c]                                                                          
        v8::internal::HandleApiCallHelper<>() [0x68cb60]                        
        v8::internal::Builtin_Impl_HandleApiCall() [0x68a795]                   
        v8::internal::Builtin_HandleApiCall() [0x68a76e]                        
        0x7f72d75e61aa
Comment 1 Dominic Cooney 2010-08-25 11:46:07 PDT
Created attachment 65452 [details]
Patch
Comment 2 Adam Langley 2010-08-25 12:08:23 PDT
Comment on attachment 65452 [details]
Patch

LGTM once the point below has been addressed. Note: I am not a WebKit Reviewer. You need a real review from someone else before landing this patch.

> +        In Chromium Skia getImageData don't retrieve the pointer into the
> +        canvas bitmap when there are no columns to copy, because this
> +        asserts when the source is outside the canvas bounds.

This sentence doesn't make sense.
Comment 3 Dominic Cooney 2010-08-25 14:07:32 PDT
Created attachment 65472 [details]
Patch
Comment 4 Eric Seidel (no email) 2010-08-27 18:21:44 PDT
Who wrote chromium's getImageData?  I suspect this fixes the crash, but is it the right fix?
Comment 5 Stephen White 2010-08-30 10:31:08 PDT
Comment on attachment 65472 [details]
Patch

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:181
> +    int numRows = numColumns > 0 ? endY - originY : 0;
If this function is meant to do nothing in this case (and I'm guessing it is), I'd put an early-return below the computation of numColumns, rather than making the numRows computation more confusing.
Comment 6 Dominic Cooney 2010-08-30 15:03:57 PDT
Created attachment 65964 [details]
Patch with early return.
Comment 7 Eric Seidel (no email) 2010-08-30 15:10:20 PDT
Comment on attachment 65964 [details]
Patch with early return.

Looks fine, but why wouldn't we update test_expectations.txt in the same commit?
Comment 8 James Robinson 2010-09-07 16:39:05 PDT
Comment on attachment 65964 [details]
Patch with early return.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +
> +        Fixes getImageData in the Chromium Skia port so that it doesn't
> +        retrieve the pointer into the canvas bitmap when there are no
> +        columns to copy. Retrieving the pointer asserts when the source is
> +        outside the canvas bounds.
> +

The double negative here is confusing.  Can you please reword this sentence?

Otherwise, looks great.  canvas/philip/ is still marked as SKIP in the chromium test_expectations so there's not much point in updating it at this point.
Comment 9 Eric Seidel (no email) 2010-09-15 00:14:59 PDT
Comment on attachment 65964 [details]
Patch with early return.

Tests?
Comment 10 Dominic Cooney 2010-09-15 00:29:19 PDT
I plan to baseline philip in a separate CL, then add updated expectations into this patch. But not actively working on this right now.