Closed Bug 942037 Opened 6 years ago Closed 6 years ago

Remove object_prototype_names

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bholley, Assigned: jorendorff)

References

Details

Attachments

(1 file)

This should be trivial. See bug 933681 comment 19.
Whiteboard: [mentor=bholley,lang=c++]
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?
(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.
I'd like to be assigned to this bug, could someone assign me please.
Assignee: nobody → jack.trocinski
Jack, can you work on this?
(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...
sorry guys I been preoccupied doing something else, should have posted earlier.  you can put someone else on this don't have time atm.
Assignee: jack.trocinski → nobody
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
(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.
Whiteboard: [mentor=bholley,lang=c++]
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. :)
Attachment #8380018 - Flags: review?(bobbyholley)
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?
Attachment #8380018 - Flags: review?(bobbyholley)
(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.
(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?
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.
Attachment #8380018 - Flags: review?(bobbyholley)
(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
(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.
(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?
> 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.
Attachment #8380018 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/78f1a88a4048
Assignee: nobody → jorendorff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.