Add evaluateJavaScript to iOS WebView #8798

thebnich · 2016-07-14T23:26:43Z

Picking this up from #1191. Adds evaluateJavaScript to iOS WebView API, enabling React -> page communication.

Verified test cases:

"123" evaluates to 123.
"'123'" evaluates to the string "123".
"'foo\"bar'" evaluates to "foo"bar".
"'foo\\'bar'" evaluates to "foo'bar".
"'foo\\\\bar'" evaluates to "foo\bar".
"window.location" returns an object with href, host, and other expected properties.
"window" results in an error (cyclic JSON).

ghost · 2016-07-14T23:26:59Z

By analyzing the blame information on this pull request, we identified @caabernathy and @nicklockwood to be potential reviewers.

ghost · 2016-07-14T23:26:59Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

ghost · 2016-07-14T23:40:09Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

thebnich · 2016-07-15T03:01:07Z

CC'ing @spicyj since he reviewed #1191.

sophiebits · 2016-07-15T03:12:02Z

Libraries/Components/WebView/WebView.ios.js

+    return new Promise((resolve, reject) => {
+      var escaped = script.replace(/\\/g, '\\\\')
+                          .replace(/"/g, '\\"');
+      var wrapped = 'JSON.stringify(eval("' + escaped + '"))';


var escaped = JSON.stringify(script).replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029'); var wrapped = 'JSON.stringify(eval(' + escaped + '))';

sophiebits · 2016-07-15T03:12:47Z

I'm not familiar with this code day-to-day so I'm not the best person to review this but it looks mostly reasonable if the escaping is fixed.

thebnich · 2016-07-15T03:42:34Z

@spicyj Thanks for the quick feedback! Updated to use the given escape handling.

satya164 · 2016-07-15T11:44:23Z

Libraries/Components/WebView/WebView.ios.js

+      var escaped = JSON.stringify(script).replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029');
+      var wrapped = 'JSON.stringify(eval(' + escaped + '))';
+
+      RCTWebViewManager.evaluateJavaScript(this.getWebViewHandle(), wrapped, function(error, result) {


Let's implement the promise on the native side instead of in JS. It allows us to pass an error message and error code too.

satya164 · 2016-07-15T17:45:13Z

React/Views/RCTWebViewManager.m

+    if (![view isKindOfClass:[RCTWebView class]]) {
+      RCTLogError(@"Invalid view returned from registry, expecting RCTWebView, got: %@", view);
+    } else {
+      callback(@[[NSNull null], [view evaluateJavaScript: script]]);


RN modules support promise natively, so changing this to use a promise instead of a callback will reduce boilerplate on the JS side.

Yeah, I agree -- looking into this API now. Thanks for the suggestion!

thebnich · 2016-07-15T20:46:45Z

Updated to use the RCTPromise API.

satya164 · 2016-07-15T21:45:15Z

Libraries/Components/WebView/WebView.ios.js

+   */
+  evaluateJavaScript: async function(
+    script: string,
+    callback?: ?(error: ?Error, result: ?string) => void,


We don't need to support both Promise and callback based APIs. React Native is moving to using Promises instead of callbacks. :)

OK -- I was following the convention I saw elsewhere in the codebase (and carried over from #1191) and didn't know whether new APIs should still be both callback- and promise-compatible. If that's the case, I'll drop it. Thanks again for the feedback!

satya164 · 2016-07-15T22:03:51Z

Libraries/Components/WebView/WebView.ios.js

@@ -462,6 +462,16 @@ var WebView = React.createClass({
  },

  /**
+   * Evaluate JavaScript on the current page.
+   */
+  evaluateJavaScript: async function(script: string): Promise {


I think Flow will soon require type parameters (not sure what they are called), so may be we have to change it to Promise<any>

satya164 · 2016-07-15T22:04:50Z

LGTM. cc @nicklockwood

satya164 · 2016-07-16T21:07:52Z

cc @javache

pardoman · 2016-07-17T22:50:47Z

Would be good to have some Unit Test script as part of this PR.

However, after looking through the repo a bit (I'm not too familiar with it), I wasn't able to find iOS unit tests, only some Android Unit Tests.

javache · 2016-07-18T08:57:05Z

Libraries/Components/WebView/WebView.ios.js

+   */
+  evaluateJavaScript: async function(script: string): Promise {
+    var escaped = JSON.stringify(script).replace(/\u2028/g, '\\u2028').replace(/\u2029/g, '\\u2029');
+    var wrapped = 'JSON.stringify(eval(' + escaped + '))';


Why enforce this JSON.stringify(eval( pattern? Why not just send over the original string? If your return value isn't string representable, you should add the JSON.stringify yourself.

Why enforce this JSON.stringify(eval( pattern`? Why not just send over the original string?

#1191 has the relevant history (starting with @spicyj's comment). For forward compatibility with WKWebView (and to be more useful in general), we want the API to return the typed result equivalent to the page-side result.

The naive approach for doing this would be to simply say var wrapped = 'JSON.stringify(' + script + ')', using JSON.parse() on the result. That works for most cases, but breaks if the script contains multiple statements (i.e., 'foo'; 'bar';). eval() fixes this by executing all statements in the script and returning the result of the last statement, which is exactly what we need to feed the result to JSON.stringify(). That means we also need to stringify the script itself so we can use it in eval.

The problem with using eval in this way is that if the page enforces a Content Security policy that restricts the use of eval, the code won't be executed.

ghost · 2016-08-18T17:01:31Z

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @caabernathy as a potential reviewer. Could you take a look please or cc someone with more context?

hramos · 2016-11-10T19:58:50Z

@javache as the last one to comment on this review, is there anything else you'd like to see before this is approved?

lacker · 2016-11-30T23:58:58Z

I think the only thing this needs is some testing. There are iOS unit tests, they are just under the UI explorer, so I would put something there.

worm-emoji · 2016-12-07T18:21:34Z

I think the biggest problem with this implementation is its internal use of eval(). Usually it wouldn't cause issues, but the problem is that on pages with a Content-Security-Policy, injecting javascript this way will not work. The native stringByEvaluatingJavaScriptFromString function effectively bypasses CSP rules, but the use of eval can once again break things..

I definitely recognize the importance of using eval in this way to get a response back, but I think it must be implemented in a way that doesn't break the injection of JS on certain pages. Perhaps we can check if eval() is allowed when the WebView is initialized, or just add another method that is more or less a dumb wrapper for stringByEvaluatingJavaScriptFromString that specifically doesn't return a response.

I wrote some code that implements a function called injectJavaScript that does it in a way where CSP rules will always be ignored. I am considering making a pull request but I'd rather not duplicate the work done here.

Summary: Currently, < WebView > allows you to pass JS to execute within the view. This works great, but there currently is not a way to execute JS after the page is loaded. We needed this for our app. We noticed that the WebView had messaging support added (see #9762) . Initially, this seemed like more than enough functionality for our use case - just write a function that's injected on initial load that accepts a message with JS, and `eval()` it. However, this broke once we realized that Content Security Policy can block the use of eval on pages. The native methods iOS provide to inject JS allow you to inject JS without CSP interfering. So, we just wrapped the native methods on iOS (and later Android) and it worked for our use case. The method injectJavaScript was born. Now, after I wrote this code, I realized that #8798 exists and hadn't been merged because of a lack of tests. I commend what was done in #8798 as it sorely solves a problem (injecting JS after the initial load) and has more features than what I' Closes #11358 Differential Revision: D4390425 fbshipit-source-id: 02813127f8cf60fd84229cb26eeea7f8922d03b3

Summary: Currently, < WebView > allows you to pass JS to execute within the view. This works great, but there currently is not a way to execute JS after the page is loaded. We needed this for our app. We noticed that the WebView had messaging support added (see facebook#9762) . Initially, this seemed like more than enough functionality for our use case - just write a function that's injected on initial load that accepts a message with JS, and `eval()` it. However, this broke once we realized that Content Security Policy can block the use of eval on pages. The native methods iOS provide to inject JS allow you to inject JS without CSP interfering. So, we just wrapped the native methods on iOS (and later Android) and it worked for our use case. The method injectJavaScript was born. Now, after I wrote this code, I realized that facebook#8798 exists and hadn't been merged because of a lack of tests. I commend what was done in facebook#8798 as it sorely solves a problem (injecting JS after the initial load) and has more features than what I' Closes facebook#11358 Differential Revision: D4390425 fbshipit-source-id: 02813127f8cf60fd84229cb26eeea7f8922d03b3

lacker · 2017-02-08T21:35:42Z

@thebnich Do you have thoughts on the status or are you still working on this?

thebnich · 2017-02-09T01:18:55Z

No, I am not actively working on this. I needed this functionality for a prototype I was creating last year, but haven't touched it since. I'm not familiar with creating/running tests since that was the only time I've used React Native, and I don't have time to figure that out now, so I'll go ahead and close this PR.

…8798

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 14, 2016