245890 - js_SetRequiredSlot fails to extend map->freeslot

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

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 <[e-mail]> for reporting this.

/be

Comment 1

[Brendan Eich [:brendan]]

Patch ASAP.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: 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

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 150282 [details] [diff] [review]
proposed fix

r=shaver

Comment 6

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 7

[Brendan Eich [:brendan]]

Did this checkin cause bug 246048?

/be

Comment 8

[Calum Mackay]

It looks like it did. I backed out the patch for this bug, and I could no longer
reproduce bug 246048.

Comment 9

[Brendan Eich [:brendan]]

Reopening due to back-out.

/be

Comment 10

[Brendan Eich [:brendan]]

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

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: proposed fix, take two

Comment 12

[Brendan Eich [:brendan]]

Attachment: 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

Comment 13

[Brendan Eich [:brendan]]

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

Comment 14

[Brendan Eich [:brendan]]

I checked in the safe part of the patch (changing JSObjectOps.setRequiredSlot's
return type to JSBool).  New, minimal patch coming up.

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: proposed fix, take four

Comment 16

[Brendan Eich [:brendan]]

Attachment: 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

Comment 17

[Brendan Eich [:brendan]]

Fixed.  This was a 1.8alpha bug only.

/be