Last Comment Bug 558541 - 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"
: js_SetPropertyHelper doesn't notify tracer when setting a non-writable proper...
Status: RESOLVED FIXED
: [sg:critical?]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Jeff Walden (remove +bmo to email)
: general
:
:
: 490666
  Show dependency treegraph
 
Reported: 2010-04-10 05:17 PDT by Jeff Walden (remove +bmo to email)
Modified: 2011-03-29 19:31 PDT (History)
8 users (show)
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1+
  ---
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
Patch (2.09 KB, patch)
2010-04-13 13:38 PDT, Jeff Walden (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
Potential 192 patch (1.86 KB, patch)
2011-01-18 15:51 PST, Jeff Walden (remove +bmo to email)
no flags Details | Diff | Splinter Review
Potential 191 patch (1.86 KB, patch)
2011-01-18 15:57 PST, Jeff Walden (remove +bmo to email)
no flags Details | Diff | Splinter Review
191 patch (1.86 KB, patch)
2011-01-18 16:18 PST, Jeff Walden (remove +bmo to email)
no flags Details | Diff | Splinter Review
192 patch (1.88 KB, patch)
2011-01-18 16:19 PST, Jeff Walden (remove +bmo to email)
no flags Details | Diff | Splinter Review

Summon comment box

Description Jeff Walden (remove +bmo to email) 2010-04-10 05:17:31 PDT
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.
Comment 1 Gary Kwong [:nth10sd] 2010-04-10 05:55:27 PDT
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.
Comment 2 Gary Kwong [:nth10sd] 2010-04-10 05:58:09 PDT
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
Comment 3 Robert Sayre 2010-04-13 13:19:24 PDT
can we get an ETA?
Comment 4 Brendan Eich [:brendan] 2010-04-13 13:26:33 PDT
I saw the patch fly by recently... bug 558249 has it. Is this a dup?

/be
Comment 5 Jeff Walden (remove +bmo to email) 2010-04-13 13:38:00 PDT
Created attachment 438835 [details] [review]
Patch

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.
Comment 6 Jeff Walden (remove +bmo to email) 2010-04-20 13:40:51 PDT
Patch landed, backed out for much orange -- to be investigated.
Comment 8 Jeff Walden (remove +bmo to email) 2010-04-26 18:47:22 PDT
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...  :-\
Comment 9 Jeff Walden (remove +bmo to email) 2011-01-18 15:51:44 PST
Created attachment 504878 [details] [review]
Potential 192 patch
Comment 10 Jeff Walden (remove +bmo to email) 2011-01-18 15:57:38 PST
Created attachment 504884 [details] [review]
Potential 191 patch

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.
Comment 11 Jeff Walden (remove +bmo to email) 2011-01-18 16:18:49 PST
Created attachment 504896 [details] [review]
191 patch

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.
Comment 12 Jeff Walden (remove +bmo to email) 2011-01-18 16:19:20 PST
Created attachment 504897 [details] [review]
192 patch

...and the 192 patch.
Comment 14 Jeff Walden (remove +bmo to email) 2011-03-09 22:02:38 PST
Tests landed in TM, since this is fixed in branches now:

http://hg.mozilla.org/tracemonkey/rev/2d196b5a4224

Note You need to log in before you can comment on or make changes to this bug.