1354733 - Avoid wrapping dead wrappers into CCWs

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Till Schneidereit [:till]]

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.

Comment 1

[Bobby Holley (:bholley)]

Correction: PrepareForWrapping, not PrepareForUnwrapping:

http://searchfox.org/mozilla-central/rev/78cefe75fb43195e7f5aee1d8042b8d8fc79fc70/js/xpconnect/wrappers/WrapperFactory.cpp#158

Comment 3

[Kris Maglione [:kmag]]

I may as well handle this and bug 1341488 at the same time.

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1354733: Part 1 - Allow creating DeadObjectProxies directly.

Review commit: https://reviewboard.mozilla.org/r/128090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128090/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1354733: Part 2 - Never rewrap dead wrappers.

Review commit: https://reviewboard.mozilla.org/r/128092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128092/

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8856144 [details]
Bug 1354733: Part 2 - Never rewrap dead wrappers.

https://reviewboard.mozilla.org/r/128092/#review130596

Comment 7

[Till Schneidereit [:till]]

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.

Comment 8

[Till Schneidereit [:till]]

Any reason this can't land?

Comment 9

[Kris Maglione [:kmag]]

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.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8856143 [details]
Bug 1354733: Part 1 - Allow creating DeadObjectProxies directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128090/diff/1-2/

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8856144 [details]
Bug 1354733: Part 2 - Never rewrap dead wrappers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128092/diff/1-2/

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8856143 [details]
Bug 1354733: Part 1 - Allow creating DeadObjectProxies directly.

Needs another review after deconflicting with bug 1356691.

Comment 13

[Till Schneidereit [: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.

Comment 14

[Kris Maglione [:kmag]]

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.

Comment 15

[Till Schneidereit [:till]]

(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.

Comment 16

[Kris Maglione [:kmag]]

(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.

Comment 17

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/175406e492828d852b4afe5c43f93f894970c819
Bug 1354733: Part 1 - Allow creating DeadObjectProxies directly. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3ede3505050d44e2aa933b82def8dc8501a5de
Bug 1354733: Part 2 - Never rewrap dead wrappers. r=bholley

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/175406e49282
https://hg.mozilla.org/mozilla-central/rev/bb3ede350505