js_SetRequiredSlot fails to extend map->freeslot

RESOLVED FIXED in mozilla1.8beta1

Status

()

P1
critical
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({crash, js1.5})

Trunk
mozilla1.8beta1
crash, js1.5
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

15 years ago
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 1

15 years ago
Patch ASAP.

/be
Status: NEW → ASSIGNED
Flags: blocking1.7+
(Assignee)

Comment 2

15 years ago
Created attachment 150282 [details] [diff] [review]
proposed fix

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

15 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

15 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

15 years ago
Fixed.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

15 years ago
Did this checkin cause bug 246048?

/be

Comment 8

15 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

15 years ago
Reopening due to back-out.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

15 years ago
Ok, new patch coming, could use help testing -- donner, you able?

/be
Status: REOPENED → ASSIGNED
(Assignee)

Comment 11

15 years ago
Created attachment 150559 [details] [diff] [review]
proposed fix, take two
Attachment #150282 - Attachment is obsolete: true
(Assignee)

Comment 12

15 years ago
Created attachment 150569 [details] [diff] [review]
proposed fix, take three

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

15 years ago
Attachment #150559 - Attachment is obsolete: true
(Assignee)

Comment 13

15 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

15 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

15 years ago
Created attachment 150584 [details] [diff] [review]
proposed fix, take four
Attachment #150569 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #150584 - Flags: review?(shaver)
(Assignee)

Updated

15 years ago
Attachment #150569 - Flags: review?(shaver)
(Assignee)

Updated

15 years ago
Attachment #150584 - Attachment is obsolete: true
Attachment #150584 - Flags: review?(shaver)
(Assignee)

Comment 16

15 years ago
Created attachment 153352 [details] [diff] [review]
proposed fix, take five

"Take Five", a seminal 5/4-time cool jazz piece by Paul Desmond made famous by
Dave Brubeck!

Here's hoping.

/be
(Assignee)

Updated

15 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
(Assignee)

Updated

15 years ago
Attachment #153352 - Flags: review?(shaver)
Attachment #153352 - Flags: review?(shaver) → review+
(Assignee)

Comment 17

14 years ago
Fixed.  This was a 1.8alpha bug only.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago14 years ago
Resolution: --- → FIXED

Updated

13 years ago
Flags: testcase-
You need to log in before you can comment on or make changes to this bug.