Bug 85117

Summary:

vertexAttribPointer needs to reject large negative offsets

Product:

WebKit

Reporter:

Kenneth Russell <kbr>

Component:

WebGL

Assignee:

Zhenyao Mo <zmo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, aestes, dino, fpizlo, gman, jamesr, ojan, oliver, webkit.review.bot, zmo

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

85722

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	kbr: review+

Kenneth Russell

Reported 2012-04-27 18:39:14 PDT

The WebGL conformance test conformance/buffers/index-validation.html is now failing because the test was updated to reflect the spec, which is that negative offsets cause an INVALID_VALUE error to be generated. The test was previously expecting that the large negative offset would be truncated to 32 bits and show up as a positive value. I am not even sure that WebKit's IDL and binding generators can support long long values passed from JavaScript to C++ right now. That might be the bulk of this fix.

Attachments
Patch (16.18 KB, patch) 2012-05-04 16:23 PDT, Zhenyao Mo	no flags	Details Formatted Diff Diff
Patch (21.00 KB, patch) 2012-05-07 09:36 PDT, Zhenyao Mo	kbr: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Zhenyao Mo

Comment 1 2012-05-04 15:16:26 PDT

Taking this one. It's the last bug fix to make chrome webgl 1.0.1 conformant.

Zhenyao Mo

Comment 2 2012-05-04 16:23:46 PDT

Created attachment 140358 [details] Patch

Zhenyao Mo

Comment 3 2012-05-04 16:25:33 PDT

Tested in chromium and webkit on Mac. The test is synced from khronos. Please have a look. (I used long long in place of GLsizeiptr and GLintptr to be consistent with the types used in the bindings)

Kenneth Russell

Comment 4 2012-05-04 16:33:05 PDT

Comment on attachment 140358 [details] Patch Looks good. r=me Let's wait for it to clear the EWS before committing though.

WebKit Review Bot

Comment 5 2012-05-05 07:50:58 PDT

Comment on attachment 140358 [details] Patch Clearing flags on attachment: 140358 Committed r116221: <http://trac.webkit.org/changeset/116221>

WebKit Review Bot

Comment 6 2012-05-05 07:51:03 PDT

All reviewed patches have been landed. Closing bug.

Andy Estes

Comment 7 2012-05-05 13:15:02 PDT

This breaks the build on platforms that compile with -Wshorten-64-to-32, since there are now several call sites that pass a long long to a function that expects a GC3Dintptr.

Filip Pizlo

Comment 8 2012-05-05 15:52:23 PDT

Reopening because of the breakage that Andy is talking about.

Filip Pizlo

Comment 9 2012-05-05 15:55:41 PDT

Sorry, but I have to roll this out. :-( Please let me know if you need assistance debugging the build errors we're seeing.

Zhenyao Mo

Comment 10 2012-05-07 09:36:28 PDT

Created attachment 140538 [details] Patch

Zhenyao Mo

Comment 11 2012-05-07 09:51:04 PDT

Comment on attachment 140538 [details] Patch Ken, I added explicit converting from long long to GC3D types to avoid compiling failures on 32. Please have another look.

Andy Estes

Comment 12 2012-05-07 11:48:03 PDT

The new patch builds for me on the same machine that couldn't build the last patch.

Zhenyao Mo

Comment 13 2012-05-07 12:57:18 PDT

Thanks Andy for testing this out.

Kenneth Russell

Comment 14 2012-05-07 13:03:40 PDT

Comment on attachment 140538 [details] Patch New patch looks OK as long as it compiles. It's a little unfortunate that we lose the ability to change the signatures here with one typedef.

Zhenyao Mo

Comment 15 2012-05-07 16:56:01 PDT

Committed r116374: <http://trac.webkit.org/changeset/116374>

Kenneth Russell

Comment 16 2012-05-08 15:38:49 PDT

*** Bug 85528 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.