Closed Bug 1251480 Opened 8 years ago Closed 8 years ago

Remove XMLHttpRequest::StateData's CustomAutoRooter

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We shouldn't need this CustomAutoRooter anymore.
This was my first attempt. It's not straightforward because the object is held in an nsAutoPtr<T>, and we want to use a Rooted<T*>. This is the straightforward way to go about it, relying on the nested destructors to fire in the right order.
Specialize for UniquePtr. One part of this is weird -- MapTypeToRootKind<UniquePtr<T>> delegates to MapTypeToRootKind<T>. Another alternative would be to just hardcode it to Traceable, on the assumption that UniquePtr<JSObject> is pretty much useless (without a *really* funky deleter, I suppose.) I don't entirely understand what the regular pointer specialization is doing, but perhaps it might make sense?
Attachment #8723880 - Flags: review?(terrence)
Attachment #8723875 - Attachment is obsolete: true
And here's the actual user of Rooted<UniquePtr<T>>. Giving review to bz mostly as an FYI that this is now possible. Assuming terrence is ok with the earlier patch.
Attachment #8723881 - Flags: review?(bzbarsky)
Comment on attachment 8723880 [details] [diff] [review]
Implement Rooted<UniquePtr<T>>

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

Nice!

::: js/public/GCPolicyAPI.h
@@ +122,5 @@
> +        tp->get()->trace(trc);
> +    }
> +    static bool needsSweep(mozilla::UniquePtr<T>* tp) {
> +        return tp->get()->needSweep();
> +    }

So I guess this is hard-coded to assume that T would use a StructGCPolicy. Could we instead write these all as:

return GCPolicy<T>::foo(t->get());

And add a comment at the top that explains somehow that the GCPolicy for UniquePtr<T> is to look through the UniquePtr and use the policy for T.

::: js/public/TraceKind.h
@@ +139,5 @@
>          JS::MapTraceKindToRootKind<JS::MapTypeToTraceKind<T>::kind>::kind;
>  };
> +template <typename T>
> +struct MapTypeToRootKind<mozilla::UniquePtr<T>> {
> +    static const JS::RootKind kind = JS::MapTypeToRootKind<T>::kind;

Ah, just like this.
Attachment #8723880 - Flags: review?(terrence) → review+
Comment on attachment 8723881 [details] [diff] [review]
Use Rooted<UniquePtr<StateData>> in place of StateData::CustomAutoRooter

r=me
Attachment #8723881 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/d0d2cd5f762b
https://hg.mozilla.org/mozilla-central/rev/80869f1474a4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.