[webkit-reviews] review denied: [Bug 79851] Return value from executed script in Chromium. : [Attachment 132067] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Eriq Augustine
<eaugusti at chromium.org>'s request for review:
Bug 79851: Return value from executed script in Chromium.
https://bugs.webkit.org/show_bug.cgi?id=79851

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132067&action=review


This patch uses a bunch of risky memory management patterns.  Using OwnPtr and
PassOwnPtr is clearly a win inside WebKit.  It's less clear to me what the best
way of returning v8 handles across the WebKit API is.  Do we do this in other
places that might establish a pattern that we should follow?

> Source/WebCore/bindings/v8/ScriptController.cpp:166
> +    WTF::Vector<v8::Handle<v8::Value> >* v8Results =
m_proxy->evaluateInIsolatedWorld(worldID, sources, 0);

You shouldn't need to use the WTF namespace here.

> Source/WebCore/bindings/v8/ScriptController.cpp:167
> +    WTF::Vector<ScriptValue>* results = new WTF::Vector<ScriptValue>();

We should never call "new" without immediately calling adoptPtr or adoptRef. 
In this case, we should call adoptPtr.

> Source/WebCore/bindings/v8/ScriptController.cpp:171
> +	 for (itr = v8Results->begin(); itr != v8Results->end(); ++itr)
> +	   results->append(ScriptValue(*itr));

This code is incorrectly indented.

> Source/WebCore/bindings/v8/ScriptController.cpp:173
> +    delete v8Results;

We should never be calling delete manually.  Do we need to use an OwnPtr and
PassOwnPtr to manage the lifetime of this object?

> Source/WebCore/bindings/v8/V8Proxy.cpp:266
> +	
evaluationResults->append(v8::Persistent<v8::Value>::New(evaluate(sources[i],
0)));

Creating a new v8::Persistent value is dangerous because it can easily lead to
memory leaks.  Perhaps the caller should be responsible for creating a
v8::HandleScope?  It's not clear to me what the best way of returning a value
is without risking memory leaks.