Closed
Bug 245890
Opened 20 years ago
Closed 20 years ago
js_SetRequiredSlot fails to extend map->freeslot
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: brendan, Assigned: brendan)
Details
(Keywords: crash, js1.5)
Attachments
(1 file, 4 obsolete files)
2.76 KB,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•20 years ago
|
||
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
Assignee | ||
Comment 3•20 years ago
|
||
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)
Assignee | ||
Comment 4•20 years ago
|
||
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+
Assignee | ||
Comment 6•20 years ago
|
||
Fixed. /be
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•20 years ago
|
||
Did this checkin cause bug 246048? /be
Comment 8•20 years ago
|
||
It looks like it did. I backed out the patch for this bug, and I could no longer reproduce bug 246048.
Assignee | ||
Comment 9•20 years ago
|
||
Reopening due to back-out. /be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 10•20 years ago
|
||
Ok, new patch coming, could use help testing -- donner, you able? /be
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 11•20 years ago
|
||
Attachment #150282 -
Attachment is obsolete: true
Assignee | ||
Comment 12•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Attachment #150559 -
Attachment is obsolete: true
Assignee | ||
Comment 13•20 years ago
|
||
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)
Assignee | ||
Comment 14•20 years ago
|
||
I checked in the safe part of the patch (changing JSObjectOps.setRequiredSlot's return type to JSBool). New, minimal patch coming up. /be
Assignee | ||
Comment 15•20 years ago
|
||
Attachment #150569 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #150584 -
Flags: review?(shaver)
Assignee | ||
Updated•20 years ago
|
Attachment #150569 -
Flags: review?(shaver)
Assignee | ||
Updated•20 years ago
|
Attachment #150584 -
Attachment is obsolete: true
Attachment #150584 -
Flags: review?(shaver)
Assignee | ||
Comment 16•20 years ago
|
||
"Take Five", a seminal 5/4-time cool jazz piece by Paul Desmond made famous by Dave Brubeck! Here's hoping. /be
Assignee | ||
Updated•20 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
Assignee | ||
Updated•20 years ago
|
Attachment #153352 -
Flags: review?(shaver)
Attachment #153352 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 17•20 years ago
|
||
Fixed. This was a 1.8alpha bug only. /be
Status: ASSIGNED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Flags: testcase-
You need to log in
before you can comment on or make changes to this bug.
Description
•