Closed Bug 861022 Opened 11 years ago Closed 11 years ago

Root WebIDL prototype and interface object setup

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

I think I'll end up doing this in two steps: root the non-global bits, then root the global.
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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.
Attachment #736639 - Flags: review?(terrence) → review+
Attachment #736640 - Flags: review?(terrence) → review+
(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);
> }
> 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?
Flags: needinfo?(terrence)
Double-period at the end, otherwise it's perfect, thanks!
Flags: needinfo?(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]:
-----------------------------------------------------------------

Ditto on a comment explaining the deal with ProtoGetter returning a Handle.
Attachment #736639 - Flags: review?(peterv) → review+
Blocks: 865969
Depends on: 860841
Attachment #736640 - Flags: review?(peterv) → review+
Blocks: 867844
OK, I'm reversing the order of this and bug 860841, because I need to get this landed....
Blocks: 860841
No longer depends on: 860841
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.