Closed
Bug 1127475
Opened 10 years ago
Closed 10 years ago
Remove unnecessary parent arguments
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(1 file)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Most of the time parent can be null, in which case we automatically use cx->global().
See also bug 1127443 comment 7.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evilpies
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
The point is that comment 1 shows that the parent being passed at both these callsites is the compartment global.
Assignee | ||
Comment 4•10 years ago
|
||
What would be the consequence if the parent wasn't a global? I think I have definitely seen some instances of that.
Assignee | ||
Updated•10 years ago
|
Assignee: evilpies → nobody
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evilpies
Comment 5•10 years ago
|
||
> 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.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8563588 -
Flags: review?(bzbarsky)
Comment 7•10 years ago
|
||
Comment on attachment 8563588 [details] [diff] [review]
remove-unnecessary-parent
r=me
Attachment #8563588 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Comment 9•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•