Bug 90694

Summary: [BlackBerry] Add an API to explicitly call a JavaScript function with args.
Product: WebKit Reporter: Benjamin Meyer <ben>
Component: WebKit BlackBerryAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, mifenton, tonikitoo, webkit.review.bot, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
patch
webkit.review.bot: commit-queue-
updated with corrections from the review
yong.li.webkit: review+
updated with corrections from the review
yong.li.webkit: review+, webkit.review.bot: commit-queue-
Add reviewed by line to changelog and commit msg none

Description Benjamin Meyer 2012-07-06 10:10:49 PDT
Created attachment 151098 [details]
patch

Add an API to explicitly call a JavaScript function with args.
    
Currently the Blackberry port doesn't expose the JavaScript
engine to 3rd parties so they rely upon executeJavaScript
which can be slower than necessary and unsafe as eval is used.
This new API provides a way to explicitly call a specific
JavaScript function with a list of args preventing the case
where an argument comes from a untrusted source and tries to
escape the arg list to take control of the JavaScript engine.
    
In the future if the Blackberry port introduces a formal
way to interact with the JavaScript engine this function should
be removed.
Comment 1 WebKit Review Bot 2012-07-06 10:14:13 PDT
Attachment 151098 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/WebPage.cpp',..." exit_code: 1
Source/WebKit/blackberry/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit/blackberry/Api/WebPage.cpp:863:  Declaration has space between type name and * in JSStringRef *argList  [whitespace/declaration] [3]
Source/WebKit/blackberry/Api/WebPage.cpp:864:  Declaration has space between type name and * in JSValueRef *argListRef  [whitespace/declaration] [3]
Source/WebKit/blackberry/Api/WebPage.cpp:873:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/blackberry/Api/WebPage.cpp:878:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/blackberry/Api/WebPage.cpp:892:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/blackberry/Api/WebPage.cpp:933:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/blackberry/Api/WebPage.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Yong Li 2012-07-06 10:21:38 PDT
Comment on attachment 151098 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151098&action=review

> Source/WebKit/blackberry/Api/WebPage.cpp:951
> +        size_t bufferSize = JSStringGetMaximumUTF8CStringSize(stringRef);
> +        char* buffer = new char[bufferSize];
> +        size_t s2 = JSStringGetUTF8CString(stringRef, buffer, bufferSize);
> +        returnValue = WebString(buffer, s2);
> +        delete buffer;

should be delete[]. actually we should use Vector<char>. Also, WebString is supposed to be either UTF16 or latin1. probably we should use WebString::fromUtf8()? or consider using std::string?
Comment 3 WebKit Review Bot 2012-07-06 10:21:41 PDT
Comment on attachment 151098 [details]
patch

Attachment 151098 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13153358
Comment 4 Benjamin Meyer 2012-07-06 11:11:06 PDT
Created attachment 151105 [details]
updated with corrections from the review
Comment 5 WebKit Review Bot 2012-07-06 11:17:00 PDT
Attachment 151105 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/blackberry/Api/WebPage.cpp',..." exit_code: 1
Source/WebKit/blackberry/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Yong Li 2012-07-06 11:20:59 PDT
Comment on attachment 151105 [details]
updated with corrections from the review

View in context: https://bugs.webkit.org/attachment.cgi?id=151105&action=review

LGTM

> Source/WebKit/blackberry/Api/WebPage.cpp:897
> +    JSObjectRef fn = obj;

minor: I prefer long names to short ones.

> Source/WebKit/blackberry/ChangeLog:3
> +
> +        Add an API to explicitly call a JavaScript function with args.

should we also put webkit bug number here?
Comment 7 Benjamin Meyer 2012-07-06 12:08:47 PDT
Created attachment 151108 [details]
updated with corrections from the review
Comment 8 WebKit Review Bot 2012-07-06 12:42:14 PDT
Comment on attachment 151108 [details]
updated with corrections from the review

Rejecting attachment 151108 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/blackberry/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13143555
Comment 9 Benjamin Meyer 2012-07-06 12:46:21 PDT
Created attachment 151111 [details]
Add reviewed by line to changelog and commit msg
Comment 10 WebKit Review Bot 2012-07-06 13:43:37 PDT
Comment on attachment 151111 [details]
Add reviewed by line to changelog and commit msg

Clearing flags on attachment: 151111

Committed r122034: <http://trac.webkit.org/changeset/122034>
Comment 11 WebKit Review Bot 2012-07-06 13:43:42 PDT
All reviewed patches have been landed.  Closing bug.