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-

Dominic Cooney
Reported 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
Attachments
Patch (1.59 KB, patch)
2010-08-25 11:46 PDT, Dominic Cooney 		no flags
DetailsFormatted DiffDiff
Patch (1.64 KB, patch)
2010-08-25 14:07 PDT, Dominic Cooney 		no flags
DetailsFormatted DiffDiff
Patch with early return. (1.60 KB, patch)
2010-08-30 15:03 PDT, Dominic Cooney 		eric: review-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dominic Cooney
Comment 1 2010-08-25 11:46:07 PDT
Created attachment 65452 [details] Patch
Adam Langley
Comment 2 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.
Dominic Cooney
Comment 3 2010-08-25 14:07:32 PDT
Created attachment 65472 [details] Patch
Eric Seidel (no email)
Comment 4 2010-08-27 18:21:44 PDT
Who wrote chromium's getImageData? I suspect this fixes the crash, but is it the right fix?
Stephen White
Comment 5 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.
Dominic Cooney
Comment 6 2010-08-30 15:03:57 PDT
Created attachment 65964 [details] Patch with early return.
Eric Seidel (no email)
Comment 7 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?
James Robinson
Comment 8 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.
Eric Seidel (no email)
Comment 9 2010-09-15 00:14:59 PDT
Comment on attachment 65964 [details] Patch with early return. Tests?
Dominic Cooney
Comment 10 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.
Note You need to log in before you can comment on or make changes to this bug.