Bug 112354 – [BlackBerry] Store floating point images in correctly sized vector.

• Status: RESOLVED
• Resolution: INVALID
• Component: WebGL

[Jonathan Feldstein]

Resolves 3 WebGL conformance suite crashes, namely:
- oes-texture-float-with-canvas
- oes-texture-float-with-image
- oes-texture-float-with-video

This bug occurs as a result of trying to create a texture with
texImage2D with either a provided floating point canvas, image,
or video. The issue occurs because a vector used to store this
texture is resized to width * height * 4 which is large enough
for non-floating point images, but is not large enough for
floating ones. ie. an RGBA floating point image is of size:
width * height * 4 * sizeof(float). As a result of having a
vector that is too small to hold the image contents, we end up
writing beyond the end of the vector which results in a variety
of different crashes depending on where in memory we end up
writing to. Additionally, needless amount of space are
allocated for smaller images. ie. an RGB image of unsigned
bytes only requires width * height * 3 bytes of space. This
commit also makes sure that the vector is resized to the exact
size that is needed to fit the provided image.

[Jonathan Feldstein]

Created attachment 193130 [details]
Patch

[Rob Buis]

Comment on attachment 193130 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193130&action=review

> Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:427
> +            sizeofChannels *= 1;

I guess this is not needed. Maybe the compiler does not generate the instructions though.

> Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:454
> +            sizeofChannels *= 1;

Ditto.

[Rob Buis]

Comment on attachment 193130 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193130&action=review

> Source/WebCore/ChangeLog:3
> +        [BlackBerry] Resolves 3 WebGL conformance suite crashes.

I think the bug title can be improved.

[Jonathan Feldstein]

(In reply to comment #2)
> (From update of attachment 193130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193130&action=review
> 
> > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:427
> > +            sizeofChannels *= 1;
> 
> I guess this is not needed. Maybe the compiler does not generate the instructions though.
> 
> > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:454
> > +            sizeofChannels *= 1;
> 
> Ditto.

You're definitely correct that it's needless. I was trying to be more clear about what's going on. Perhaps I could add case statements which simply break immediately without doing a multiply. Is that a fair compromise?

[Rob Buis]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 193130 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=193130&action=review
> > 
> > > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:427
> > > +            sizeofChannels *= 1;
> > 
> > I guess this is not needed. Maybe the compiler does not generate the instructions though.
> > 
> > > Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp:454
> > > +            sizeofChannels *= 1;
> > 
> > Ditto.
> 
> You're definitely correct that it's needless. I was trying to be more clear about what's going on. Perhaps I could add case statements which simply break immediately without doing a multiply. Is that a fair compromise?

Sounds good :)

[Jonathan Feldstein]

Created attachment 193136 [details]
Patch2

Removed code that needlessly multiplied by one.

[Jonathan Feldstein]

Created attachment 193142 [details]
Patch3

[Rob Buis]

Comment on attachment 193142 [details]
Patch3

Nice fix.

[WebKit Review Bot]

Comment on attachment 193142 [details]
Patch3

Rejecting attachment 193142 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 193142, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
ffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp
Hunk #1 FAILED at 411.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/platform/graphics/blackberry/GraphicsContext3DBlackBerry.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Rob Buis']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17041591