[webkit-reviews] review denied: [Bug 123380] JSExport doesn't support constructors : [Attachment 215235] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 123380: JSExport doesn't support constructors
https://bugs.webkit.org/show_bug.cgi?id=123380

Attachment 215235: Patch
https://bugs.webkit.org/attachment.cgi?id=215235&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215235&action=review


> Source/JavaScriptCore/API/JSWrapperMap.mm:167
> +	   // Don't copy over init* methods because we handle those specially 

Let's say "init family" instead of "init*", since there's a term of art for it.


> Source/JavaScriptCore/API/JSWrapperMap.mm:341
> +    __block HashMap<String, Protocol *> initTable;

This is a dangerous use of __block with C++. If any of our blocks are copied,
they will copy initTable, instead of sharing it:

-----
<https://developer.apple.com/library/ios/documentation/cocoa/conceptual/Blocks/
Articles/bxVariables.html>

C++ Objects
In general you can use C++ objects within a block. Within a member function,
references to member variables and functions are via an implicitly imported
this pointer and thus appear mutable. There are two considerations that apply
if a block is copied:

If you have a __block storage class for what would have been a stack-based C++
object, then the usual copy constructor is used.
If you use any other C++ stack-based object from within a block, it must have a
const copy constructor. The C++ object is then copied using that constructor.
-----

I guess this usage ends up succeeding because
forEachProtocolImplementingProtocol doesn't copy the passed-in block. I wonder
what the best practice is in a case like this.

> Source/JavaScriptCore/API/JSWrapperMap.mm:344
> +    forEachProtocolImplementingProtocol(cls, exportProtocol, ^(Protocol
*protocol){
> +	   forEachMethodInProtocol(protocol, YES, YES, ^(SEL selector, const
char*){

Need spaces before "{".

> Source/JavaScriptCore/API/JSWrapperMap.mm:377
> +	       // FIXME: Do we need to log here?
> +	       NSLog(@"Too many inits found, abandoning ship.");

Let's log, but try to write something that will be meaningful to an app
developer: "ERROR: Class @{class} exported more than one init family method
through JSExport. Class @{class} will not have a callable constructor
function."

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:589
>      [m_invocation invoke];

This is a bug in the init case because it doesn't account for init releasing
the allocated object and returning a different object in its place (or nil).
You should fix this and add a test for it. c.f.
<https://developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-Transitioni
ngToARC/Introduction/Introduction.html>.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:591
> +    // After calling init we need to autorelease.

This comment could say more: "Balance our call to -alloc with a call to
-autorelease. We have to do this after calling -init because init family
methods are allowed to release the allocated object and return something else
in its place."