1025338 - Remove JSPropertyDescriptor::object()

Status: RESOLVED DUPLICATE of bug 1708698

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Right now we do all this work to maintain the object we found the described property upon. In reality though, we only care about two things:

(1) Was the property found? !!desc.object()
(2) Was the property |own|? desc.object() == obj

Both of these could be solved with simple flags.

The current setup is bad because:
* It costs us an extra wrap() call when wrapping property descriptors across membranes.
* For pseudo (intra-compartment) membranes, we generally get this wrong, leading to bugs like bug 1025323.
* It's an extra thing to root in JSPropertyDescriptor.

This shouldn't take more than a couple of hours, and seems like it would have very good bang for the buck. Does anyone have an intern we could give this to? I'm happy to mentor it.

Comment 1

[Eric Faust [:efaust]]

+1. This also helps us if we decide that we want to use js::PropDesc externally instead of JSPropertyDescriptor. It's not clear to me that we care to do that, since [[Origin]] has been removed, though.

This would be amamzingly cool to have done, though.

Comment 2

[Bobby Holley (:bholley)]

Eric is going to take this. Should be a quick performance and correctness win with little effort.

Comment 3

[Eric Faust [:efaust]]

OK, so it looks like ChromeObjectWrapper.cpp contains PropIsFromStandardPrototype(), which uses the object() in a non-trivial way. Can we just add a js::UnshadowedPrototypeWithProperty() (or something, that name is bad) friend API function to do this lookup? It looks like we are already inside the security boundary.

Bobby, does this seem sane/reasonable?

Comment 4

[Bobby Holley (:bholley)]

Yeah, this machinery is all on its way out, so I'm fine with implementing this check in a dumb way. The simplest drop-in replacement would be to just manually climb the prototype chain invoking getOwnPropertyDescriptor.

Comment 5

[Bobby Holley (:bholley)]

Eric, are you ever going to get back to this?

Comment 6

[Eric Faust [:efaust]]

It's unclear to me that we actually want to do this. I don't actually think the two things in comment 1 are true? I've seen several cases where we do other things with that object.

I have to admit, it's not high on my list.

Comment 7

[Bobby Holley (:bholley)]

Attachment: PoC of comment 0.

I wrote this to prove to efaust that we didn't need desc.object(). He has now
relented and agreed to fix this. :-)

The one hangup is BaseProxyHandler::get, which uses object() in a weird way
after bug 895223. Both efaust and jorendorff have ideas to solve this.

Comment 8

[Bobby Holley (:bholley)]

Eric says that he'll look at this on monday - setting NI to track that. Thanks for doing it!

Comment 9

[Jason Orendorff [:jorendorff]]

My idea is to reimplement BaseProxyHandler::get to follow 9.1.8 closely. Then it won't use getPropertyDescriptor at all anymore.

(I previously did this for BPH::set; BPH::get should be easier.)

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 11

[Jan de Mooij [:jandem]]

Bug 1708698 did this.