Closed Bug 558541 Opened 15 years ago Closed 15 years ago

js_SetPropertyHelper doesn't notify tracer when setting a non-writable property with strict option on or "Assertion failure: s0->isQuad(), at ../jstracer.cpp" or "Assertion failure: v_ins->isF64(), at ../jstracer.cpp"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+
blocking1.9.2 --- .14+
status1.9.2 --- .14-fixed
blocking1.9.1 --- .17+
status1.9.1 --- .17-fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical?])

Attachments

(4 files, 1 obsolete file)

Shell testcase, crashes with gnarly low-level assertion: options("strict"); for (var i = 0; i < 5; i++) Boolean.prototype = 42; Unlike bug 558249 (fallout from bug 550402), this goes back a ways -- I can reproduce the problem with a 3.6 tree. Strangely, I have trouble causing a failure inside the browser with javascript.options.strict set to true; not sure what's up there, but the shell behavior is more than enough to make me worry about the browser as well. I noticed this while working on the fix for bug 558249, but since it's applicable a ways back it seems better to separate it out into this bug -- let that one take care of the shorter-term regression.
autoBisect shows this is probably related to bug 490666: The first bad revision is: changeset: 27505:3519ddacbe2e user: Andreas Gal date: Thu Apr 30 15:52:13 2009 -0700 summary: We don't cache access to shared properties in the property cache (490666, r=igor,brendan). I had to follow this method: (cat | js -j -i or by direct typing/copy+paste) to get the testcase to crash as per bug 558249 comment 3. This might be a reason why jsfunfuzz didn't find it beforehand, though I might be wrong.
Blocks: 490666
This used to assert at: Assertion failure: s0->isQuad(), at ../jstracer.cpp but now seems to assert at: Assertion failure: v_ins->isF64(), at ../jstracer.cpp:9347
Summary: js_SetPropertyHelper doesn't notify tracer when setting a non-writable property with strict option on → js_SetPropertyHelper doesn't notify tracer when setting a non-writable property with strict option on or "Assertion failure: s0->isQuad(), at ../jstracer.cpp" or "Assertion failure: v_ins->isF64(), at ../jstracer.cpp"
can we get an ETA?
I saw the patch fly by recently... bug 558249 has it. Is this a dup? /be
Attached patch PatchSplinter Review
The test has the same problem as the test in the bug Brendan just mentioned (this isn't a dup, btw). It's basically hopeless to attempt to obfuscate the intent of this patch. Maybe a diversionary bug of some sort might work, conceivably. Not sure what would be a good diversion that doesn't spill the beans completely -- ideas welcome.
Attachment #438835 - Flags: review?(jorendorff)
Attachment #438835 - Flags: review?(jorendorff) → review+
Patch landed, backed out for much orange -- to be investigated.
blocking2.0: --- → beta1+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
This made its way into TM successfully, then got ported to m-c in the changeset in comment 7. Still needs backporting to 192 and 191... :-\
status1.9.1: --- → ?
status1.9.2: --- → ?
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking1.9.1: ? → .17+
blocking1.9.2: ? → .14+
Attached patch Potential 191 patch (obsolete) — Splinter Review
So, I tweaked the patch to apply to branches -- they're what I just uploaded. Porting was fairly easy, I think, although the code through there had changed somewhat (but not too much, if my transformation was correct -- see below). I tried to rerun the original testcase and verify failure before, pass after. Neither patch fails before and passes after. I don't know how to explain this. And, because I've been awake since 5:20 EST and it's now 15:55 PST (18:55 EST) due to a BOS->SFO flight today (and I didn't get a full night's sleep last night), I am not at my most lucid right now, nor do I believe I could really do this full justice if I were to investigate. And being confident enough to push? It just ain't gonna happen today, at least not for me personally. But the freeze is today, so something needs to be done. I'll see if I can hunt anyone down who can help me out here.
Attached patch 191 patchSplinter Review
Okay, I had dmandelin look at this, and on *his* system the test was spewing the wrong number of warnings pre-patch and the right number post, so on that basis he thinks it's good. So I think this is good to go.
Attachment #504884 - Attachment is obsolete: true
Attached patch 192 patchSplinter Review
...and the 192 patch.
Tests landed in TM, since this is fixed in branches now: http://hg.mozilla.org/tracemonkey/rev/2d196b5a4224
Flags: in-testsuite+
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: