Closed Bug 1174861 Opened 4 years ago Closed 4 years ago

Remove unnecessary Rooted from Prefable::isEnabled()

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file, 1 obsolete file)

It isn't entirely clear we want to handlify PropertyEnabled (bug 1173965) so in the meanwhile I might as well drop the unnecessary rooting of obj that I came across.
See Also: → 1173965
This is a very simple patch, because all of the callers already have a handle.

I also factored out the getting of the global because it seems silly to have it in three places. (We check before getting there that we have at least one function to call.)

Asking review from bholley because he is the only DOM peer with an empty review queue. ;)

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64a283b39178

Though this does not actually include a hazard analysis. But it should be okay...
Attachment #8622781 - Flags: review?(bobbyholley)
Comment on attachment 8622781 [details] [diff] [review]
Remove unnecessary Rooted from Prefable::isEnabled().

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

::: dom/bindings/DOMJSClass.h
@@ +58,5 @@
>      }
>      if (!enabledFunc && !availableFunc && !checkPermissions) {
>        return true;
>      }
> +    JSObject* global = js::GetGlobalForObjectCrossCompartment(obj);

So you're relying on the global being tenured here? I'm not sure if the static analysis is going to be smart enough to figure that out.

r=me if it is, providing you add a comment explaining that.
Attachment #8622781 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #2)
> So you're relying on the global being tenured here? I'm not sure if the
> static analysis is going to be smart enough to figure that out.
> 
> r=me if it is, providing you add a comment explaining that.

Well, in terms of GC safety, this is equivalent to the existing code: we're grabbing an unrooted pointer to something. My patch just gives this intermediate value a name.

The reason the existing code is safe is because any of these functions we call have to manually root the global themselves if they do anything that could GC (calling GetPreferences() seems to be the most common reason).
(In reply to Andrew McCreight [:mccr8] from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > So you're relying on the global being tenured here? I'm not sure if the
> > static analysis is going to be smart enough to figure that out.
> > 
> > r=me if it is, providing you add a comment explaining that.
> 
> Well, in terms of GC safety, this is equivalent to the existing code: we're
> grabbing an unrooted pointer to something. My patch just gives this
> intermediate value a name.

But the existing code grabs the unrooted pointer afresh off of the rooted object each time. So if the global were moved (which doesn't happen because of tenuring, but imagine it were), the former code would be correct, and your patch would not be.

> 
> The reason the existing code is safe is because any of these functions we
> call have to manually root the global themselves if they do anything that
> could GC (calling GetPreferences() seems to be the most common reason).

But that doesn't help call B if call A triggers a GC that moves the referent of the unrooted pointer, right?
(In reply to Bobby Holley (:bholley) from comment #4)
> But that doesn't help call B if call A triggers a GC that moves the referent
> of the unrooted pointer, right?

Oh right, you could end up calling all of the functions! I didn't think about that. I'll just revert that part of the change.
Comment on attachment 8622793 [details] [diff] [review]
Remove unnecessary Rooted from Prefable::isEnabled().

Patch without the invalid hoisting of GetGlobalForObjectCrossCompartment(). carrying forward the r+.
Attachment #8622793 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/05bd2af568e2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.