WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Eriq Augustine
Comment 1
2012-02-28 17:54:33 PST
Created
attachment 129365
[details]
Patch
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
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 4
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
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
Created
attachment 129796
[details]
Patch
Gustavo Noronha (kov)
Comment 8
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
Gyuyoung Kim
Comment 9
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
Build Bot
Comment 10
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
Eriq Augustine
Comment 11
2012-03-06 16:21:51 PST
Created
attachment 130463
[details]
Patch
Build Bot
Comment 12
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
Eriq Augustine
Comment 13
2012-03-06 16:49:30 PST
Created
attachment 130473
[details]
Patch
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
Created
attachment 132067
[details]
Patch
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
Created
attachment 132400
[details]
Patch
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
Created
attachment 133928
[details]
Patch
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
Created
attachment 136572
[details]
Patch
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
Created
attachment 136682
[details]
Patch
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
Created
attachment 136721
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug