record_SetPropHit does not emit a check for objects that share their prototype's shape. function D() {} var b = D.prototype = {x: 1}; var d = new D; var arr = [b, b, b, d]; // all same shape, but d has no slot for x for (var i = 0; i < 4; i++) arr[i].x = i; // code emitted here does the wrong thing when arr[i]===d. d.y = 12; // Assertion failure: JSVAL_IS_VOID( STOBJ_GET_SLOT(obj, scope->freeslot)), at ../jsobj.cpp:3446 assertEq(d.x, 3); // TypeError: Assertion failed: got 2, expected 3 The quickest way to fix this bug is to add a guard in the JIT. But prototype-scope-sharing has led to other bugs (such as bug 501690) and makes shape generally less useful for the JIT, which really wants to believe that shape defines layout. Inserting a guard will make us slower on every property set. I think it's time for prototype-scope-sharing to go. (Bug 497789 comment 39 again.)

I don't know if this is exploitable, but if you put N properties on b, instead of just 1, then you can write any odd number you like to memory location ((int *)0 + (N-3)) ...or something like that. For example, I was able to produce this by putting 10000 properties on b: ==4718== Address 0x9c34 is not stack'd, malloc'd or (recently) free'd I imagine that is far short of any actual mapped pages, though.

Taking. This needs to be patched on the branch pronto, so I'll just add the guard and slow down every property set. Maybe it won't be measurable. Bug 503080 is the followup bug for killing prototype-scope-sharing.

The same bug affects test_property_cache, i.e. reads as well as writes. offsetof(JSScope, object) does not currently appear in jstracer.cpp. Patch with extra guards coming in a few hours.

Extra review from Andreas would be welcome. Is MISMATCH_EXIT the right exit type here? This patch guards against objects that share their prototype's scope, when that prototype is *not* the global object. Technically the global object needs this guard too, if it is possible for an object to share its scope with the global object. But that would require the embedding to do something weird, namely create an object that has the same number of reserved slots as a global and also has the global as its prototype. (More generally I don't understand the JIT's assumptions regarding the global object. Does the global object necessarily have a branded scope? a unique shape? Must it have JSCLASS_GLOBAL_FLAGS? In some places we seem to assume that if obj == global at record time, then the same will be true at run time. Do we really assume that? How do we know?)

(In reply to comment #4) > Technically the global object needs this guard too, if it is possible for an > object to share its scope with the global object. But that would require the > embedding to do something weird, namely create an object that has the same > number of reserved slots as a global and also has the global as its prototype. We should rule that out, assertions ought to be enough. But where? > (More generally I don't understand the JIT's assumptions regarding the global > object. Does the global object necessarily have a branded scope? No, but its shape can't change. > a unique shape? We do not require uniqueness since we trigger traces only for matching object pointer, not shape. > Must it have JSCLASS_GLOBAL_FLAGS? Hadn't thought of that, it's a good thing (doing otherwise is bad for modern, codified-in-ES5 "original value of Object.prototype" memoization). Where do you see a dependency? > In some places we seem to assume that > if obj == global at record time, then the same will be true at run time. Do we > really assume that? How do we know?) See getLoop and under. /be

Bet the extra guard will cause noticeable slowdown. Since this was discovered by you, perhaps we could cut to the chase and fix only bug 503080 -- whaddya think? /be

I'd prefer to fix bug 503080, but that will be a big, moderately risky patch, and hard to backport. We urgently need a patch for this that can land in 1.9.1. Tomorrow morning I plan to measure the performance hit with v1. If there is no measurable loss on any of the usual benchmarks, I want to check this into tracemonkey. But even if we lose a percent or two, I will backport that patch to 1.9.1 and request approval to land it (unless there's a better way).

Let's work on bug 503080 a bit more -- time is an issue but not paramount yet. /be

As of right now, bug 503080 is not "almost done"; it depends on bug 503860 which depends on bug 497789 which looks like a stumper to me. Leaning towards the plan in comment 7, to hedge bets.

Any perf news? If we can add the guard without regression, I'm in favor. /be

Well... now bug 503080 is almost done!

Bug 503080 is fixed-in-tracemonkey. Scroll down to the bottom of http://hg.mozilla.org/tracemonkey/rev/3915e2d2c748 to see the test case from comment 0 added to trace-test.js.

Comment on attachment 387644 [details] [diff] [review] v1 Would like perf measures to see whether we could slip this into a dot release. /be

This is marked as blocking Firefox 3.6a1, any ETA? Should it not block that milestone?

OK, time to get something landed in the branch. Beltzner, would you rather have the 9KB patch in this bug that narrowly fixes the issue, isn't tested, and would make future merges more difficult ...or the 64KB patch in bug 503080 (and the teensy followup fix in bug 506347) that probably fixes other undiscovered issues and has landed in tm and m-c?

Fixed in m-c as well, as of 7-24. http://hg.mozilla.org/mozilla-central/rev/3915e2d2c748

(In reply to comment #15) > Beltzner, would you rather have the 9KB patch in this bug that narrowly fixes > the issue, isn't tested, and would make future merges more difficult ...or the > 64KB patch in bug 503080 (and the teensy followup fix in bug 506347) that > probably fixes other undiscovered issues and has landed in tm and m-c? Assuming you mean for 1.9.1.x (the Firefox 3.5.x releases) then I would probably want the patch that you guys feel most securely about. Given the above, that would likely be the larger patch, but we'd still want to see: - bake time on mozilla-central (shipping it in the 3.6 alpha will give us confidence here, as well) - good test coverage - an explanation of why you're confident that it doesn't add risk (regression testing, etc) - a reviewed branch-safe patch, obviously :)

Mass change: adding fixed1.9.2 keyword (This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)

It looks to me like bug 503080 now has more than one follow up. Are we sure it's stable and ready for the 1.9.1 branch? If not, can we just take this, safer, targeted fix for 1.9.1?

My two cents: you're better off taking the common code. The spot-fix patch here is unreviewed and has no miles on perf-testing infra. Apart from one dependency of bug 503080 which I think you don't need to worry about (jorendorff please correct me), the work for bug 503080 including followups looks "done" and "baked" to me. /be

comment #12 notes the test added to js/src/trace-test.js in http://hg.mozilla.org/tracemonkey/rev/3915e2d2c748

(In reply to comment #20) > Apart from one dependency of bug 503080 which I think you don't need to worry > about (jorendorff please correct me), the work for bug 503080 including > followups looks "done" and "baked" to me. I'm confused. The one dependency you mention is, I'm assuming, bug 503860, which in turn has 10 dependencies in its dependency tree itself. That seem scary to me simply by the amount of dependencies. Is that really not something we should be worrying about? Likewise, bug 514222 will need to be fixed if we take bug 503080, but it still has an open dependency (bug 514454), which is explicitly titled "Fix all the bugs introduced by the patch in bug 514222". That scares me and makes me think that bug 514222 wasn't ready to land and certainly isn't ready to land on the stable branch. It also hasn't been touched since early September... Where do we stand on this?

testBug501690|testObjectVsPrototype v 1.9.3, 1.9.2