861022 - Root WebIDL prototype and interface object setup

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I think I'll end up doing this in two steps: root the non-global bits, then root the global.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Root the non-globals in WebIDL prototype and interface object setup.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Root the global object in WebIDL prototype and interface object setup.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 736639 [details] [diff] [review]
part 1.  Root the non-globals in WebIDL prototype and interface object setup.

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

::: dom/bindings/BindingUtils.cpp
@@ +554,1 @@
>    }

This may be venturing into unreasonable paranoia.

::: dom/bindings/DOMJSClass.h
@@ +151,5 @@
>    eInterfacePrototype
>  };
>  
>  typedef JSObject* (*ParentGetter)(JSContext* aCx, JSObject* aObj);
> +typedef JS::Handle<JSObject*> (*ProtoGetter)(JSContext* aCx, JSObject* aGlobal);

It's not immediately obvious (or really apparent at all), why it is that the referenced proto will always outlive the returned handle. It would be good to have a comment somewhere explaining how this works and stating the invariants required for it to continue to working in the future.

Comment 4

[Terrence Cole [:terrence]]

(In reply to Terrence Cole [:terrence] from comment #3)
> Comment on attachment 736639 [details] [diff] [review]
> part 1.  Root the non-globals in WebIDL prototype and interface object setup.
> 
> Review of attachment 736639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/BindingUtils.cpp
> @@ +554,1 @@
> >    }
> 
> This may be venturing into unreasonable paranoia.

Heh, I was expecting splinter to include 5 lines of context, as it normally does. To be clear, this was supposed to be:

> else {
>   MOZ_ASSERT(!proto);
> }

Comment 5

[Boris Zbarsky [:bzbarsky]]

> It would be good to have a comment somewhere explaining how this works

I'll add the following comment above the typedef:

/**
 * Returns a handle to the relevent WebIDL prototype object for the given global
 * (which may be a handle to null on out of memory).  Once allocated, the
 * prototype object is guaranteed to exist as long as the global does, since the
 * global traces its array of WebIDL prototypes and constructors..
 */

Is that clearer?

Comment 6

[Terrence Cole [:terrence]]

Double-period at the end, otherwise it's perfect, thanks!

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 736639 [details] [diff] [review]
part 1.  Root the non-globals in WebIDL prototype and interface object setup.

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

Ditto on a comment explaining the deal with ProtoGetter returning a Handle.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, I'm reversing the order of this and bug 860841, because I need to get this landed....

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60e671f364b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a606d921d45a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60e671f364b9
https://hg.mozilla.org/mozilla-central/rev/a606d921d45a