Closed Bug 438415 Opened 12 years ago Closed 12 years ago

"Assertion failure: *vp != JSVAL_HOLE" at nxp.com

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: crowderbt)

References

()

Details

(Keywords: assertion, verified1.9.0.2)

Attachments

(2 files, 2 obsolete files)

Steps to reproduce:
1. Load http://www.nxp.com/ in a debug build of Firefox.
2. Wait a few seconds for some AJAXy stuff to happen.

Result:
Assertion failure: *vp != JSVAL_HOLE, at /Users/jruderman/central/js/src/jsarray.cpp:2084

I'm not sure how to go about making a reduced testcase.  If I load a local copy of the page, it gets a security error when it tries to use XHR.
Coincidentally, I just got the same assertion with jsfunfuzz.  Here's the testcase reduced from the jsfunfuzz output:

[1,,].pop();

With any luck, that's the same bug that happens on nxp.com :)
Attached patch Use GetArrayElement in array_pop (obsolete) — Splinter Review
The original array_pop here diverged from the spec in also failing to search up the scope chain for holes encountered while popping.  This fixes both issues.
Assignee: general → crowder
Status: NEW → ASSIGNED
Attached patch use DeleteArrayElement, too (obsolete) — Splinter Review
Realized I was reinventing the DeleteArrayElement wheel as I played with this more.
Attachment #324530 - Attachment is obsolete: true
Comment on attachment 324532 [details] [diff] [review]
use DeleteArrayElement, too

This passes js/tests
Attachment #324532 - Flags: review?(shaver)
Review-ping.
Comment on attachment 324532 [details] [diff] [review]
use DeleteArrayElement, too

r=shaver.  What does this do to perf, if anything?
Attachment #324532 - Flags: review?(shaver) → review+
Comment on attachment 324532 [details] [diff] [review]
use DeleteArrayElement, too

No return value checking? Suppressed errors leave exceptions pending...

/be
meaculpa=shaver, too.  Should propagate errors there, and I should have caught it, having just been chasing something similar in a local tracemonkey hack!

Sorry.
Attachment #324532 - Attachment is obsolete: true
Attachment #327490 - Flags: review?(brendan)
Flags: wanted1.9.0.x?
Comment on attachment 327490 [details] [diff] [review]
now with ERROR checking!

No need to request review for obvious fix.

/be
Attachment #327490 - Flags: review?(brendan) → review+
Attachment #327490 - Flags: approval1.9.0.2?
changeset:   15603:886099dc030c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #329518 - Flags: approval1.9.0.2?
Attachment #327490 - Flags: approval1.9.0.2?
Would love of a test here, if it's possible, prior to approving for 1.9.0.x.
Flags: in-testsuite?
The testcase from comment #1 is good.  A testcase that checks pop() for getting a prop from the proto would be nice, too.  bclary:  I'll write these, if you like?
I'm working on docs for adding new tests while adding new tests today, so I can go ahead and do the paper work, but an short example of what you mean by "checks pop() for getting a prop from the proto" would be great.
Whiteboard: [needs test]
Checking in regress-438415-01.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-438415-01.js,v  <--  regress-438415-01.js
initial revision: 1.1

mozilla-central: changeset:   16236:fcdfa3729ed5
Flags: in-testsuite? → in-testsuite+
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Keywords: qawanted
Whiteboard: [needs test]
Comment on attachment 329518 [details] [diff] [review]
applied to 1.9 branch

Approved for 1.9.0.2. Please land in CVS. a=ss

Bob, thanks for the test!
Attachment #329518 - Flags: approval1.9.0.2? → approval1.9.0.2+
bc: In answer to your question from comment #15:

js> Array.prototype[0] = "zero"
zero
js> a = []

js> a.length = 1
1
js> a.pop()
zero

Keywords: fixed1.9.0.2
Flags: in-testsuite+ → in-testsuite?
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-438415-02.js,v  <--  regress-438415-02.js
initial revision: 1.1

mozilla-central: changeset:   16419:e3afa3b996e1
Flags: in-testsuite? → in-testsuite+
verified fixed 1.9.0/trunk linux/mac/win 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.