Closed Bug 1127475 Opened 9 years ago Closed 9 years ago

Remove unnecessary parent arguments

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(1 file)

Most of the time parent can be null, in which case we automatically use cx->global().
See also bug 1127443 comment 7.
Depends on: 1127443
Assignee: nobody → evilpies
OK, so the two cases:

1)  nsWindowSH::GlobalResolve.  This is called from one place only: nsGlobalWindow::DoResolve.  The "obj" passed in is the aObj argument to DoResolve.    

DoResolve is called from bindings code only and passes either the "obj" of the resolve hook or the "obj" or "wrapper" passed to ResolveOwnPropertyViaResolve.  So in all cases it's either the JSObject for the (inner) window or the JSObject for the Xray.  Importantly, cx is in the compartment of obj.

Since the JS_NewObject call here is conditioned on !xpc::IsXrayWrapper(obj), we know that "obj" is in fact the window and hence obj == cx->global(), so we can drop it: that will be the default parent anyway.  We can add an assert, I guess.

2)  xpc::NewOutObject.  This passes the return value of JS_GetGlobalForObject(cx, scope).  JS_GetGlobalForObject asserts cx and obj are same-compartment, so JS_GetGlobalForObject(cx, scope) is the same thing as cx->global().  So again, we can elide the parent arg (and drop the "scope" arg to NewOutObject, for that matter).

Even if we're worried that maybe we only hit this codepath in an opt build when cx and scope are not same-compartment, xpc::NewOutObject only has two callers.  One caller is in WrapperAnswer.cpp and looks like this:

420             JSCompartment *compartment = js::GetContextCompartment(cx);
421             RootedObject global(cx, JS_GetGlobalForCompartmentOrNull(cx, compartment));
422             RootedObject obj(cx, xpc::NewOutObject(cx, global));

OK, so that one is clearly cx->global().  We should be able to just kill off lines 420/421!

The second caller is in XPCWrappedJSClass.cpp and is:

1122             RootedObject out_obj(cx, NewOutObject(cx, obj));

This is in the scope of a "JSAutoCompartment ac(cx, obj)" and no one has messed with obj or other compartment stuff, so cx and obj are certainly same-compartment.
Blocks: 805052
We already assert that proto and parent are the same compartment as cx, so this probably trivial as a custom parent on a normal object should not matter.
The point is that comment 1 shows that the parent being passed at both these callsites is the compartment global.
Depends on: 1131801
What would be the consequence if the parent wasn't a global? I think I have definitely seen some instances of that.
Assignee: evilpies → nobody
No longer depends on: 1131801
Assignee: nobody → evilpies
> What would be the consequence if the parent wasn't a global?

Then it would need to be passed in explicitly, no?  The whole point is that if no parent is passed the compartment global is used.  So if that's what you want, you don't have to pass anything.
Attachment #8563588 - Flags: review?(bzbarsky)
Comment on attachment 8563588 [details] [diff] [review]
remove-unnecessary-parent

r=me
Attachment #8563588 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/c80e833d780e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: