Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Inherited non-configurable accessor properties mis-reported as own properties

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P1
major
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Mark S. Miller, Assigned: brendan)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
On Minefield 4.0b13pre (2011-03-01)

> var gopd = Object.getOwnPropertyDescriptor
> gopd
function getOwnPropertyDescriptor() { [native code] }

> Object.defineProperty(Function.prototype, 'ident___', { get: function(){return "gotten";} });

> Array.ident___
gotten

> gopd(Array, 'ident___')
[object Object]

> gopd(Array, 'ident___').get()
gotten

> Object.getOwnPropertyDescriptor(Array, 'ident___')
[object Object]

> Array.hasOwnProperty('ident___')
true
(Assignee)

Updated

7 years ago
Depends on: 575997
(Reporter)

Comment 1

6 years ago
Is there any news on this? It is still unfixed on Nightly 6.0a1 (2011-04-30) and seems rather serious. Speculating in ignorance, I would also guess it's rather easy to fix.
It's not all that easy to fix, as it involves intricacies of the way different kinds of properties (including kinds not in the spec :-\ although to introspection they appear spec-like, and only through other interactions can you discover their real nature) are represented.  That's strike one.  There are also various dependencies on those intricacies, some of which we've removed (bug 640072 and bug 640503, for example), some of which remain.  That's strike two.  It's going to take time to properly and fully fix this bug.
(Assignee)

Comment 3

6 years ago
Someone taking this or really the bug it depends on, for FF6 ? Seems fixable.

/be
(Reporter)

Comment 4

6 years ago
The bug seems to occur only for non-configurable accessor properties. I don't see the symptom for configurable accessor properties until they are made non-configurable.
(Reporter)

Updated

6 years ago
See Also: → bug 636989
(Assignee)

Updated

6 years ago
Assignee: general → brendan
(Assignee)

Comment 5

6 years ago
(In reply to comment #4)
> The bug seems to occur only for non-configurable accessor properties. I
> don't see the symptom for configurable accessor properties until they are
> made non-configurable.

Yes, that is expected. The "Shared" property hack involved applies only if the property also has the "DontDelete" attribute, so that it cannot be shadowed (by assignment, the only way to shadow in ES3 or older days). Working on this now...

/be
(Assignee)

Comment 6

6 years ago
Created attachment 538611 [details] [diff] [review]
proposed fix

Passes the suite. The Namespace and QName instance properties were the only cases of proto-properties pretending to be instance properties that I could find.

String and RegExp class init code duplicates a lot of js_InitClass now, could be reworked to use the sharedShape optional parameter to js_InitClass, but I left that for another bug.

Should get this in for baking, if it looks good, air-drop into a nearer release.

/be
Attachment #538611 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Updated

6 years ago
Summary: Inherited accessor properties (sometimes?) reported as own properties → Inherited non-configurable accessor properties mis-reported as own properties
(Assignee)

Comment 7

6 years ago
Created attachment 538625 [details] [diff] [review]
fix a comment that moved to refer to the right names
Attachment #538611 - Attachment is obsolete: true
Attachment #538611 - Flags: review?(jwalden+bmo)
Attachment #538625 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

6 years ago
Attachment #538625 - Attachment is patch: true
Attachment #538625 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 8

6 years ago
Comment on attachment 538625 [details] [diff] [review]
fix a comment that moved to refer to the right names

Igor, can you have a look at the E4X changes? I'm not trying to redo how that stuff works, since the plan is to self-host it this year.

/be
Attachment #538625 - Flags: review?(igor)
I have comments I'm in the process of writing.  I apologize for the delay in making them.  I'm working on them, and related comments in bug 655192, as fast as I can.  Hopefully I'll finish them tonight, but I have an early-morning engagement which requires that I get to sleep earlier than normal, so I might not finish the response until morning.
(Assignee)

Comment 10

6 years ago
How about a sneak peek? Release early and often, and all that. If the patch has a bug, tests showing it welcome.

/be
It's not bugs, or at least I haven't seen any yet.  It's design and complexity concerns.

I'm sorry.  I'm too mentally and emotionally exhausted to have the energy to do this now.  I'll do it first thing in the morning when I get in.
(Assignee)

Comment 12

6 years ago
Ok, no worries. Believe it or not, I share many of your complexity concerns. But they seem best dealt with in followup bugs I'm happy to file and even own, so we can get ES5 done sooner.

/be

Comment 13

6 years ago
Comment on attachment 538625 [details] [diff] [review]
fix a comment that moved to refer to the right names

Review of attachment 538625 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comment below addressed.

::: js/src/jsxml.cpp
@@ +290,5 @@
> +     * to JSCLASS_CONSTRUCT_PROTOTYPE, in which case the js_InitClass code will
> +     * add namespace_props and namespace_methods after the constructor returns,
> +     * after which js_InitClass sets the compartment's initialNamespaceShape
> +     * member if necessary.
> +     */

The comment should state why the code sets the last property here and only then explain when cx->compartment->initialNamespaceShape can be null and why we do it only for namespace and QName, and not, for example, for RegExp. Even better would be to add such comments about the initialization of the shapes before the field definitions in jscompartment and refer to that comment here.

@@ +293,5 @@
> +     * member if necessary.
> +     */
> +    if (cx->compartment->initialNamespaceShape)
> +        setLastProperty(cx->compartment->initialNamespaceShape);
> +

We need an assertion that cx->compartment->initialNamespaceShape is null only during the class initialization. I suppose checking that the reserved slot in the global object is null for the class would do the trick.
Attachment #538625 - Flags: review?(igor) → review+
(Assignee)

Comment 14

6 years ago
(In reply to comment #13)
> The comment should state why the code sets the last property here and only
> then explain when cx->compartment->initialNamespaceShape can be null and why
> we do it only for namespace and QName, and not, for example, for RegExp.

I don't see why regexp and string class init should not do likewise. Right now there is quite a lot of code (source and inlined compiled code) for regxp and string to do the same thing, modulo the offset of the cx->compartment->initial... member. But that's for another bug.

> Even better would be to add such comments about the initialization of the
> shapes before the field definitions in jscompartment and refer to that
> comment here.

Good idea -- I was trying to keep this commenting to jsxml.cpp but better to DRY over in jscompartment.h.

> @@ +293,5 @@
> > +     * member if necessary.
> > +     */
> > +    if (cx->compartment->initialNamespaceShape)
> > +        setLastProperty(cx->compartment->initialNamespaceShape);
> > +
> 
> We need an assertion that cx->compartment->initialNamespaceShape is null
> only during the class initialization. I suppose checking that the reserved
> slot in the global object is null for the class would do the trick.

Ok. If we treat jsxml.cpp as something to self-host, eliminating this kind of code (and possibly JSCLASS_CONSTRUCT_PROTOTYPE if that's not used by embedders), then this is all a sideshow to fixing this ES5 bug. But assertions are good even in condemned buildings ;-). I will add one here and in initQName.

/be
Comment on attachment 538625 [details] [diff] [review]
fix a comment that moved to refer to the right names

The application of the saved-shape trick for String and RegExp was obscure.  But it was completely restricted to JSObject::initString and JSObject::initRegExp.  (Those methods could have defined the properties completely normally if they didn't care about speed.)  Your attempting to generalize it in js_InitClass, rather than doing it akin to how String and RegExp do it, for QName and Namespace specifically, points out an honest-to-goodness bug:

[jwalden@find-waldo-now src]$ dbg/js
js> Namespace.prototype // resolve the class
({prefix:"", uri:""})
js> Namespace.prototype.a = 17; Namespace.prototype.b = 42; delete Namespace.prototype.a;
true
js> gc()
"before 81920, after 77824, break 02279000\n"
js> new Namespace("http://foo").hasOwnProperty("prefix")
false

For what it's worth, we have tests verifying that this problem doesn't exist for String and RegExp.

The end result of my changes in bug 652199 will make it easier to simplify this caching by restricting it to initQName and initNamespace.  I am more than happy to make the changes you're making here, in patches in that bug (or atop them), if you're willing.

As a super-minor nit, unrelated to the above, I suggest that removing the shared-permanent hack be a separate patch (hg revision, that is) from adjusting all the QName and Namespace stuff.  The latter does not require the former, so for simplicity and clarity they should be kept separate.
Attachment #538625 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 16

6 years ago
(In reply to comment #15)
> The application of the saved-shape trick for String and RegExp was obscure. 
> But it was completely restricted to JSObject::initString and
> JSObject::initRegExp.  (Those methods could have defined the properties
> completely normally if they didn't care about speed.)

I may just do the slow thing in jsxml.cpp, at this point.

> Your attempting to
> generalize it in js_InitClass, rather than doing it akin to how String and
> RegExp do it, for QName and Namespace specifically, points out an
> honest-to-goodness bug:

My attempt at generalizing does not point out a bug, the code simply contains a bug. And the generalization is not the source of the bug. You could generalize the copy/pasted code in jsstr.cpp and jsregexp.cpp, avoid the code duplication and bloat, and not have the bug.

It seems to me you believe copy and paste avoids bugs that can come up from generalizing. I agree that there is a trade-off here. It's not an absolute, of course, since copy/paste risks different bugs (and we've had 0days due to copy and paste).

So don't blame generalizing here.

> js> Namespace.prototype // resolve the class
> ({prefix:"", uri:""})
> js> Namespace.prototype.a = 17; Namespace.prototype.b = 42; delete
> Namespace.prototype.a;
> true
> js> gc()
> "before 81920, after 77824, break 02279000\n"
> js> new Namespace("http://foo").hasOwnProperty("prefix")
> false

Thanks, great catch.

> For what it's worth, we have tests verifying that this problem doesn't exist
> for String and RegExp.

I would like to write E4X tests in some other life. For this bug I'll probably just avoid gilding the lily and make per-instance properties the slow way.

> As a super-minor nit, unrelated to the above, I suggest that removing the
> shared-permanent hack be a separate patch (hg revision, that is) from
> adjusting all the QName and Namespace stuff.  The latter does not require
> the former, so for simplicity and clarity they should be kept separate.

Will do.

/be
(Assignee)

Comment 17

6 years ago
(In reply to comment #16)
> > As a super-minor nit, unrelated to the above, I suggest that removing the
> > shared-permanent hack be a separate patch (hg revision, that is) from
> > adjusting all the QName and Namespace stuff.  The latter does not require
> > the former, so for simplicity and clarity they should be kept separate.
> 
> Will do.


Er, won't -- I found jsxml.cpp needed some kind of change by running the test suite, and changing only jsobj.cpp would leave two tests failing. There's no reason to split hairs like this. The fix is the thing, not curating micro-revs in hg (not where the first cset leaves tests failing, at any rate).

/be
I have no qualms with not writing E4X tests.  :-)

If "the latter [does] require the former", then yes, by all means keep them together.  Although from what I understand, layout people have gotten quite good mileage out of curating micro-revs which each individually build and pass tests.  To each his own, perhaps.
(Assignee)

Comment 19

6 years ago
Created attachment 539335 [details] [diff] [review]
much better fix, thx to Waldo-review

Confine the damage to jsxml.cpp.

/be
Attachment #538625 - Attachment is obsolete: true
Attachment #539335 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 20

6 years ago
Created attachment 539338 [details] [diff] [review]
even better, thanks to irc-Waldo
Attachment #539335 - Attachment is obsolete: true
Attachment #539335 - Flags: review?(jwalden+bmo)
Attachment #539338 - Flags: review?(jwalden+bmo)
Comment on attachment 539338 [details] [diff] [review]
even better, thanks to irc-Waldo

Review of attachment 539338 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming this passes tests -- looks like it should.
Attachment #539338 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 22

6 years ago
(In reply to comment #21)
> r=me assuming this passes tests -- looks like it should.

Already did, several times. My laptop is all hot now :-/. Some of that was due to those ES5 torture tests that you noticed got slower recently -- is that change understood?

/be
Yeah, it's understood.  I'm probably going to push out the patch for it later today.
(Assignee)

Comment 24

6 years ago
http://tbpl.mozilla.org/?tree=Try&rev=aad7550f0d99 is philor "100% tryserver approved" -- \o/

/be
(Assignee)

Comment 25

6 years ago
http://hg.mozilla.org/tracemonkey/rev/7e00a56f7405

/be
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

6 years ago
Blocks: 575997
No longer depends on: 575997
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/7e00a56f7405
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 689717
You need to log in before you can comment on or make changes to this bug.