in "first + count"
1) in drawArrays, possible overflow for (first + count) 2) drawArrays(first, count=0) where first is out-of-range should generate INVALID_OPERATION. 3) drawElements(count=0, offset=0) with buffer of size 0 should not generate an error.
Created attachment 60295 [details] patch CheckedInt.h is imported from mozilla as recommended by kbr to deal with integer overflow situation. Modification is made to the file to remove further dependencies.
Attachment 60295 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 Last 3072 characters of output: use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:214: twice_bigger_type_is_supported is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:219: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:230: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:241: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:247: More than one command on the same line in if [whitespace/parens] [4] WebCore/html/canvas/CheckedInt.h:247: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:250: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/html/canvas/CheckedInt.h:255: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/html/canvas/CheckedInt.h:265: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:269: More than one command on the same line in if [whitespace/parens] [4] WebCore/html/canvas/CheckedInt.h:269: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:283: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:285: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:340: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:409: CheckedInt_internal::is_sub_valid is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:468: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:469: return_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:475: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:476: return_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:490: COMPOUND_OP is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:509: Missing spaces around / [whitespace/operators] [3] Total errors found: 39 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 60295 [details] patch Looks good. I think we need Darin's feedback on whether it's okay to incorporate CheckedInt verbatim. In order to be able to take future updates from Mozilla I really think we should not force the code into WebKit style. One comment. WebCore/html/canvas/WebGLRenderingContext.cpp:859 + if (!(checkedFirst + checkedCount).valid() || !validateRenderingState(first + count)) { How about mozilla::CheckedInt checkedSum = checkedFirst + checkedCount; if (!checkedSum.valid() || !validateRenderingState(checkedSum.value()) {
WebKit has several other source files imported from Mozilla. I don't know what the process is for adding more. Preserving the original formatting and style of the code makes sense to me, but we might want to make the interface more WebKit friendly. For example, instead of the mozilla namespace, perhaps we'd want to just have this code live in the WebCore namespace. That way instead of writing: mozilla::CheckedInt<int32_t> checkedCount(count); We could just write: CheckedInt<int32_t> checkedCount(count); I doubt the cost of renaming the namespace would cause that much of a merge headache, especially considering the other changes that were needed to support the removal of prtypes.h.
(In reply to comment #5) > Preserving the original formatting and style of the code makes sense to me, but we might want to make the interface more WebKit friendly. For example, instead of the mozilla namespace, perhaps we'd want to just have this code live in the WebCore namespace. Agree with renaming the namespace -- usage is tedious otherwise. BTW, the only reason we placed this class here was to keep it close to its only current uses. We plan to use it more in the WebGL implementation. Open to suggestions of where to incorporate it in the tree. (Hoping there aren't any issues doing so -- Mozilla encouraged us to integrate it.)
Created attachment 60316 [details] revised patch
Attachment 60316 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 Last 3072 characters of output: use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:215: twice_bigger_type_is_supported is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:220: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:231: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:242: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:248: More than one command on the same line in if [whitespace/parens] [4] WebCore/html/canvas/CheckedInt.h:248: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:251: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/html/canvas/CheckedInt.h:256: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/html/canvas/CheckedInt.h:266: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:270: More than one command on the same line in if [whitespace/parens] [4] WebCore/html/canvas/CheckedInt.h:270: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:284: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:286: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:341: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:410: CheckedInt_internal::is_sub_valid is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:469: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:470: return_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:476: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:477: return_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:491: COMPOUND_OP is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:510: Missing spaces around / [whitespace/operators] [3] Total errors found: 39 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 60316 [details] revised patch Looks good to me.
Comment on attachment 60316 [details] revised patch WebCore/html/canvas/CheckedInt.h:44 + #ifndef mozilla_CheckedInt_h one more nit: let's rename this include guard to be CheckedInt_h to match WebKit style. it seems valuable to follow this rule since it is related to the naming conventions for files. otherwise, r=me
Created attachment 60373 [details] revised patch: changed define to CheckedInt_h
Comment on attachment 60373 [details] revised patch: changed define to CheckedInt_h thanks, r=me
Comment on attachment 60373 [details] revised patch: changed define to CheckedInt_h Rejecting patch 60373 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1 Last 500 characters of output: (offset 18 lines). Hunk #3 succeeded at 871 (offset 18 lines). Hunk #4 succeeded at 915 (offset 26 lines). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/canvas/webgl/draw-arrays-out-of-bounds-expected.txt patching file LayoutTests/fast/canvas/webgl/draw-arrays-out-of-bounds.html patching file LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds-expected.txt patching file LayoutTests/fast/canvas/webgl/draw-elements-out-of-bounds.html Full output: http://webkit-commit-queue.appspot.com/results/3410090
Created attachment 60382 [details] revised patch: resolve a svn merge conflict
Attachment 60382 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 Last 3072 characters of output: use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:216: twice_bigger_type_is_supported is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:221: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:232: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:243: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:249: More than one command on the same line in if [whitespace/parens] [4] WebCore/html/canvas/CheckedInt.h:249: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:252: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/html/canvas/CheckedInt.h:257: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/html/canvas/CheckedInt.h:267: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:271: More than one command on the same line in if [whitespace/parens] [4] WebCore/html/canvas/CheckedInt.h:271: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:285: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:287: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/html/canvas/CheckedInt.h:342: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:411: CheckedInt_internal::is_sub_valid is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:470: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:471: return_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:477: This { should be at the end of the previous line [whitespace/braces] [4] WebCore/html/canvas/CheckedInt.h:478: return_type is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:492: COMPOUND_OP is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/html/canvas/CheckedInt.h:511: Missing spaces around / [whitespace/operators] [3] Total errors found: 38 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 60382 [details] revised patch: resolve a svn merge conflict once more.
Comment on attachment 60382 [details] revised patch: resolve a svn merge conflict Clearing flags on attachment: 60382 Committed r62397: <http://trac.webkit.org/changeset/62397>
All reviewed patches have been landed. Closing bug.
Hi, I wanted to let you know that we also have an extensive unit test for CheckedInt, in case you might want to import that too: http://hg.mozilla.org/mozilla-central/file/tip/xpcom/tests/TestCheckedInt.cpp