Avoid wrapping dead wrappers into CCWs

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: till, Assigned: kmag)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

nsXPCComponents_Utils::IsDeadWrapper has special handling for dead wrappers because they can be rewrapped into normal wrappers. See this comment:
http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/js/xpconnect/src/XPCComponents.cpp#2844-2846

According to bholley, this could be avoided by checking for dead wrappers in PrepareForUnwrapping and just returning a new dead wrapper.

There's more code that could be simplified after this happens, following the same pattern as in IsDeadWrapper.
Duplicate of this bug: 1341488
I may as well handle this and bug 1341488 at the same time.
Assignee: nobody → kmaglione+bmo
See Also: → 1354843
Comment on attachment 8856144 [details]
Bug 1354733: Part 2 - Never rewrap dead wrappers.

https://reviewboard.mozilla.org/r/128092/#review130596
Attachment #8856144 - Flags: review?(bobbyholley) → review+
Comment on attachment 8856143 [details]
Bug 1354733: Part 1 - Allow creating DeadObjectProxies directly.

https://reviewboard.mozilla.org/r/128090/#review135738

efaust isn't at mozilla anymore so might be slow to respond, so I'm stealing this. r=me with the one nit addressed.

::: js/src/proxy/DeadObjectProxy.cpp:166
(Diff revision 1)
> +JSObject*
> +js::NewDeadProxyObject(JSContext* cx)
> +{
> +    RootedValue val(cx, NullValue());
> +
> +    return NewProxyObject(cx, &DeadObjectProxy::singleton, val, nullptr, ProxyOptions());

You can use `NullHandleValue` here instead of creating a rooted null value.
Attachment #8856143 - Flags: review+
Attachment #8856143 - Flags: review?(efaustbmo)
Any reason this can't land?
Flags: needinfo?(kmaglione+bmo)
Oh, sorry, I got the review canceled email, but I didn't realize you'd also given an r+, or I would have gotten back to it sooner.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8856143 [details]
Bug 1354733: Part 1 - Allow creating DeadObjectProxies directly.

Needs another review after deconflicting with bug 1356691.
Attachment #8856143 - Flags: review+ → review?(till)
Comment on attachment 8856143 [details]
Bug 1354733: Part 1 - Allow creating DeadObjectProxies directly.

https://reviewboard.mozilla.org/r/128090/#review144888

::: js/src/proxy/DeadObjectProxy.cpp:215
(Diff revision 2)
> +js::NewDeadProxyObject(JSContext* cx, JSObject* origObj)
> +{
> +    MOZ_ASSERT_IF(origObj, origObj->is<ProxyObject>());
> +
> +    const BaseProxyHandler* handler;
> +    if (origObj && origObj->is<ProxyObject>())

The second part of this condition is redundant with the assert above.
Attachment #8856143 - Flags: review?(till) → review+
Comment on attachment 8856143 [details]
Bug 1354733: Part 1 - Allow creating DeadObjectProxies directly.

https://reviewboard.mozilla.org/r/128090/#review144888

> The second part of this condition is redundant with the assert above.

Yup, but it's not a release assert, so if we ever pass a non-proxy value that gets past a debug build, not checking could result in memory corruption.
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #14)
> Yup, but it's not a release assert, so if we ever pass a non-proxy value
> that gets past a debug build, not checking could result in memory corruption.

Good point. Though you could also just make it a release assert :) r=me either way, though.
(In reply to Till Schneidereit [:till] from comment #15)
> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment
> #14)
> > Yup, but it's not a release assert, so if we ever pass a non-proxy value
> > that gets past a debug build, not checking could result in memory corruption.
> 
> Good point. Though you could also just make it a release assert :) r=me
> either way, though.

I think I'd rather leave it a debug assert. The worst that can happen in that case is that we wind up with a dead wrapper with the wrong callable or constructable attributes, which doesn't seem like a good enough reason to crash a release build.
https://hg.mozilla.org/mozilla-central/rev/175406e49282
https://hg.mozilla.org/mozilla-central/rev/bb3ede350505
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.