Bug 79851 - Return value from executed script in Chromium.
Summary: Return value from executed script in Chromium.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-28 17:39 PST by Eriq Augustine
Modified: 2012-04-23 18:10 PDT (History)
13 users (show)

See Also:


Attachments
Patch (29.28 KB, patch)
2012-02-28 17:54 PST, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (29.41 KB, patch)
2012-03-01 19:48 PST, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (29.31 KB, patch)
2012-03-06 16:21 PST, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (29.32 KB, patch)
2012-03-06 16:49 PST, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (29.29 KB, patch)
2012-03-15 10:12 PDT, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (28.89 KB, patch)
2012-03-16 15:33 PDT, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (28.96 KB, patch)
2012-03-26 16:46 PDT, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (23.43 KB, patch)
2012-04-10 16:46 PDT, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (23.43 KB, patch)
2012-04-11 09:48 PDT, Eriq Augustine
no flags Details | Formatted Diff | Diff
Patch (29.04 KB, patch)
2012-04-11 12:09 PDT, Eriq Augustine
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eriq Augustine 2012-02-28 17:39:32 PST
Return value from executed script in Chromium.
Comment 1 Eriq Augustine 2012-02-28 17:54:33 PST
Created attachment 129365 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 Gyuyoung Kim 2012-02-28 18:20:44 PST
Comment on attachment 129365 [details]
Patch

Attachment 129365 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11695043
Comment 4 Collabora GTK+ EWS bot 2012-02-29 09:00:33 PST
Comment on attachment 129365 [details]
Patch

Attachment 129365 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11686353
Comment 5 Adam Barth 2012-02-29 10:30:13 PST
Comment on attachment 129365 [details]
Patch

Looks like you've got some compilation trouble.
Comment 6 Kent Tamura 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/).
Comment 7 Eriq Augustine 2012-03-01 19:48:15 PST
Created attachment 129796 [details]
Patch
Comment 8 Gustavo Noronha (kov) 2012-03-01 19:52:10 PST
Comment on attachment 129796 [details]
Patch

Attachment 129796 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11769722
Comment 9 Gyuyoung Kim 2012-03-01 19:53:08 PST
Comment on attachment 129796 [details]
Patch

Attachment 129796 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11767730
Comment 10 Build Bot 2012-03-01 19:58:22 PST
Comment on attachment 129796 [details]
Patch

Attachment 129796 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11766735
Comment 11 Eriq Augustine 2012-03-06 16:21:51 PST
Created attachment 130463 [details]
Patch
Comment 12 Build Bot 2012-03-06 16:32:06 PST
Comment on attachment 130463 [details]
Patch

Attachment 130463 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11838596
Comment 13 Eriq Augustine 2012-03-06 16:49:30 PST
Created attachment 130473 [details]
Patch
Comment 14 WebKit Review Bot 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
Comment 15 Eriq Augustine 2012-03-15 10:12:21 PDT
Created attachment 132067 [details]
Patch
Comment 16 Adam Barth 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.
Comment 17 Adam Barth 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.
Comment 18 Eriq Augustine 2012-03-16 15:33:54 PDT
Created attachment 132400 [details]
Patch
Comment 19 Adam Barth 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
Comment 20 Adam Barth 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.
Comment 21 Eriq Augustine 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.
Comment 22 Eriq Augustine 2012-03-26 16:46:17 PDT
Created attachment 133928 [details]
Patch
Comment 23 WebKit Review Bot 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.
Comment 24 Adam Barth 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.
Comment 25 Eriq Augustine 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.
Comment 26 Adam Barth 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?
Comment 27 Eriq Augustine 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.
Comment 28 Eriq Augustine 2012-04-10 16:46:28 PDT
Created attachment 136572 [details]
Patch
Comment 29 WebKit Review Bot 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
Comment 30 Eriq Augustine 2012-04-11 09:48:00 PDT
Created attachment 136682 [details]
Patch
Comment 31 WebKit Review Bot 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
Comment 32 Eriq Augustine 2012-04-11 12:09:40 PDT
Created attachment 136721 [details]
Patch
Comment 33 Adam Barth 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.
Comment 34 Eriq Augustine 2012-04-13 13:42:12 PDT
Yes I did.
Comment 35 WebKit Review Bot 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>
Comment 36 WebKit Review Bot 2012-04-23 18:10:50 PDT
All reviewed patches have been landed.  Closing bug.