Bug 79851 – Return value from executed script in Chromium.

• Component: New Bugs
• Status: RESOLVED FIXED

[Eriq Augustine]

Return value from executed script in Chromium.

[Eriq Augustine]

Created attachment 129365 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Gyuyoung Kim]

Comment on attachment 129365 [details]
Patch

Attachment 129365 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11695043

[Collabora GTK+ EWS bot]

Comment on attachment 129365 [details]
Patch

Attachment 129365 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11686353

[Adam Barth]

Comment on attachment 129365 [details]
Patch

Looks like you've got some compilation trouble.

[Kent Tamura]

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]

Created attachment 129796 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 129796 [details]
Patch

Attachment 129796 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11769722

[Gyuyoung Kim]

Comment on attachment 129796 [details]
Patch

Attachment 129796 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11767730

[Build Bot]

Comment on attachment 129796 [details]
Patch

Attachment 129796 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11766735

[Eriq Augustine]

Created attachment 130463 [details]
Patch

[Build Bot]

Comment on attachment 130463 [details]
Patch

Attachment 130463 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11838596

[Eriq Augustine]

Created attachment 130473 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 132067 [details]
Patch

[Adam Barth]

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]

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]

Created attachment 132400 [details]
Patch

[Adam Barth]

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]

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 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]

Created attachment 133928 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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]

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]

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 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]

Created attachment 136572 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 136682 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 136721 [details]
Patch

[Adam Barth]

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]

Yes I did.

[WebKit Review Bot]

Comment on attachment 136721 [details]
Patch

Clearing flags on attachment: 136721

Committed r114974: <http://trac.webkit.org/changeset/114974>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.