942037 - Remove object_prototype_names

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

This should be trivial. See bug 933681 comment 19.

Comment 1

[salehqt]

I am new to the source code repository, if it is a trivial bug, can you elaborate what is it specifically that needs to be changed. The comment 19 only gives one example. The example makes sense but what are object_property_names?

Comment 2

[Bobby Holley (:bholley)]

(In reply to salehqt from comment #1)
> I am new to the source code repository, if it is a trivial bug

Whether or not this is a trivial bug depends on whether anything breaks when we try to do this. ;-)

> can you
> elaborate what is it specifically that needs to be changed. The comment 19
> only gives one example. The example makes sense but what are
> object_property_names?

The idea is to get rid of the block here:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#1311

And replace it with what jorendorff suggests.

Comment 3

[Jack Trocinski [:jtrocinski]]

I'd like to be assigned to this bug, could someone assign me please.

Comment 4

[Jason Orendorff [:jorendorff]]

Jack, can you work on this?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Jack, can you work on this?

Can we wait until we land the patches that I have in your queue? This might bitrot them...

Comment 6

[Jack Trocinski [:jtrocinski]]

sorry guys I been preoccupied doing something else, should have posted earlier.  you can put someone else on this don't have time atm.

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

I couldn't understand. If I remove the code[1], and |object_prototype_names|, how should I deal with this code[2]?

[1]http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#1293
[2]http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#1296

Comment 8

[Bobby Holley (:bholley)]

(In reply to Peiyong Lin[:lpy](UTC-8) from comment #7)
> I couldn't understand. If I remove the code[1], and
> |object_prototype_names|, how should I deal with this code[2]?
> 
> [1]http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#1293
> [2]http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#1296

It would be replaced with a call to getOrCreateObjectPrototype.

I'm actually refactoring all this code right now though, so it's probably not a great thing for a new contributor to work on. Removing the flag.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: bug-942037-rm-object_prototype_names-v1.patch

bholley, I just found this patch in my queue, and it's more or less exactly what you said, so... do you care if I check this in? If going to conflict up your other work, no worries, I'm not attached to it. It seems pretty nicely localized though. :)

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8380018 [details] [diff] [review]
bug-942037-rm-object_prototype_names-v1.patch

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

Yeah it's fine to do this now - the stuff that it would conflict with has already landed (the ensureConstructor stuff).

::: js/src/jsapi.cpp
@@ +1285,5 @@
> +    // more way: its prototype chain is lazily initialized. That is,
> +    // global->getProto() might be null right now because we haven't created
> +    // Object.prototype yet. We force it here. Otherwise, `this.toString()` as
> +    // the first script in a new global would fail.
> +    return global->getOrCreateObjectPrototype(cx) != nullptr;

Why do we unconditionally return |true| if we successfully resolve Object.prototype?

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Bobby Holley (:bholley) from comment #10)
> Why do we unconditionally return |true| if we successfully resolve
> Object.prototype?

Resolve hooks return true on success, even if no property is found.

They only return false if an error occurred.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > Why do we unconditionally return |true| if we successfully resolve
> > Object.prototype?
> 
> Resolve hooks return true on success, even if no property is found.
> 
> They only return false if an error occurred.

Ok. But to be consistent with the old behavior, don't we still need to set *resolved to true if the property ends up being something on the prototype? Or do callers not rely on that?

Comment 13

[Jason Orendorff [:jorendorff]]

Callers definitely don't rely on that, because in the existing code, we already don't even look at object_prototype_names unless global->getProto() is null, which happens at most once per global.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> Callers definitely don't rely on that, because in the existing code, we
> already don't even look at object_prototype_names unless global->getProto()
> is null, which happens at most once per global.

Right, but once global->getProto() is non-null, we'd never end up in the resolve hook for things that we might find on the prototype, right?

Suppose we trigger lookupGeneric for "valueOf" on a global for whom Object.prototype has not yet been created. Will we trigger the creation of Object.prototype before the resolve hook? If so, why do we need this in the resolve hook at all? If not, I'm worried that stuff like [1] will start returning null where it didn't before.

I'm totally down to simplify this somehow - I just want to make sure we do it right. ;-)

[1] http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#2161

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to Bobby Holley (:bholley) from comment #14)
> (In reply to Jason Orendorff [:jorendorff] from comment #13)
> > Callers definitely don't rely on that, because in the existing code, we
> > already don't even look at object_prototype_names unless global->getProto()
> > is null, which happens at most once per global.
> 
> Right, but once global->getProto() is non-null, we'd never end up in the
> resolve hook for things that we might find on the prototype, right?

We certainly could!

The code for lookups is in js/src/jsobj.cpp, these three functions:
  LookupPropertyWithFlagsInline
    -> LookupOwnPropertyWithFlagsInline
         -> CallResolveOp

Note that the resolve hook is called *before* searching the prototype chain.

> Suppose we trigger lookupGeneric for "valueOf" on a global for whom
> Object.prototype has not yet been created. Will we trigger the creation of
> Object.prototype before the resolve hook?

No.

> If not, I'm worried that stuff like [1] will start
> returning null where it didn't before.
>
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.
> cpp#2161

It could, but that would be a pure win, assuming the end result is that a JSNewResolveOp returns true with a null *objp rather than a non-null one.

All resolve hooks, both old and new, should ignore the prototype chain. In both cases, the system is going to walk the prototype chain anyway.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
> All resolve hooks, both old and new, should ignore the prototype chain. In
> both cases, the system is going to walk the prototype chain anyway.

Ok. Then why do we need this in JS_ResolveStandardClass at all then? Shouldn't the subsequent prototype walk on the global cause us to ensure that the prototype is set up? Or is this really the only place that this happens?

Comment 17

[Jason Orendorff [:jorendorff]]

> Shouldn't the subsequent prototype walk on the global cause us to ensure that the prototype is set up?

Maybe. Unfortunately, getProto() by itself won't force the global's prototype to be lazily filled in.

This optimization is not fully transparent. It never has been. Ancient badness.

Comment 18

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78f1a88a4048

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78f1a88a4048