Closed Bug 1308919 Opened 3 years ago Closed 3 years ago

Creating a Handle to a Heap<T> bypasses the read barrier


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox52 --- fixed


(Reporter: jonco, Assigned: jonco)



(1 file, 2 obsolete files)

Attached patch add-heap-to-handle (obsolete) — Splinter Review
Following on from bug 1297558, there's still a spot where we can read the contents of a Heap<T> without triggering the read barrier: sometimes we call address() and use this to create a Handle.

Here's a patch that adds a toHandle() method which does trigger the read barrier.  This also lets us get rid of a few Handle::fromMarkedLocation calls.
Attachment #8799389 - Flags: review?(sphink)
Comment on attachment 8799389 [details] [diff] [review]

Requesting review for the browser parts of this.
Attachment #8799389 - Flags: review?(continuation)
Comment on attachment 8799389 [details] [diff] [review]

Review of attachment 8799389 [details] [diff] [review]:

r=me for the browser bits, except for the changes.

::: dom/bindings/
@@ -3062,5 @@
>               * Calling fromMarkedLocation() is safe because protoAndIfaceCache is
>               * traced by TraceProtoAndIfaceCache() and its contents are never
>               * changed after they have been set.
>               */
> -            return JS::Handle<JSObject*>::fromMarkedLocation(protoAndIfaceCache.EntrySlotMustExist(${id}).address());

Please get bz to review this change. If I understand correctly, this is adding a new exposeToJS() call here, and I'm not sure how sensitive this bit is.

::: dom/xbl/nsXBLProtoImplProperty.cpp
@@ +358,4 @@
>    MOZ_ASSERT_IF(mJSAttributes & (JSPROP_GETTER | JSPROP_SETTER), mIsCompiled);
>    if (mJSAttributes & JSPROP_GETTER) {
> +    rv = XBL_SerializeFunction(aStream, mGetter.AsHeapObject().toHandle());

Could you make a new AsHandle() method and call that everywhere instead of AsHeapObject().toHandle()? That would be a little nicer, I think.
Attachment #8799389 - Flags: review?(continuation) → review+
Boris, could you take a look at the patch changes in The rest looks fine to me. I just don't know if adding an expose to JS call in there might be a problem or not. Thanks.
Flags: needinfo?(bzbarsky)
(That part of the patch is a single line, FWIW.)
First of all, please add a comment to CallbackObject::CallbackPreserveColor explaining why it's not using the convenient toHandle() API, so no one tries to "fix" it to do that.

Second, it's not clear to me that the changes in XULDocument::ExecuteScript are safe.  In particular, they're assuming that gc cannot happen between the GetScriptObject() call and the JS::CloneAndExecuteScript call.  That's entirely non-obvious.  It would be better to reget the scriptobject at the JS::CloneAndExecuteScript callsite, unmarking gray there as needed, with comments explaining what's going on.

Note that this is a general problem with pushing the expose down into this handle-getting API.  Nothing ensures that the thing _stays_ exposed.  That suggests to me that in fact this whole pattern of "create a Handle from a cycle-collected Heap<T>" is just wrong, and we should explicitly not support it.  I'm pretty sure the CallbackObject code is carefully audited to make sure it's safe, but it might be double-checking again.  All the other non-codegen places here (XUL, XBL) should just switch to using a Rooted for their thing as needed, and for the boolean tests having a method that internally does an unsafeGet() or something.

Anyway, for the change, the situation is different because these Heap values are GC-traced, not CC-traced.  I would like to understand the performance implications of it, given that the following statements are true:

1)  We got the protoAndIfaceCache from the return value of JS::CurrentGlobalOrNull.
2)  The return value of JS::CurrentGlobalOrNull is never gray
    (we have asserts in JSAutoCompartment now).
3)  The trace hook for a DOM global is JS_GlobalObjectTraceHook,
    which calls global->compartment()->creationOptions().getTrace() (if any), which for all the objects that
    have a protoAndIfaceCache will trace the protoAndIfaceCache.
4)  The trace hooks for all out other globals also end up tracing the protoAndIfaceCache, some via the
    creation options, some directly (e.g. see XPCWrappedNative::Trace and the things it calls).

So as far as I can tell, we can't have a gray thing here anyway.  Given that, how fast will the check that we don't have one be?  Can we just assert that it's not gray and avoid exposing?  This code is mostly hot in the "allocate a new DOM object" codepath, which is already doing various things like object-allocation, so it's probably not fatal if we do a fast "check that this is not already gray" thing here, but if we can avoid it, so much the better.
Flags: needinfo?(bzbarsky)
Attachment #8799389 - Flags: review?(sphink)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> That suggests to me that in fact this whole pattern of "create a Handle from a
> cycle-collected Heap<T>" is just wrong, and we should explicitly not support
> it. 

I totally agree (and I'm not sure why we had this in the first place).
Summary: Add a read barrier when making a Handle to a Heap<T> → Creating a Handle to a Heap<T> bypasses the read barrier
We had it in some places (the binding codegen, CallbackObject) because those were very performance-sensitive.  Then people cargo-culted.  :(
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
Ah, right.

I added an assertion that the object was not gray for the codegen case, but it's failing occasionally:
What's failing in that log is the "this object is not null" assert in CellsIsMarkedGray, not the "this object is not gray" assert in its caller.

Note that the comment right above the code you added says:

                * The object might _still_ be null, but that's OK.

In practice what that means is that we hit OOM when trying to allocate the object, and caller will null-check and propagate the error out.
Thoguh the fact that we're ooming here so consistently is a bit disconcerting.
> What's failing in that log is the "this object is not null" assert in
> CellsIsMarkedGray, not the "this object is not gray" assert in its caller.

Ah right, indeed.

Here's a new patch to remove making Handles from Heap<T> pointers in XBL and XUL code and to add an assert that object is never gray in codegen.

Try run:
Attachment #8799389 - Attachment is obsolete: true
Please don't add new AutoJSContext uses.  If you just need a cx for rooting, mozilla::dom::RootingCx() (probably usable in these files without the namespace bits) will get you a useful thing.
bz, would you like to review this?
Flags: needinfo?(bzbarsky)
Comment on attachment 8800704 [details] [diff] [review]
bug1308919-remove-handles-to-heapt v2

I guess I've read through this a few times, so I really should... ;)

>+             * Calling address() avoids the read read barrier that does grey
>+             * unmarking, but it's not possible for the object to be gray here.

Please spell "gr[ea]y" consistently within at least that once sentence.  Probably "gray", since that's what the JSAPI uses.

Flags: needinfo?(bzbarsky)
Attachment #8800704 - Flags: review+
Pushed by
Don't make Handles to Heap<T> as it avoids the read barrier r=bz
Pushed by
Fix typo in comments r=bz on a CLOSED TREE
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.