Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(37)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1234083003: Canvas.toDataURL to use SkBitmap::readPixels to avoid uninitialized memory (Closed)

Created:
5 years, 5 months ago by Justin Novosad
Modified:
5 years, 5 months ago
CC:
blink-reviews, krit, blink-reviews-html_chromium.org, drott+blinkwatch_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, danakj, dglazkov+blink, Rik, f(malita), Stephen Chennney, rwlbuis
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Canvas.toDataURL to use SkBitmap::readPixels to avoid uninitialized memory This change refactors toDataURL to make it read the canvas data into a pre-initialized buffer to avoid accessing unitialized memory in cases where a GPU readback fails silently. BUG=504690 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199131 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199183

Patch Set 1 #

Total comments: 1

Patch Set 2 : rearranged #

Total comments: 1

Patch Set 3 : added toImageData #

Total comments: 1

Patch Set 4 : whoops fix #

Patch Set 5 : fixed asan error in webp encoder #

Total comments: 1

Patch Set 6 : batter asan fix #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -30 lines) Patch
M Source/core/html/HTMLCanvasElement.h View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/html/HTMLCanvasElement.cpp View 1 2 3 1 chunk +29 lines, -14 lines 0 comments Download
M Source/platform/graphics/ImageBuffer.h View 1 chunk +0 lines, -1 line 0 comments Download
M Source/platform/graphics/ImageBuffer.cpp View 2 chunks +1 line, -13 lines 0 comments Download
M Source/platform/image-encoders/skia/WEBPImageEncoder.cpp View 1 2 3 4 5 1 chunk +1 line, -2 lines 1 comment Download

Depends on Patchset:

Issue 895153002 Patch 100001

Messages

Total messages: 24 (4 generated)
Justin Novosad
PTAL
5 years, 5 months ago (2015-07-16 17:06:46 UTC) #2
Stephen White
https://codereview.chromium.org/1234083003/diff/1/Source/core/html/HTMLCanvasElement.cpp File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/1234083003/diff/1/Source/core/html/HTMLCanvasElement.cpp#newcode493 Source/core/html/HTMLCanvasElement.cpp:493: ScopedDisposal<ImageData> disposer(imageData); As discussed, let's see if we can ...
5 years, 5 months ago (2015-07-16 17:49:22 UTC) #3
Justin Novosad
new patch
5 years, 5 months ago (2015-07-16 18:08:03 UTC) #4
Stephen White
https://codereview.chromium.org/1234083003/diff/20001/Source/core/html/HTMLCanvasElement.cpp File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/1234083003/diff/20001/Source/core/html/HTMLCanvasElement.cpp#newcode477 Source/core/html/HTMLCanvasElement.cpp:477: if (imageData) { I think this would be clearer ...
5 years, 5 months ago (2015-07-16 19:13:01 UTC) #5
Justin Novosad
new patch
5 years, 5 months ago (2015-07-16 21:06:41 UTC) #6
Stephen White
https://codereview.chromium.org/1234083003/diff/40001/Source/core/html/HTMLCanvasElement.cpp File Source/core/html/HTMLCanvasElement.cpp (right): https://codereview.chromium.org/1234083003/diff/40001/Source/core/html/HTMLCanvasElement.cpp#newcode483 Source/core/html/HTMLCanvasElement.cpp:483: ScopedDisposal<ImageData> disposer(imageData); It looks like this ScopedDisposal is going ...
5 years, 5 months ago (2015-07-16 22:09:41 UTC) #7
Justin Novosad
On 2015/07/16 22:09:41, Stephen White wrote: > https://codereview.chromium.org/1234083003/diff/40001/Source/core/html/HTMLCanvasElement.cpp > File Source/core/html/HTMLCanvasElement.cpp (right): > > https://codereview.chromium.org/1234083003/diff/40001/Source/core/html/HTMLCanvasElement.cpp#newcode483 ...
5 years, 5 months ago (2015-07-17 15:14:28 UTC) #8
Justin Novosad
new patch
5 years, 5 months ago (2015-07-17 15:18:43 UTC) #9
Stephen White
LGTM
5 years, 5 months ago (2015-07-17 17:12:39 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234083003/60001
5 years, 5 months ago (2015-07-17 17:13:23 UTC) #12
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199131
5 years, 5 months ago (2015-07-17 19:35:36 UTC) #13
sof
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1236173005/ by sigbjornf@opera.com. ...
5 years, 5 months ago (2015-07-18 06:45:57 UTC) #14
Justin Novosad
New patch. fixed ASAN error
5 years, 5 months ago (2015-07-20 15:28:47 UTC) #15
Stephen White
https://codereview.chromium.org/1234083003/diff/80001/Source/platform/image-encoders/skia/WEBPImageEncoder.cpp File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): https://codereview.chromium.org/1234083003/diff/80001/Source/platform/image-encoders/skia/WEBPImageEncoder.cpp#newcode59 Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:59: rgb.resize(pixelCount * 3); As discussed, could be cleaner to ...
5 years, 5 months ago (2015-07-20 15:33:10 UTC) #16
Justin Novosad
On 2015/07/20 15:33:10, Stephen White wrote: > https://codereview.chromium.org/1234083003/diff/80001/Source/platform/image-encoders/skia/WEBPImageEncoder.cpp > File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): > > https://codereview.chromium.org/1234083003/diff/80001/Source/platform/image-encoders/skia/WEBPImageEncoder.cpp#newcode59 ...
5 years, 5 months ago (2015-07-20 15:36:49 UTC) #17
Stephen White
LGTM
5 years, 5 months ago (2015-07-20 15:37:55 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1234083003/100001
5 years, 5 months ago (2015-07-20 20:14:54 UTC) #20
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=199183
5 years, 5 months ago (2015-07-20 20:22:18 UTC) #21
Noel Gordon
https://codereview.chromium.org/1234083003/diff/100001/Source/platform/image-encoders/skia/WEBPImageEncoder.cpp File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right): https://codereview.chromium.org/1234083003/diff/100001/Source/platform/image-encoders/skia/WEBPImageEncoder.cpp#newcode58 Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:58: Overall patch seems fine, but just to confirm, there ...
5 years, 5 months ago (2015-07-21 00:48:36 UTC) #23
Justin Novosad
5 years, 5 months ago (2015-07-22 21:09:58 UTC) #24
Message was sent while issue was closed. 
On 2015/07/21 00:48:36, noel gordon wrote:
>
https://codereview.chromium.org/1234083003/diff/100001/Source/platform/image-...
> File Source/platform/image-encoders/skia/WEBPImageEncoder.cpp (right):
> 
>
https://codereview.chromium.org/1234083003/diff/100001/Source/platform/image-...
> Source/platform/image-encoders/skia/WEBPImageEncoder.cpp:58: 
> Overall patch seems fine, but just to confirm, there was a perf hit here?

Yes. But it is not critical. The cost of the readback from the GPU and the cost
of encoding will dwarf the cost of precociously initializing the buffer.

Powered by Google App Engine
This is Rietveld 408576698