564171 - Difference in behaviour between quickstubbed and non-quickstubbed readonly properties

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Peter Van der Beken [:peterv]]

In bug 563948 I'm trying to quickstub nsIDOMElement::classList, which is a readonly IDL property, and it results in a change in behaviour.

The quickstubs code defines a property for classList with JSPROP_SHARED, a JSPropertyOp for the getter and js_GetterOnlyPropertyStub for the setter. As a result we throw an exception (from js_GetterOnlyPropertyStub) when trying to set .classList.

If this property is not quickstubbed, then XPConnect defines the property with JSPROP_SHARED | JSPROP_GETTER, a JSFunction for the getter and js_GetterOnlyPropertyStub for the setter. This throws an exception only in strict mode when trying to set .classList. The difference in behaviour seems to come from JSScopeProperty::set where we end up calling js_ReportGetterOnlyAssignment, but only if (attrs & JSPROP_GETTER).

I haven't tested this, but the behaviour probably changes as soon as someone uses __lookupGetter__ on the property, because we then do set JSPROP_GETTER on the property.

Jason asked me to file this bug, because he thinks it could be an API bug. If it's decided that's not the case I can always fix this in quickstubs by using a custom setter JSPropertyOp that does the same call to JS_ReportErrorFlagsAndNumber as js_ReportGetterOnlyAssignment.

Comment 1

[David Mandelin [:dmandelin]]

Hmmm, this looks like it might be related to bug 564171, where in JM the "window.content", property gets defined as JSPROP_SHARED | JSPROP_GETTER (like the non-quickstubbed case) on [global].__proto__, while in TM it gets defined without either flag on [global]. Could this be related?

Comment 2

[Peter Van der Beken [:peterv]]

(In reply to comment #1)
> bug 564171

That doesn't look like the right bug number.

This blocks the landing of the patch in bug 563948. I'll probably end up writing a custom setter JSPropertyOp just for quickstubs for now, so I can land that patch.

Comment 3

[Jeff Walden [:Waldo]]

As far as setting behavior goes, the most webby behavior would be to throw only in strict mode.

I think this is the issue I alluded to in bug 529404 comment 1; maybe the change identified there would also fix this issue.  However, the attrs mismatch still needs to be fixed regardless, because Object.getOwnPropertyDescriptor will expose the difference.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Workaround

Workaround I'd like to land so i can land the patch in bug 563948.

Comment 5

[Jason Orendorff [:jorendorff]]

I am only 70% certain of the following four assertions...

1. This is an engine bug.

2. Suppose we fix bug 552432. Then here's the behavior I think we want. Assigning to a non-writable own non-SHARED property should act exactly the same as assigning to a SHARED property with no setter:
  - it's a TypeError in ES5 "use strict";
  - it's a warning if JSOPTION_STRICT is in force
    (an Error if we have JSOPTION_WERROR too);
  - it silently fails otherwise.

3. That's compatible with ES5.

4. JSPROP_GETTER/SETTER really should not affect this no matter what else is going on.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 454853 [details] [diff] [review]
Workaround

I have no real objection to workarounds.

Comment 7

[Peter Van der Beken [:peterv]]

Checked in workaround: http://hg.mozilla.org/mozilla-central/rev/e5a3822abe5a. I'll leave this bug open for a real fix.

Comment 8

[Boris Zbarsky [:bzbarsky]]

We nixed all the property reification stuff, so this is not an issue anymore.