Closed Bug 940505 Opened 6 years ago Closed 6 years ago

Fix exact rooting hazard in XPCMaps.h

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jonco, Assigned: jonco)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

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.
Attachment #8334654 - Flags: review?(terrence)
Comment on attachment 8334654 [details] [diff] [review]
xpc-rooting-patch

Bobby should probably look at this, too.
Attachment #8334654 - Flags: review?(bobbyholley+bmo)
As soon as JSD is gone, we can get rid of all the lazy CompartmentPrivate stuff.
Attachment #8334654 - Flags: review?(terrence)
Attachment #8334654 - Flags: review?(bobbyholley+bmo)
Attachment #8334654 - Flags: review+
While investigating, I handlified GetXrayWaiver to clarify what exactly was rooted near these paths. It is trivial, so I think we should keep it.
Attachment #8334706 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8334706 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/4973ab787729
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Reopening as the first patch hasn't landed yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
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)
https://hg.mozilla.org/mozilla-central/rev/a8220e44cf03
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.