Closed Bug 1288791 Opened 4 years ago Closed 4 years ago

Binding code can call JS_NewObjectWithGivenProto with a gray proto

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(3 files, 1 obsolete file)

Most simply, consider an object which returns an nsIGlobalObject* from GetParentObject.  We'll call GetGlobalJSObject() on it, which does NOT ungray at least on window, and return that.  Then we'll get the default proto from it, but if the global is gray the proto will be too.
Attachment #8773898 - Attachment is obsolete: true
Attachment #8773898 - Flags: review?(bkelly)
Boris, can you help me understand the impact of returning a gray thing from these APIs?  The changes all seem consistent and ok, but I want to make sure I understand whats happening.
Flags: needinfo?(bzbarsky)
(In reply to Ben Kelly [:bkelly] from comment #5)
> Boris, can you help me understand the impact of returning a gray thing from
> these APIs?  The changes all seem consistent and ok, but I want to make sure
> I understand whats happening.

The cycle collector requires as an invariant that a black object never point to a gray object ("the black-gray invariant"). We want to maintain that invariant by ensuring that we never write a gray object into a black object. We do that by making sure that any JS object we are going to do anything substantial with is black, so we don't have to worry about whether we can write it or not.

The black-gray invariant is needed because the CC assumes that any gray object is not reachable from a JS root. (It does this to avoid tracing black JS objects, except in an all-traces CC for debugging.)
Thanks, that makes sense.
Flags: needinfo?(bzbarsky)
Attachment #8773897 - Flags: review?(bkelly) → review+
Comment on attachment 8774012 [details] [diff] [review]
part 2.  Rename WrapNativeParent to FindAssociatedGlobal and update it to actually do that

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

::: js/xpconnect/crashtests/786142-iframe.html
@@ +24,5 @@
>    // getting to scope 3 (as explained below). This means that we can't trigger
>    // the PreCreate hook for |input|, because that will call WrapNativeParent
> +  // (Well... No, it won't: there is no WrapNativeParent, but there are also no
> +  // more pre-create hooks, slimwrappers, parenting to the form, or any of the
> +  // stuff this test is trying to test.)

Should we just remove the test?
Attachment #8774012 - Flags: review?(bkelly) → review+
Attachment #8773899 - Flags: review?(bkelly) → review+
> Should we just remove the test?

Hard to say.  It's still testing an interesting situation, so removing it would lose _some_ test coverage.  Just the particular codepaths it was trying to test no longer exist.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1188f77967d
part 1.  Rename the GetParentObject template bits to make it clearer what they're really doing: finding the associated global for an object's native.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/45010c0db1c5
part 2.  Rename WrapNativeParent to FindAssociatedGlobal and update it to actually do that.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c51ccab098
part 3.  Ensure that FindAssociatedGlobal never returns a gray global.  r=bkelly
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.