[ios] Do not JSON encode messages sent from WebView to native code.

[ios] Do not JSON encode messages sent from WebView to native code.

UIWebView did not have API for JS -> Native code messaging and
communication was implemented using iframes with custom URL
schemes and messages were encoded into URL as JSON.

WKWebView allows sending messages between JS and native code
and there is no need to encode messages into JSON, so they can be
decoded in the native code.

This change should improve the performance and security as decoding
JSON format can be a source of security vulnerabilities.

Since WKWebView sends all numbers as doubles (kCFNumberDoubleType)
clients should extract them as doubles, not as integers.

BUG=579697
Committed: https://crrev.com/cbc1a720e8e6b094f30b58c653fe97b32cd4dcdd
Cr-Commit-Position: refs/heads/master@{#440123}

[Eugene But (OOO till 7-30), 2016-12-16 02:23:12]

Description was changed from

==========
Do not JSON encode messages.

BUG=
==========

to

==========
Do not JSON encode messages sent from WebView to native code.

UIWebView did not have API for JS -> Native code messaging and
communication was implemented using iframes with custom URL
schemes and messages were encoded into URL as JSON.

WKWebView allows sending messages between JS and native code
and there is no need to encode messages into JSON, so they can be
decoded in the native code.

This change should improve the performance and security as decoding
JSON format can be a source of security vulnerabilities. 

Since WKWebView sends all numbers as doubles (kCFNumberDoubleType)
clients should extract them as doubles, not as integers.

BUG=579697
==========

[Eugene But (OOO till 7-30), 2016-12-16 02:23:23]

The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 02:24:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eugene But (OOO till 7-30), 2016-12-16 03:39:59]

eugenebut@chromium.org changed reviewers:
+ kkhorimoto@chromium.org, sdefresne@chromium.org

[Eugene But (OOO till 7-30), 2016-12-16 03:40:00]

I would like to remove unnecessary JSON encoding from JS -> Native messages.
This should improve performance and security but will change all integers to
doubles.

Kurt and Sylvain, could you please review.

CCed people whose features depend on JS -> Native messaging, so you know you
could break your stuff.

[Eugene But (OOO till 7-30), 2016-12-16 03:40:53]

Description was changed from

==========
Do not JSON encode messages sent from WebView to native code.

UIWebView did not have API for JS -> Native code messaging and
communication was implemented using iframes with custom URL
schemes and messages were encoded into URL as JSON.

WKWebView allows sending messages between JS and native code
and there is no need to encode messages into JSON, so they can be
decoded in the native code.

This change should improve the performance and security as decoding
JSON format can be a source of security vulnerabilities. 

Since WKWebView sends all numbers as doubles (kCFNumberDoubleType)
clients should extract them as doubles, not as integers.

BUG=579697
==========

to

==========
[ios] Do not JSON encode messages sent from WebView to native code.

UIWebView did not have API for JS -> Native code messaging and
communication was implemented using iframes with custom URL
schemes and messages were encoded into URL as JSON.

WKWebView allows sending messages between JS and native code
and there is no need to encode messages into JSON, so they can be
decoded in the native code.

This change should improve the performance and security as decoding
JSON format can be a source of security vulnerabilities. 

Since WKWebView sends all numbers as doubles (kCFNumberDoubleType)
clients should extract them as doubles, not as integers.

BUG=579697
==========

[commit-bot: I haz the power, 2016-12-16 04:18:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 04:18:13]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2016-12-16 17:39:24]

I probably won't have time to look at this CL before middle of next week.

[Eugene But (OOO till 7-30), 2016-12-16 17:46:23]

On 2016/12/16 17:39:24, sdefresne wrote:
> I probably won't have time to look at this CL before middle of next week.
Thanks for heads up. This is not urgent at all.

[kkhorimoto, 2016-12-16 22:33:27]

lgtm. Death to UIWebView tech debt!

[Justin Donnelly, 2016-12-16 22:59:37]

On Thu, Dec 15, 2016 at 7:39 PM, <eugenebut@chromium.org> wrote:

> CCed people whose features depend on JS -> Native messaging, so you know
> you
> could break your stuff.
>

Could you add a little more detail on what this means? Like, are there
changes I need to make now? Or I should just be on the lookout for
breakages? Or is it just that if I want to send numeric data in the future
I need to be aware of the int/double thing?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Eugene But (OOO till 7-30), 2016-12-17 00:20:27]

On 2016/12/16 22:59:37, Justin Donnelly wrote:
> On Thu, Dec 15, 2016 at 7:39 PM, <mailto:eugenebut@chromium.org> wrote:
> 
> > CCed people whose features depend on JS -> Native messaging, so you know
> > you
> > could break your stuff.
> >
> 
> Could you add a little more detail on what this means? Like, are there
> changes I need to make now? Or I should just be on the lookout for
> breakages? Or is it just that if I want to send numeric data in the future
> I need to be aware of the int/double thing?
> 
Sorry. There is no need to change anything and I don't think this CL breaks any
features. But if it does, then you know whom to blame :).
As for the future, it's a good point. Yes, you have to be aware that all numbers
will be doubles, not integers.

Thanks!

[sdefresne, 2016-12-21 13:05:38]

Looks like there is a bug with the conversion.

https://codereview.chromium.org/2581883002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2581883002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:2926: keyCode =
static_cast<double>(keyCode);
I think this should instead be:

    keyCode = static_cast<int>(keyCodeAsDouble);

And then I'm surprised that no test caught this error as keyCode is initialized
to web::WebStateObserver::kInvalidFormKeyCode. Can we add a test that fails when
this code is incorrect (or should we instead remove keyCode?).

[Eugene But (OOO till 7-30), 2016-12-21 15:58:46]

Fixed casting. PTAL

https://codereview.chromium.org/2581883002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2581883002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:2926: keyCode =
static_cast<double>(keyCode);
On 2016/12/21 13:05:38, sdefresne ooo till 2nd Jan 17 wrote:
> I think this should instead be:
> 
>     keyCode = static_cast<int>(keyCodeAsDouble);
> 
> And then I'm surprised that no test caught this error as keyCode is
initialized
> to web::WebStateObserver::kInvalidFormKeyCode. Can we add a test that fails
when
> this code is incorrect (or should we instead remove keyCode?).
Good catch. I don't think that casting to |int| will change anything here in
terms of program correctness, so I can't think about test with would catch this
issue.

If I understand casting correctly then assigning double to int does implicit
cast anyways. Am I missing something?


Removing |keyCode| will not be possible without code duplication like this:
if (!message->GetDouble("keyCode", &keyCodeAsDouble) || keyCodeAsDouble < 0) {
  _webStateImpl->OnFormActivityRegistered(formName, fieldName, type, value,
web::WebStateObserver::kInvalidFormKeyCode, inputMissing);
} else {
  _webStateImpl->OnFormActivityRegistered(formName, fieldName, type, value,
static_cast<int>(keyCode), inputMissing);
}
And I'm not sure if that's a better way :)
}

[sdefresne, 2016-12-21 16:02:53]

https://codereview.chromium.org/2581883002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2581883002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:2919: keyCode =
static_cast<int>(keyCode);
The issue I pointed is still there. Should be:

  keyCode = static_cast<int>(keyCodeAsDouble);

[Eugene But (OOO till 7-30), 2016-12-21 16:09:13]

Sorry about that :)

https://codereview.chromium.org/2581883002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2581883002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:2919: keyCode =
static_cast<int>(keyCode);
On 2016/12/21 16:02:53, sdefresne ooo till 2nd Jan 17 wrote:
> The issue I pointed is still there. Should be:
> 
>   keyCode = static_cast<int>(keyCodeAsDouble);

Done.

[sdefresne, 2016-12-21 16:15:38]

lgtm

I am okay if the tests are added in a later CL, but please create a bug to track
adding them so that we do not forget while relaxing during the holiday season
:-)

[Eugene But (OOO till 7-30), 2016-12-21 16:20:48]

Thanks. File crbug.com/676340

[Eugene But (OOO till 7-30), 2016-12-21 16:20:53]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2016-12-21 16:20:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2581883002/#ps40001
(title: "Actually addressed review comments :)")

[commit-bot: I haz the power, 2016-12-21 16:21:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 16:33:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482337253450890,
"parent_rev": "2eb7d383de685abd61c9c4707c7b7ff8187a02ab", "commit_rev":
"88d0ddfc4d054a7ead051bd5ab18c5fd0124c2a9"}

[commit-bot: I haz the power, 2016-12-21 16:33:42]

Description was changed from

==========
[ios] Do not JSON encode messages sent from WebView to native code.

UIWebView did not have API for JS -> Native code messaging and
communication was implemented using iframes with custom URL
schemes and messages were encoded into URL as JSON.

WKWebView allows sending messages between JS and native code
and there is no need to encode messages into JSON, so they can be
decoded in the native code.

This change should improve the performance and security as decoding
JSON format can be a source of security vulnerabilities. 

Since WKWebView sends all numbers as doubles (kCFNumberDoubleType)
clients should extract them as doubles, not as integers.

BUG=579697
==========

to

==========
[ios] Do not JSON encode messages sent from WebView to native code.

UIWebView did not have API for JS -> Native code messaging and
communication was implemented using iframes with custom URL
schemes and messages were encoded into URL as JSON.

WKWebView allows sending messages between JS and native code
and there is no need to encode messages into JSON, so they can be
decoded in the native code.

This change should improve the performance and security as decoding
JSON format can be a source of security vulnerabilities. 

Since WKWebView sends all numbers as doubles (kCFNumberDoubleType)
clients should extract them as doubles, not as integers.

BUG=579697

Review-Url: https://codereview.chromium.org/2581883002
==========

[commit-bot: I haz the power, 2016-12-21 16:33:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-21 16:37:14]

Description was changed from

==========
[ios] Do not JSON encode messages sent from WebView to native code.

UIWebView did not have API for JS -> Native code messaging and
communication was implemented using iframes with custom URL
schemes and messages were encoded into URL as JSON.

WKWebView allows sending messages between JS and native code
and there is no need to encode messages into JSON, so they can be
decoded in the native code.

This change should improve the performance and security as decoding
JSON format can be a source of security vulnerabilities. 

Since WKWebView sends all numbers as doubles (kCFNumberDoubleType)
clients should extract them as doubles, not as integers.

BUG=579697

Review-Url: https://codereview.chromium.org/2581883002
==========

to

==========
[ios] Do not JSON encode messages sent from WebView to native code.

UIWebView did not have API for JS -> Native code messaging and
communication was implemented using iframes with custom URL
schemes and messages were encoded into URL as JSON.

WKWebView allows sending messages between JS and native code
and there is no need to encode messages into JSON, so they can be
decoded in the native code.

This change should improve the performance and security as decoding
JSON format can be a source of security vulnerabilities.

Since WKWebView sends all numbers as doubles (kCFNumberDoubleType)
clients should extract them as doubles, not as integers.

BUG=579697

Committed: https://crrev.com/cbc1a720e8e6b094f30b58c653fe97b32cd4dcdd
Cr-Commit-Position: refs/heads/master@{#440123}
==========

[commit-bot: I haz the power, 2016-12-21 16:37:15]

Patchset 3 (id:??) landed as
https://crrev.com/cbc1a720e8e6b094f30b58c653fe97b32cd4dcdd
Cr-Commit-Position: refs/heads/master@{#440123}