1174861 - Remove unnecessary Rooted from Prefable::isEnabled()

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Remove unnecessary Rooted from Prefable::isEnabled().

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...

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Andrew McCreight [:mccr8]]

(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).

Comment 4

[Bobby Holley (:bholley)]

(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?

Comment 5

[Andrew McCreight [:mccr8]]

(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 6

[Andrew McCreight [:mccr8]]

Attachment: Remove unnecessary Rooted from Prefable::isEnabled().

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8622793 [details] [diff] [review]
Remove unnecessary Rooted from Prefable::isEnabled().

Patch without the invalid hoisting of GetGlobalForObjectCrossCompartment(). carrying forward the r+.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05bd2af568e2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05bd2af568e2