Bug 44628

Summary: [Skia] canvas/philip/tests/2d.imageData.get.source.outside.html crashes test_shell in Chromium Linux debug
Product: WebKit Reporter: Dominic Cooney <dominicc>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: brettw, darin, eric, schenney
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 20553    
Attachments:
Description Flags
Patch
none
Patch
none
Patch with early return. eric: review-

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.