367589 - "Assertion failure: !SPROP_HAS_STUB_SETTER(sprop) || (sprop->attrs & JSPROP_GETTER)"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase

Steps to reproduce:
1. Load the testcase in a ***debug build*** of Firefox.
2. Click the "crash" button.

Result:

Assertion failure: !SPROP_HAS_STUB_SETTER(sprop) || (sprop->attrs & JSPROP_GETTER), at /Users/admin/trunk/mozilla/js/src/jsobj.c:3473

Comment 1

[Jesse Ruderman]

Actually, this crashes opt builds too.

Comment 2

[Brendan Eich [:brendan]]

Is this affecting 1.8 or 1.8.0?  Probably.

/be

Comment 3

[Brendan Eich [:brendan]]

It seems that nsGenericArraySH::NewResolve is defining the 0 element with property attrs JSPROP_SHARED | JSPROP_ENUMERATE, yet with the "HTMLCollection" JSClass having a stub setProperty hook.  The JSPROP_SHARED attr means there will be no slot allocated for value storage, and the lack of a non-stub setter means that no code runs on assignment to store the value otherwise.  So assigning 'wtf' to the 0 element of the button sends that value to the bit-bucket, or should.

The JS engine should cope with the API abusage here, so I'll have a patch soon, but the content code should be fixed too, I think.  jst, why is there both the JSPROP_SHARED attr and a stub setter for this case?

/be

Comment 4

[Brendan Eich [:brendan]]

This will be fixed today, no need for blocking1.9 nomination. But please do nominate blocking1.8.x as needed.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix

Should the HTMLCollection have a setter for each element?  Or should it not use JSPROP_SHARED and have the standard value storage (a slot in obj->dslots)?

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 252172 [details] [diff] [review]
fix

Patch applies cleanly with hunk skew to both branches.

/be

Comment 7

[Brendan Eich [:brendan]]

Jesse, did you find this by fuzzing?

/be

Comment 8

[Jesse Ruderman]

No, I found it by being a dumbass and accidentally assigning to document.getElementsByTagName("ol")[0] when I meant to set an attribute on it.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #3)
> means that no code runs on assignment to store the value otherwise.  So
> assigning 'wtf' to the 0 element of the button

Correction: to the 0 element of the HTMLCollection, whose value is the reference to the button object. Doesn't change the fact that stub setter and JSPROP_SHARED without JSPROP_READONLY is kind of a nonsense state.

The JS engine should cope, and did (and will again with this patch), but perhaps it's not right for the DOM to throw the value being assigned away?  If the collection should have readonly properties at indexes from 0 up to the value of its length property, then you could get strict warnings or errors, at your option.

/be

Comment 10

[Brendan Eich [:brendan]]

Fixed on the trunk:

js/src/jsobj.c 3.324

/be

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 252172 [details] [diff] [review]
fix

a=dveditz for 1.8/1.8.0 branches

Comment 12

[Brendan Eich [:brendan]]

Fixed on the

1.8 branch:   js/src/jsobj.c 3.208.2.48
1.8.0 branch: js/src/jsobj.c 3.208.2.12.2.22

/be

Comment 13

[Martijn Wargers (dead)]

Verified fixed on trunk. I crashed with a 2007-01-19 trunk build, but not anymore with a 2007-01-21 trunk build.
Also verified that the testcase doesn't crash in the latest branch builds.

Comment 14

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367589.js,v  <--  regress-367589.js

Comment 15

[Jesse Ruderman]

Filed bug 371711 on making
  document.getElementsByTagName('button')[0] = 'wtf';
throw an exception.

Comment 16

[Bob Clary [:bc] (inactive)]

Browser based tests which rely on the delayed ending of the test must call reportCompare before jsTestDriverEnd in order for the test results to be recorded prior to their dumping to stdout.

Checking in regress-367589.js;
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367589.js,v  <--  regress-367589.js
new revision: 1.3; previous revision: 1.2