1040181 - Transplant crash when setting document.domain with a cross-compartment eval reference

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

We have belt-and-suspenders code in WrapperFactory::Rewrap that checks if somebody is trying to wrap a cross-origin |eval| or |Function|, and makes the wrap fail. This is unfortunate, because we also have transplant code that MOZ_CRASHes when the transplant fails. This makes me hit a MOZ_CRASH in js::RemapWrapper, which might be related to the crashes we're seeing in bug 856670.

Instead of returning null, we should just return an opaque wrapper. It would also be nice to redesign the callback to return a Wrapper& to make it clearer that it should never intentionally return null.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Use an opaque wrapper rather than failing in Rewrap. v1

Comment 2

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8458280 [details] [diff] [review]
Use an opaque wrapper rather than failing in Rewrap. v1

Review of attachment 8458280 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this seems a lot nicer approach than a hard crash.

::: js/xpconnect/tests/chrome/test_documentdomain.xul
@@ +51,5 @@
>      win1A.storeReference(win1B);
>      win1B.storeReference(win1A);
>      ok(win1A.tryToAccessStored(), "Stored references work when same-origin");
> +    win1A.evalFromB = Cu.unwaiveXrays(win1B.eval); // Crashtest for bug 1040181.
> +    win1B.functionFromA = Cu.unwaiveXrays(win1A.Function); // Crashtest for bug 1040181.

A call attempt would be nice...

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=5816b069362d

Comment 4

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb5b8d67c00

KaiRo, once this lands and settles, can you check whether it has any impact on the crash volume for bug 856670?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fb5b8d67c00

Comment 6

[Robert Kaiser]

(In reply to Bobby Holley (:bholley) from comment #4)
> KaiRo, once this lands and settles, can you check whether it has any impact
> on the crash volume for bug 856670?

Looking at the graph tab on https://crash-stats.mozilla.com/report/list?signature=js::RemapWrapper%28JSContext*,%20JSObject*,%20JSObject*%29 for nightly and a little longer time range doesn't look like anything changed around 7/20.

Also, looking at the build ID facet of https://crash-stats.mozilla.com/search/?signature=~js%3A%3ARemapWrapper&release_channel=nightly&date=>2014-05-01&_facets=build_id&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform and actually sorting by build ID also does not imply a change there.