Closed Bug 867329 Opened 7 years ago Closed 7 years ago

"Assertion failure: nelements <= 0x7fffffff" with canvas createImageData

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jruderman, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file testcase
Assertion failure: nelements <= 0x7fffffff, at js/src/jstypedarray.cpp:3401

(Same assertion as bug 826667, different testcase.)
Attached file stack
Bug 748426 has another canvas testcase that trips this assertion.  It uses getImageData while this one uses createImageData.
Depends on: 748426
Er, yes.  Either the JS engine needs to do that check and bail, or it should be documented which APIs expect the check to be done and then the binding code should....
There was no justification for the JS_New##Name##Array.  Just making every parameter uint32_t is fine, because createBufferWithSizeAndCount tests that the count won't cause total memory to exceed int32_t.  (It's possible that int32_t restriction no longer matters any more -- now that LENGTH_SLOT, BYTELENGTH_SLOT, and BYTEOFFSET_SLOT aren't exposed as actual values, we could probably use PrivateUint32Value for them, and have them have uint32_t range.  But that's definitely a separate bug, and that assumes there's no complications I haven't thought of.)

The second case is trickier because some internal APIs are overloading for -1 being an acceptable value.  I figured I'd get this patch out, tho, and leave the harder part to another patch.
Attachment #743991 - Flags: review?(sphink)
Assignee: general → jwalden+bmo
(Why must bzexport slurp up the patch *before* I close the editor?  :-(  Seems like I always think of something else while typing up the bug comment, and I guess editing/refreshing the revision underneath bzexport won't have it picked up by bzexport.)
Attachment #743993 - Flags: review?(sphink)
Attachment #743991 - Attachment is obsolete: true
Attachment #743991 - Flags: review?(sphink)
Actually that wasn't so bad, either.  (lengthInt is the messy argument, but that's not asserted to be anything here.)  Given byteOffset == -1 is interpreted as byteOffset == 0, I am a bit befuddled as to why byteOffset == -1 was ever supposed to be the default that triggered special behavior.  This passes jstests, jit-tests, and jsapi-tests.  I'll try both these before pushing them, of course.
Attachment #744013 - Flags: review?(sphink)
Comment on attachment 744013 [details] [diff] [review]
Make JS_NewUint8ArrayWithBuffer and friends accept any uint32_t as byteOffset, throwing if overlarge

Review of attachment 744013 [details] [diff] [review]:
-----------------------------------------------------------------

I thought I had gotten rid of the pointless uses of -1 as the special value, but I guess not.

Anyway, thanks for doing this. I've been procrastinating on the original report of this bug for a while. Looks good to me.
Attachment #744013 - Flags: review?(sphink) → review+
Attachment #743993 - Flags: review?(sphink) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/06962a458da3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d894bdbe5856
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla23
Duplicate of this bug: 826667
You need to log in before you can comment on or make changes to this bug.