Closed
Bug 245890
Opened 21 years ago
Closed 21 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•21 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•21 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•21 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 5•21 years ago
|
||
Comment on attachment 150282 [details] [diff] [review]
proposed fix
r=shaver
Attachment #150282 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 6•21 years ago
|
||
Fixed.
/be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•21 years ago
|
||
Did this checkin cause bug 246048?
/be
Comment 8•21 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•21 years ago
|
||
Reopening due to back-out.
/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 10•21 years ago
|
||
Ok, new patch coming, could use help testing -- donner, you able?
/be
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 11•21 years ago
|
||
Attachment #150282 -
Attachment is obsolete: true
Assignee | ||
Comment 12•21 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•21 years ago
|
Attachment #150559 -
Attachment is obsolete: true
Assignee | ||
Comment 13•21 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•21 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•21 years ago
|
||
Attachment #150569 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #150584 -
Flags: review?(shaver)
Assignee | ||
Updated•21 years ago
|
Attachment #150569 -
Flags: review?(shaver)
Assignee | ||
Updated•21 years ago
|
Attachment #150584 -
Attachment is obsolete: true
Attachment #150584 -
Flags: review?(shaver)
Assignee | ||
Comment 16•21 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•21 years ago
|
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
Assignee | ||
Updated•21 years ago
|
Attachment #153352 -
Flags: review?(shaver)
Updated•21 years ago
|
Attachment #153352 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 17•21 years ago
|
||
Fixed. This was a 1.8alpha bug only.
/be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Flags: testcase-
You need to log in
before you can comment on or make changes to this bug.
Description
•