[Webkit-unassigned] [Bug 79851] Return value from executed script in Chromium.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=79851


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #132067|review?                     |review-
               Flag|                            |




--- Comment #16 from Adam Barth <abarth at webkit.org>  2012-03-15 15:12:23 PST ---
(From update of attachment 132067)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.