RESOLVED FIXED 79851
Return value from executed script in Chromium.
https://bugs.webkit.org/show_bug.cgi?id=79851
Summary Return value from executed script in Chromium.
Eriq Augustine
Reported 2012-02-28 17:39:32 PST
Return value from executed script in Chromium.
Attachments
Patch (29.28 KB, patch)
2012-02-28 17:54 PST, Eriq Augustine
no flags
Patch (29.41 KB, patch)
2012-03-01 19:48 PST, Eriq Augustine
no flags
Patch (29.31 KB, patch)
2012-03-06 16:21 PST, Eriq Augustine
no flags
Patch (29.32 KB, patch)
2012-03-06 16:49 PST, Eriq Augustine
no flags
Patch (29.29 KB, patch)
2012-03-15 10:12 PDT, Eriq Augustine
no flags
Patch (28.89 KB, patch)
2012-03-16 15:33 PDT, Eriq Augustine
no flags
Patch (28.96 KB, patch)
2012-03-26 16:46 PDT, Eriq Augustine
no flags
Patch (23.43 KB, patch)
2012-04-10 16:46 PDT, Eriq Augustine
no flags
Patch (23.43 KB, patch)
2012-04-11 09:48 PDT, Eriq Augustine
no flags
Patch (29.04 KB, patch)
2012-04-11 12:09 PDT, Eriq Augustine
no flags
Eriq Augustine
Comment 1 2012-02-28 17:54:33 PST
WebKit Review Bot
Comment 2 2012-02-28 17:57:45 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Gyuyoung Kim
Comment 3 2012-02-28 18:20:44 PST
Collabora GTK+ EWS bot
Comment 4 2012-02-29 09:00:33 PST
Adam Barth
Comment 5 2012-02-29 10:30:13 PST
Comment on attachment 129365 [details] Patch Looks like you've got some compilation trouble.
Kent Tamura
Comment 6 2012-02-29 21:04:52 PST
Comment on attachment 129365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129365&action=review > LayoutTests/ChangeLog:11 > + * platform/chromium/http/tests/misc/execute-and-return-value-expected.txt: Added. > + * platform/chromium/http/tests/misc/execute-and-return-value.html: Added. If you think layoutTestController.evaluateScriptInIsolatedWorldAndReturnValue() is chromium-specific, we don't need to provide empty implementations of it for other platforms. If not, we should put the test to the common place (not platform/chromium/).
Eriq Augustine
Comment 7 2012-03-01 19:48:15 PST
Gustavo Noronha (kov)
Comment 8 2012-03-01 19:52:10 PST
Gyuyoung Kim
Comment 9 2012-03-01 19:53:08 PST
Build Bot
Comment 10 2012-03-01 19:58:22 PST
Eriq Augustine
Comment 11 2012-03-06 16:21:51 PST
Build Bot
Comment 12 2012-03-06 16:32:06 PST
Eriq Augustine
Comment 13 2012-03-06 16:49:30 PST
WebKit Review Bot
Comment 14 2012-03-06 19:05:49 PST
Comment on attachment 130473 [details] Patch Attachment 130473 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11834746 New failing tests: fast/xmlhttprequest/xmlhttprequest-gc.html
Eriq Augustine
Comment 15 2012-03-15 10:12:21 PDT
Adam Barth
Comment 16 2012-03-15 15:12:23 PDT
Comment on attachment 132067 [details] Patch 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.
Adam Barth
Comment 17 2012-03-15 15:13:05 PDT
Also, consider using out parameters rather than returning heap-allocated vectors. That makes it easier for the function to know whether its caller cares about the return value.
Eriq Augustine
Comment 18 2012-03-16 15:33:54 PDT
Adam Barth
Comment 19 2012-03-19 00:10:58 PDT
Comment on attachment 132400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132400&action=review This is much better than the previous iterations. > Source/WebCore/bindings/v8/V8Proxy.cpp:265 > + if (results) Can we move this test outside of the loop? I guess the compiler will probably do that optimization of us... > Source/WebKit/chromium/src/WebFrameImpl.cpp:906 > + sources.append(ScriptSourceCode( > + sourcesIn[i].code, sourcesIn[i].url, position)); No need to wrap at 80 cols > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1350 > + // TODO: There are many more types that can be handled. TODO => FIXME
Adam Barth
Comment 20 2012-03-19 00:11:53 PDT
Did you want to mark this patch for review? It looks pretty good. If you'd like me to review it, I can study it in more detail.
Eriq Augustine
Comment 21 2012-03-19 10:00:07 PDT
Comment on attachment 132400 [details] Patch Yes, that would be great. I think that the constant re-packaging of the evaluated values is a bit unfortunate, but I can't see a way around it that follows the conventions of the code around it.
Eriq Augustine
Comment 22 2012-03-26 16:46:17 PDT
WebKit Review Bot
Comment 23 2012-03-26 16:49:44 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Adam Barth
Comment 24 2012-04-06 16:38:14 PDT
Comment on attachment 133928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133928&action=review Sorry that it's taken me so long to review this patch. This looks fine. A couple of small suggestions below. I think the main thing is that we can avoid converting back and forth between ScriptValues. If WebCore wants to use this function at some point, we might need to use ScriptValue, but it doesn't seem to be needed right now. > Source/WebCore/ChangeLog:8 > + Providing a varaiant of evaluateScriptInIsolatedWorld that typo: varaiant > Source/WebKit/chromium/public/WebFrame.h:279 > + virtual void executeScriptInIsolatedWorld( > + int worldID, const WebScriptSource* sourcesIn, unsigned numSources, > + int extensionGroup, WebVector<v8::Handle<v8::Value> >* results) = 0; Should we be more specific that these are local handles? > Source/WebKit/chromium/src/WebFrameImpl.cpp:906 > + for (unsigned i = 0; i < numSources; ++i) { > + TextPosition position(OrdinalNumber::fromOneBasedInt(sourcesIn[i].startLine), OrdinalNumber::first()); > + sources.append(ScriptSourceCode(sourcesIn[i].code, sourcesIn[i].url, position)); > + } Is this code copy/pasted from somewhere? Should we share code instead? > Source/WebKit/chromium/src/WebFrameImpl.cpp:909 > + Vector<ScriptValue> scriptResults; Do we really need to round-trip through ScriptValue? Both the caller and the callee of this function always use V8, so it seems like we could just pass the vector directly.
Eriq Augustine
Comment 25 2012-04-09 18:23:37 PDT
Humm... if we pass the Vector of v8::Handle<v8::Value>'s, do we need the Handle to be Persistent? I could be mistaken, but doesn't the result of the eval get placed into a Local which is managed by the HandleScope created in V8Proxy::evaluateInIsolatedWorld(). I think it was fine before when I was using ScriptValue because ScriptValue uses Persistent. Sorry if I am totally off, it's all a bit confusing for a first dive into WebKit.
Adam Barth
Comment 26 2012-04-09 23:39:38 PDT
I'm not 100% sure either. Certainly, if we're returning local handles, we'll need the caller to have their own HandleScope. I'm not sure what happens if you have two HandleScopes on the stack. Are locals bound to the most recent HandleScope or to the last to unwind?
Eriq Augustine
Comment 27 2012-04-10 16:43:29 PDT
Comment on attachment 133928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133928&action=review >> Source/WebCore/ChangeLog:8 >> + Providing a varaiant of evaluateScriptInIsolatedWorld that > > typo: varaiant done. >> Source/WebKit/chromium/public/WebFrame.h:279 >> + int extensionGroup, WebVector<v8::Handle<v8::Value> >* results) = 0; > > Should we be more specific that these are local handles? Yes, I think we should. Doing this should make it clear to the caller that they need their own HandleScope. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:906 >> + } > > Is this code copy/pasted from somewhere? Should we share code instead? It's from WebFrameImpl::executeScriptInIsolatedWorld. Do you think it is worth it? >> Source/WebKit/chromium/src/WebFrameImpl.cpp:909 >> + Vector<ScriptValue> scriptResults; > > Do we really need to round-trip through ScriptValue? Both the caller and the callee of this function always use V8, so it seems like we could just pass the vector directly. Discussed in IRC. ScriptValue bundles the value in a Persistent which helps the hand-off to the caller's HandleScope. This is only an issue because the Vector is passed by reference instead of by value.
Eriq Augustine
Comment 28 2012-04-10 16:46:28 PDT
WebKit Review Bot
Comment 29 2012-04-10 21:34:36 PDT
Comment on attachment 136572 [details] Patch Attachment 136572 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12384609
Eriq Augustine
Comment 30 2012-04-11 09:48:00 PDT
WebKit Review Bot
Comment 31 2012-04-11 10:25:10 PDT
Comment on attachment 136682 [details] Patch Attachment 136682 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12387136
Eriq Augustine
Comment 32 2012-04-11 12:09:40 PDT
Adam Barth
Comment 33 2012-04-12 16:55:34 PDT
Comment on attachment 136721 [details] Patch Did you mean to mark this patch for review? The bubbles are all green. I think we're all set here.
Eriq Augustine
Comment 34 2012-04-13 13:42:12 PDT
Yes I did.
WebKit Review Bot
Comment 35 2012-04-23 18:10:41 PDT
Comment on attachment 136721 [details] Patch Clearing flags on attachment: 136721 Committed r114974: <http://trac.webkit.org/changeset/114974>
WebKit Review Bot
Comment 36 2012-04-23 18:10:50 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.