Use CallbackPreserveColor() less in bindings code

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
DOM bindings codegen uses CallbackPreserveColor() on callbacks that it knows should be already non-gray. The reason for this is not immediately obvious when looking at the call sites, and nothing actually verifies this property. It seems like there should be some kind of CallbackAlreadyNonGray() method (name suggestions welcome) that is like CallbackPreserveColor(), but asserts that the object is non-gray.
Sure, having a CallbackKnownNotGray() would be fine.
(Assignee)

Updated

2 years ago
Assignee: nobody → continuation
(Assignee)

Comment 2

2 years ago
CallbackObject::CallSetup::CallSetup() does this:
  JSObject* realCallback = js::UncheckedUnwrap(aCallback->CallbackPreserveColor());
The callback object is gray here sometimes, for instance during ./mach package, but I think it is okay because the uses of realCallback only grab the C++ native off of the object, AFAICT.  UncheckUnwrap itself can do a write to the gray object, but that should be okay, because we're only trying to avoid black->gray edges. Seems a little fragile but I'll leave it alone.
(Assignee)

Comment 3

2 years ago
Created attachment 8770702 [details] [diff] [review]
Use CallbackPreserveColor() less.

Some places call CallbackPreserveColor() because they know the object must have already been marked black. This patch replaces those calls with a new function CallbackKnownNotGray() which documents and dynamically checks this assumption.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1204210b2817
Attachment #8770702 - Flags: review?(bzbarsky)
Comment on attachment 8770702 [details] [diff] [review]
Use CallbackPreserveColor() less.

r=me
Attachment #8770702 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 5

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/347b46d43b8b
Use CallbackPreserveColor() less. r=bz
Keywords: checkin-needed

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/347b46d43b8b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.