940505 - Fix exact rooting hazard in XPCMaps.h

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: xpc-rooting-patch

There's an assertion in JSObject2JSObjectMap::Add() that calls xpc::GetObjectScope().  The analysis thinks this could GC because GetObjectScope() calls EnsureCompartmentPrivate() which could in theory create a CompartmentPrivate for the compartment the object is in.  In practice this won't happen as the assertion would fail in that case anyway.

Instead, we can just get the compartment private without the possibility of creating it.  xpc::GetCompartmentPrivate() will assert if it hasn't been created.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 8334654 [details] [diff] [review]
xpc-rooting-patch

Bobby should probably look at this, too.

Comment 2

[Bobby Holley (:bholley)]

As soon as JSD is gone, we can get rid of all the lazy CompartmentPrivate stuff.

Comment 3

[Terrence Cole [:terrence]]

Attachment: handlify_GetXrawWaiver-v0.diff

While investigating, I handlified GetXrayWaiver to clarify what exactly was rooted near these paths. It is trivial, so I think we should keep it.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8334706 [details] [diff] [review]
handlify_GetXrawWaiver-v0.diff

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

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +48,5 @@
>             Wrapper::wrapperHandler(obj) == &ChromeObjectWrapper::singleton;
>  }
>  
>  JSObject *
> +WrapperFactory::GetXrayWaiver(JS::HandleObject obj)

no JS:: namespacing in XPConnect cpp files.

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4973ab787729

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4973ab787729

Comment 7

[Jon Coppeard (:jonco)]

Reopening as the first patch hasn't landed yet.

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/017d3e4e80bf

Comment 9

[Ed Morley [:emorley]]

Unfortunately this and the other bugs in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=db0f8a5eeb33 have been backed out for causing rootanalysis assertions, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=30835010&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=05a0228c2caa

(For quick relanding, I recommend the third party qbackout extension and '--apply' mode)

Comment 10

[Jon Coppeard (:jonco)]

Relanding:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8220e44cf03

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a8220e44cf03