602441 - JM: Botched extensibility checking when attempting to add a new property to an object

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Method JIT:

[jwalden@find-waldo-now src]$ dbg/js -j -m
js> var o = Object.preventExtensions({});
js> var output = [];
js> for (var i = 0; i < 5; i++) try { o.u = ""; output.push(i + ": no exception"); } catch (e) { output.push(i + ": " + e); }
5
js> print(output.join("\n"));
0: no exception
1: TypeError: o.u is not extensible
2: no exception
3: no exception
4: no exception

Tracing only:

[jwalden@find-waldo-now src]$ dbg/js -j
js> var o = Object.preventExtensions({});
js> var output = [];
js> for (var i = 0; i < 5; i++) try { o.u = ""; output.push(i + ": no exception"); } catch (e) { output.push(i + ": " + e); }
5
js> print(output.join("\n"));
0: no exception
1: no exception
2: no exception
3: no exception
4: no exception

Tracing is correct: only in strict mode should we be throwing a TypeError.

This is only observable a tree with bug 600128 fixed (should land tomorrow).  Note also that that bug's test includes a non-chatty version of the above example (using freeze rather than preventExtensions -- the former subsumes the latter's minimal lockdown).  When it lands, that test will be disabled in browser (where it fails) and partially disabled in shell when the methodjit option's set, so it'll need to be adjusted when fixing this bug.

Comment 1

[Jeff Walden [:Waldo]]

Looking at the revision implementing extensibility:

http://hg.mozilla.org/tracemonkey/rev/aa9b86572020

I see I flat-out removed some if-sealed code from the SetPropCompiler implementation, which seems...likely wrong.  Betting that's the thing to fix, will kick off a build with the likely fix to test in the morning...

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 1 - Patch and test (also fixes a second bug!)

Yay for tests!  In trying to be more careful about getting all the edge cases when test-writing, I stumbled on an improper assertion of extensibility (that's what the assertion move is all about) -- so we get two fixes in one here, of course with testing goodness.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 2: Remove dead property-setting property-cache code

In looking at other potentially dodgy isExtensible() assertions, I noticed an instance that looked bad.  Talking it over on IRC, plus looking at PropertyCache::testForSet, revealed the entire bit of code to be dead -- this patch kills it, slightly adjusts surrounding code, and fixes up docs.

Comment 4

[Jeff Walden [:Waldo]]

Oh, I pushed the fix for bug 600128 to TM just now:

http://hg.mozilla.org/tracemonkey/rev/7fc2209ef579

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 481664 [details] [diff] [review]
2: Remove dead property-setting property-cache code

..12345678901234567890123456789012345678901234567890123456789012345678901234567890
>+             * Property cache hit, only partially confirmed by testForSet. We
>+             * know that the entry applies to regs.pc and that obj's shape
>+             * matches.
>              *
>              * The entry predicts either a new property to be added
>              * directly to obj by this set, or on an existing "own"

Mega-nit: may as well rewrap (vim Q2j or equivalent) the second paragraph here too.

Thanks for finding this!

/be

Comment 6

[Brendan Eich [:brendan]]

Very early on in the push for Firefox 3, hacking the proprerty cache to win against kjs on SunSpider, I overreached and tried to cache prototype object hits (setters). Then I backed off. That left the logic (you can see it in cvs.m.o) seeming to handle a "full test" (hit on a non-direct prototype) at first, but then bailing because (obj == obj2) conditions failed.

So this was dead code all along!

/be

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a827b3a89300

is this fixed?

It looks like both patches went to to TM and MC.

Comment 8

[Jeff Walden [:Waldo]]

The second patch landed in TM, but we still had some code that triggered the dead code, so a bunch of test runs were orange from the !!atom assertion.  With titles gone, the second patch should be fine now:

http://hg.mozilla.org/tracemonkey/rev/56645320b950

Hasn't passed tinderboxen yet, but a linux64 try run I kicked off last night came back all green, so I expect it's good.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/56645320b950