Property descriptors in self hosted code should not inherit from Object.prototype

RESOLVED FIXED in mozilla31

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: anba, Unassigned)

Tracking

Trunk
mozilla31
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
test case:
---
js> Object.prototype.get = "kaboom"
"kaboom"
js> Intl.Collator.supportedLocalesOf(["de"])
typein:2:0 TypeError: property descriptor's getter field is neither undefined nor a function
---

Expected:
Intl.Collator.supportedLocalesOf() returns an array

Actual:
A TypeError was thrown


The TypeError is thrown in the SupportedLocales() function (Intl.js, l.794), the property descriptors created in that function should set their [[Prototype]] property to `null`. 



Note: SpiderMonkey does not supported the __proto__ in ObjectLiteral semantics from the current ES6 draft, so it is not possible to use the __proto__ short-hand definition to fix this bug. Instead SpiderMonkey calls internally [[Set]] (resp. [[Put]]), so users could re-define Object.prototype.__proto__ and execute their own code. For example this is the shell output from a simple test which adds `__proto__: null` to the object literal in Intl.js, l. 819 . 

---
js> Object.defineProperty(Object.prototype, "__proto__", {set: () => {print("hello")}})
({})
js> Intl.Collator.supportedLocalesOf(["de"])
hello
["de"]
---
Clever and dastardly, well-spotted.

It's not clear to me that we necessarily want "make all object literals in self-hosted code have null prototype" as a solution, but definitely we need to do something about this.  CCing till in case he has some great idea here.  This is easy to fix with std_Object_create(null), but in the long run we don't want to eat the perf difference of that, necessarily.  (Or perhaps we should have code in Ion to do smart things for Object.create(null), maybe?)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Huh, I remember seeing this bug. How I didn't see that I should act on it, I don't know.

I do think that the jits (or, type inference, I suppose) should be able to deal with Object.create(null) without completely deoptimizing things.

Looking through Intl.js, there a quite a few object literals. I wonder how many of them are problematic.

For the Object.defineProperty usecase, we could introduce an intrinsic that works without the object. Given how slow defineProperty is[1], that probably makes sense anyway.


[1]: bug 772334
Most shouldn't be.  We were pretty careful about using them only when accessing the specific fields added to them, or doing the has-own qualification when considering named properties via for-in.  (But remember bug 822080, if that's relevant any more -- last comment suggests another yak to shave, not the original one, but I haven't re-investigated.)  That said, for-in plus own-testing probably breaks as well if you mutate Object.prototype's [[Prototype]] to point to a proxy that has an enumerate hook on it.  (Yeah, yeah.  :-) )  The look-up-other-properties behavior here is hidden behind an implementation function, which is why it didn't stick out during review.

As far as the self-hosting intrinsic goes, it's a fair quick'n'dirty hack.  We probably should add special-casing to the JIT for calling Object.defineProperty, and some sort of multi-method dispatch based on the shape of the descriptor passed in.  That would eat up the current slowness pretty well, I bet, although it's kind of messily complex to get right, I'm sure.  We're not going to unilaterally add an Object.defineDataProperty function or what-have-you without spec say-so (which seems unlikely, but I could be surprised), so *something* will have to be done about the standard method so normal sites can benefit.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Most shouldn't be.  We were pretty careful about using them only when
> accessing the specific fields added to them, or doing the has-own
> qualification when considering named properties via for-in.  (But remember
> bug 822080, if that's relevant any more -- last comment suggests another yak
> to shave, not the original one, but I haven't re-investigated.)  That said,
> for-in plus own-testing probably breaks as well if you mutate
> Object.prototype's [[Prototype]] to point to a proxy that has an enumerate
> hook on it.  (Yeah, yeah.  :-) )  The look-up-other-properties behavior here
> is hidden behind an implementation function, which is why it didn't stick
> out during review.

Ok, that's reassuring. I also remember Norbert being pretty careful about this.

> As far as the self-hosting intrinsic goes, it's a fair quick'n'dirty hack. 
> We probably should add special-casing to the JIT for calling
> Object.defineProperty, and some sort of multi-method dispatch based on the
> shape of the descriptor passed in.

I think in the vast majority of cases defineProperty is called with an object literal. If we can pattern-detect that, and guard against Object.defineProperty being changed (why can't this be syntax?), we might even be able to not create the object at all. Another task to nicely ask efaust about. :)

> That would eat up the current slowness
> pretty well, I bet, although it's kind of messily complex to get right, I'm
> sure.

I'm somewhat hopeful that the approach outlined above might not be the hardest thing ever.

> We're not going to unilaterally add an Object.defineDataProperty
> function or what-have-you without spec say-so (which seems unlikely, but I
> could be surprised), so *something* will have to be done about the standard
> method so normal sites can benefit.

Oh, I fully agree. We very much want this in Shumway, and it's a Games:P1, too.
Looping in efaust for awareness of the last couple comments.  ("The watch is not done.", and all that.)
(Reporter)

Comment 6

3 years ago
Is this bug report still relevant now that bug 988416 has landed?
I don't believe so, at a skim.  Certainly SupportedLocales is fixed, and no other places were explicitly mentioned to double-check.  And that bug audited for use of Object.defineProperty (or a std_* version of it) and censored the function from the SHG, so I see no reason to believe the fix wasn't comprehensive.

Marking FIXED just because we may not have flagged this exact issue in that bug, even tho that bug contemplated this problem, just not pointedly.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.