519933 - Add option to allow setting... read only properties

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Marc]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.14) Gecko/2009090216 Ubuntu/9.04 (jaunty) Firefox/3.0.14
Build Identifier: 

Properties defined as read only (with only a GetterSlot) could be set without problem until Rhino 1.7R2 and we used this feature in HtmlUnit.

Since 1.7R2 an exception is thrown in such a case what breaks our code. Attached patch adds a new Context feature allowing to have this "feature" again

Reproducible: Always

Comment 1

[Marc]

Attachment: Patch with unit test and fix

Comment 2

[Attila Szegedi]

This looks quite valid to me, but I'm wondering since when should "TC39 ES3.1 Draft of 9-Feb-2009, 8.12.4, step 2" be Rhino's default behavior. It's an ECMAScript 5 behaviour, so when Rhino runs with lower VERSION_1_x setting, it should actually be disabled by default, should it not? (Question aimed at Norris and maybe Rapha).

Which brings me to the point of what's the VERSION_x value we'll use for ECMAScript 5 in Rhino 1.7R2? Since it's correlated to JavaScript 2.0, I guess we should introduce VERSION_2_0 and actually have such ECMAScript 5 behaviors that aren't compatible with earlier JS only be enabled when VERSION_2_0 is in effect. 

I'll accept this patch as-is for now, but I'd even be more permissive than it is now (having the feature default to "false") and I'd in fact have it default to true when engine is run with earlier VERSION_1_x values.

Comment 3

[Attila Szegedi]

Committing patch in current version:

cvs ci -m "Fix for Bug 519933 - Add option to allow setting... read only properties" -l "/Rhino/src/org/mozilla/javascript/Context.java" "/Rhino/src/org/mozilla/javascript/ScriptableObject.java" "/Rhino/src/org/mozilla/javascript/ContextFactory.java"
    Checking in src/org/mozilla/javascript/ScriptableObject.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
    new revision: 1.160; previous revision: 1.159
    done
    Checking in src/org/mozilla/javascript/Context.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/Context.java,v  <--  Context.java
    new revision: 1.280; previous revision: 1.279
    done
    Checking in src/org/mozilla/javascript/ContextFactory.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ContextFactory.java,v  <--  ContextFactory.java
    new revision: 1.35; previous revision: 1.34
    done
ok (took 0:04.367)

Comment 4

[Attila Szegedi]

From an e-mail with Norris:

See bug https://bugzilla.mozilla.org/show_bug.cgi?id=523846 "Assignments to a property that has a getter but not a setter should only throw a TypeError in strict mode". This bug is filed against SpiderMonkey, but applies to Rhino as well as you point out. So for this particular issue, we will change default behavior to be compatible with previous JavaScript and ECMAScript versions.

This seems to imply that we don't need a new FEATURE_WRITE_READONLY_PROPERTIES, but rather should scope this behaviour to FEATURE_STRICT_MODE.

Comment 5

[Attila Szegedi]

Reworked it; removed FEATURE_WRITE_READONLY_PROPERTIES and instead the new ES5 functionality is only enabled when FEATURE_STRICT_MODE is set to true.

cvs ci -m "Different Fix for Bug 519933 - only throw TypeError when FEATURE_STRICT_MODE is set" -l "/Rhino/src/org/mozilla/javascript/Context.java" "/Rhino/src/org/mozilla/javascript/ScriptableObject.java" "/Rhino/testsrc/org/mozilla/javascript/tests/WriteReadOnlyPropertyTest.java" "/Rhino/src/org/mozilla/javascript/ContextFactory.java"
    Checking in src/org/mozilla/javascript/ScriptableObject.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
    new revision: 1.161; previous revision: 1.160
    done
    Checking in src/org/mozilla/javascript/Context.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/Context.java,v  <--  Context.java
    new revision: 1.281; previous revision: 1.280
    done
    Checking in src/org/mozilla/javascript/ContextFactory.java;
    /cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ContextFactory.java,v  <--  ContextFactory.java
    new revision: 1.36; previous revision: 1.35
    done
    RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/WriteReadOnlyPropertyTest.java,v
    done
    Checking in testsrc/org/mozilla/javascript/tests/WriteReadOnlyPropertyTest.java;
    /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/WriteReadOnlyPropertyTest.java,v  <--  WriteReadOnlyPropertyTest.java
    initial revision: 1.1
    done
ok (took 0:07.409)

Comment 6

[Norris Boyd]

I changed the behavior of the current Rhino HEAD slightly; it was modifying the value when there was a getter but no setter. I've changed it to be compatible with SpiderMonkey and ES5 where it will not change the value of the property.