Bug 16573 – Remove redundant calls to setPrototype in WebCore

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Sam Weinig]

The way prototypes are set in the JS bindings in WebCore is inefficient and can be improved by passing the prototype up the constructor chain.  Patch forthcoming.

[Sam Weinig]

Created attachment 18057 [details]
patch

[Maciej Stachowiak]

Actually, I think the way it was is a deliberate choice, because this way can introduce a GC hazard (you might end up triggering a garbage collection while an object is partially allocated). I'm not sure if this is still a real problem or not. Possibly not, if the prorotype is known to exist already. Another possibility is to pass the prototype to the outermost constructor in the bindings code, so it can be definitely allocated before creating anything.

[Maciej Stachowiak]

I'll review in more detail tomorrow.

[Sam Weinig]

Comment on attachment 18057 [details]
patch

Maciej r- this.  

"I think this introduces a GC hazard. If getting the prototype during construction actually allocates it, then this could trigger a garbage collection while an object is half-constructed, which will crash. The current code is specifically designed to avoid this. You *could* get around this by always passing in the prototype as a constructor argument and making the generated code handle this. As long as it is allocated before you start constructing the wrapper, there is no GC hazard."

[Geoffrey Garen]

I think that passing the prototype to the outermost constructor would be a neat solution.

To answer Maciej's implicit question, there *is* a GC hazard. If GC ran, the new object would be caught with its _prop and _proto data members uninitialized. When it tried to mark those members, the weasel would go pop.

[Sam Weinig]

Created attachment 18249 [details]
take 2

This removes the GC hazard by constructing the prototype before the js object and passing it in.

[Darin Adler]

Comment on attachment 18249 [details]
take 2

I think the prototype parameter should be JSObject unless there's some reason it can be null in some cases. It can be more efficient to have the more-specific type, so I'd prefer that all the prototype code use JSObject rather than JSValue.

+JSEventTargetNode::JSEventTargetNode(KJS::JSValue* prototype, Node* n)

You should not need an explicit KJS prefix here. Please leave them off except in header files or in places where they are necessary.

For JSHTMLAudioElementConstructor you passed the ExecState and set up the prototype inside the constructor, but for JSNamedNodesCollection, JSRGBColor, and Navigator you passed the prototype in instead. I'm not sure why you did them differently, since in all cases it's the object prototype.

+JSObject* XSLTProcessorConstructorImp::construct(ExecState* exec, const List& args)

Elsewhere we call these functions "create", I think largely because we don't want to confuse them with constructors. Would you consider calling it create instead of construct?

Could the return type be JSXSLTProcessor* instead of JSObject*? In general types should always be as specific as possible unless there's a problem.

We really need to take these "Imp" suffixes off things! Can you add a lot more of them to do-webcore-rename?

+        DOMObject(JSValue* prototype)

This needs to be marked explicit because we don't want it to be invoked implicitly as a conversion (even though that seems unlikely).

+    private:
+        DOMObject();

What is this for? Does someone need to construct a DOMObject this way? Can you instead just omit this constructor?

If JSLocation's prototype is going to be null, then why change JSLocation so that you can pass the prototype in? You should have just left its constructor alone, I think.

r=me as is, because none of these problems are showstoppers

If you could take some of my requests/suggestions I'd be happy to review again.

[Sam Weinig]

> I think the prototype parameter should be JSObject unless there's some reason
> it can be null in some cases. It can be more efficient to have the
> more-specific type, so I'd prefer that all the prototype code use JSObject
> rather than JSValue.

I used JSValue because in one place (for the JSLocation object) we still use jsNull() as the prototype (thought this is a bug) and because the prototype ends up being converted to a JSValue when it is stored in _proto.  (Also, in a complete cop-out, Geoff told me too :) ).

> +JSEventTargetNode::JSEventTargetNode(KJS::JSValue* prototype, Node* n)
>
> You should not need an explicit KJS prefix here. Please leave them off except
> in header files or in places where they are necessary.

Fixed.

> For JSHTMLAudioElementConstructor you passed the ExecState and set up the
> prototype inside the constructor, but for JSNamedNodesCollection, JSRGBColor,
> and Navigator you passed the prototype in instead. I'm not sure why you did
> them differently, since in all cases it's the object prototype.

I only changed the objects that represent real wrapped objects to take a prototype instead of an execState.   The objects excluded were Constructors, like JSHTMLAudioElementConstructor and prototypes themselves.  I tried to be consistent in this manner.

> +JSObject* XSLTProcessorConstructorImp::construct(ExecState* exec, const List&
> args)
>
> Elsewhere we call these functions "create", I think largely because we don't
> want to confuse them with constructors. Would you consider calling it create
> instead of construct?
>
> Could the return type be JSXSLTProcessor* instead of JSObject*? In general
> types should always be as specific as possible unless there's a problem.

This is actually an implementation of the [[Construct]] internal property and is an implementation of the method defined in JSObject.  This cannot be changed.

> We really need to take these "Imp" suffixes off things! Can you add a lot more
> of them to do-webcore-rename?

Sure!

> +        DOMObject(JSValue* prototype)
>
> This needs to be marked explicit because we don't want it to be invoked
> implicitly as a conversion (even though that seems unlikely).

Fixed.

> +    private:
> +        DOMObject();
>
> What is this for? Does someone need to construct a DOMObject this way? Can you
> instead just omit this constructor?

I added this to try and make it impossible to create a derived class that didn't pass a prototype.  Will this not work as intended?

> If JSLocation's prototype is going to be null, then why change JSLocation so
> that you can pass the prototype in? You should have just left its constructor
> alone, I think.

I changed it to be uniform among the constructors and because I want to fix the bug shortly.

I don't think it is worth posting a new patch with just the 2 changes mentioned above, but I will wait for your response before going forward.  Thanks for the timely review!

[Geoffrey Garen]

> I used JSValue because in one place (for the JSLocation object) we still use
> jsNull() as the prototype (thought this is a bug) and because the prototype
> ends up being converted to a JSValue when it is stored in _proto.  (Also, in a
> complete cop-out, Geoff told me too :) ).

I agree with Darin: JSObject* is better.

As for JSLocation, we should just give it a real prototype. I think that will require a little security work, though. So, in the short term, you could just make JSLocation ignore the passed-in prototype and use jsNull() instead (with a comment).

[Sam Weinig]

Landed in r29134.