Bug 85117 - vertexAttribPointer needs to reject large negative offsets
Summary: vertexAttribPointer needs to reject large negative offsets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
: 85528 (view as bug list)
Depends on: 85722
Blocks:
  Show dependency tree / graph
 
Reported: 2012-04-27 18:39 PDT by Kenneth Russell
Modified: 2012-05-08 15:38 PDT (History)
10 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 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.
Comment 1 Zhenyao Mo 2012-05-04 15:16:26 PDT 
Taking this one.  It's the last bug fix to make chrome webgl 1.0.1 conformant.
Comment 2 Zhenyao Mo 2012-05-04 16:23:46 PDT 
Created attachment 140358 [details]
Patch
Comment 3 Zhenyao Mo 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)
Comment 4 Kenneth Russell 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.
Comment 5 WebKit Review Bot 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>
Comment 6 WebKit Review Bot 2012-05-05 07:51:03 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 7 Andy Estes 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.
Comment 8 Filip Pizlo 2012-05-05 15:52:23 PDT 
Reopening because of the breakage that Andy is talking about.
Comment 9 Filip Pizlo 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.
Comment 10 Zhenyao Mo 2012-05-07 09:36:28 PDT 
Created attachment 140538 [details]
Patch
Comment 11 Zhenyao Mo 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.
Comment 12 Andy Estes 2012-05-07 11:48:03 PDT 
The new patch builds for me on the same machine that couldn't build the last patch.
Comment 13 Zhenyao Mo 2012-05-07 12:57:18 PDT 
Thanks Andy for testing this out.
Comment 14 Kenneth Russell 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.
Comment 15 Zhenyao Mo 2012-05-07 16:56:01 PDT 
Committed r116374: <http://trac.webkit.org/changeset/116374>
Comment 16 Kenneth Russell 2012-05-08 15:38:49 PDT 
*** Bug 85528 has been marked as a duplicate of this bug. ***