Closed Bug 1264377 Opened 8 years ago Closed 8 years ago

Get rid of some unnecessary jsclass hooks for sandboxes

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Whiteboard: btpp-active)

Attachments

(1 file)

sandboxe_enumerate just calls JS_EnumerateStandardClasses, for example.

Also, going to give sandbox and simple global a sane mayresolve hook while I'm here.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8741051 [details] [diff] [review]
Get rid of some unnecessary custom JSClass hook functions in xpconnect sandboxes and DOM simple globals

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

r=me modulo that.

::: dom/bindings/SimpleGlobalObject.cpp
@@ +63,5 @@
>      nullptr,
>      nullptr,
> +    JS_EnumerateStandardClasses,
> +    JS_ResolveStandardClass,
> +    JS_MayResolveStandardClass,

I haven't seen this last hook before. Are you sure it's the right thing rather than nullptr?
Attachment #8741051 - Flags: review?(bobbyholley) → review+
Whiteboard: btpp-active
> Are you sure it's the right thing rather than nullptr?

Yes.  The mayResolve hook is used by the JIT to decide whether it can optimize access to a property.  It needs to either not exist (in which case no property access will be optimized on the object if it has a resolve hook), or exist and return true for any property that the resolve hook might resolve (and then just those properties will fail to be optimized).

If your resolve hook is JS_ResolveStandardClass, then using JS_MayResolveStandardClass for the mayResolve hook is the right thing to do.  Using null won't cause correctness issues, but can lead to suboptimal performance.
https://hg.mozilla.org/mozilla-central/rev/7dd74bf49527
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.