Closed Bug 245890 Opened 21 years ago Closed 21 years ago

js_SetRequiredSlot fails to extend map->freeslot

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: brendan, Assigned: brendan)

Details

(Keywords: crash, js1.5)

Attachments

(1 file, 4 obsolete files)

This bites the 1.7-era JSOP_REGEXP code that ensures a brutally shared function has its compiler-created regexps cloned when the function is cloned. Result is a dangling slot (crash). Many thanks to Frierich Munch <colsebas@hotmail.com> for reporting this. /be
Patch ASAP. /be
Status: NEW → ASSIGNED
Flags: blocking1.7+
Attached patch proposed fix (obsolete) — Splinter Review
JSObjectOps.setRequiredSlot was wrongly void, but even before this patch could suffer failure to realloc. I fixed it (it's not frozen) and its uses in js/src and js/src/liveconnect. /be
Comment on attachment 150282 [details] [diff] [review] proposed fix Not a 1.7 problem, sorry to raise that alarm. This is for the trunk only. /be
Attachment #150282 - Flags: review?(shaver)
No worries, my memory was wrong: the JSOP_REGEXP auto-cloning change went into the 1.8a trunk, not into 1.7 or the 1.7 branch. /be
Flags: blocking1.7+
Comment on attachment 150282 [details] [diff] [review] proposed fix r=shaver
Attachment #150282 - Flags: review?(shaver) → review+
Fixed. /be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Did this checkin cause bug 246048? /be
It looks like it did. I backed out the patch for this bug, and I could no longer reproduce bug 246048.
Reopening due to back-out. /be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ok, new patch coming, could use help testing -- donner, you able? /be
Status: REOPENED → ASSIGNED
Attached patch proposed fix, take two (obsolete) — Splinter Review
Attachment #150282 - Attachment is obsolete: true
Attached patch proposed fix, take three (obsolete) — Splinter Review
So jsobj.c:js_Mark returns the number of slots for js_MarkGCThing to scan, and in the case of an unmutated clone (scope->object != obj), it has to return the object's slots vector's intrinsic length (stored in obj->slots[-1], untagged). This means all allocated slots will be scanned for such unmutated objects. The backed-out patch, besides having an unnecessary rlimit local variable in js_SetRequiredSlot, always mutated such clones to have their own scopes. This was unnecessary and inefficient: required/reserved slots should be settable without cloning the shared proto-scope, precisely due to the js_Mark special case mentioned above. Worse, by setting scope->map.freeslot, the backed-out patch broke the rule that freeslot must jump over all computed-reserved slots (those claimed by non-stub clasp->reserveSlots, not by constant JSCLASS_HAS_RESERVED_SLOTS). That rule is enforced by js_AllocSlot, which is called only on mutated (scope-owning) objects. So if an unmutated object then was mutated to hold ad-hoc properties, after having only some of its computed-reserved slots set, its (own) scope's freeslot member might index a computed-reserved slot, and not the first ad-hoc property slot. The unmutated object would be a cloned function object, and its computed reserved slots hold regexp clones, created lazily. Clear as mud? This needs lots of testing. Cc: list buddies, can you help? /be
Attachment #150559 - Attachment is obsolete: true
Comment on attachment 150569 [details] [diff] [review] proposed fix, take three If it were possible for an object to get its own mutable scope, but not allocate any slots, then the assertion I'm adding in js_SetRequiredSlot would botch, and we would want to update scope->map.nslots and scope->map.freeslot, if (scope->object == obj). I have mail to Frierich because he seemed to run afoul of such a case, but I don't see how it can happen. Objects that need no ad-hoc property slots should never get their own scopes. /be
Attachment #150569 - Flags: review?(shaver)
I checked in the safe part of the patch (changing JSObjectOps.setRequiredSlot's return type to JSBool). New, minimal patch coming up. /be
Attached patch proposed fix, take four (obsolete) — Splinter Review
Attachment #150569 - Attachment is obsolete: true
Attachment #150584 - Flags: review?(shaver)
Attachment #150569 - Flags: review?(shaver)
Attachment #150584 - Attachment is obsolete: true
Attachment #150584 - Flags: review?(shaver)
"Take Five", a seminal 5/4-time cool jazz piece by Paul Desmond made famous by Dave Brubeck! Here's hoping. /be
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
Attachment #153352 - Flags: review?(shaver)
Attachment #153352 - Flags: review?(shaver) → review+
Fixed. This was a 1.8alpha bug only. /be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Flags: testcase-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: