New issue
Advanced search Search tips

Issue 829913 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Native Extension Bindings: Validate return result in debug builds

Project Member Reported by rdevlin....@chromium.org, Apr 6 2018

Issue description

The JS bindings validate the return result from an API call to ensure it matches the documented schema. [1]  This is a nice feature to have, and should be ported to the native bindings implementation.

[1] https://chromium.googlesource.com/chromium/src/+/fa0473b133768b9345178303d198a2012aa8c83b/extensions/renderer/resources/send_request.js#58
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e6da60e36f6f43d439ac59c1e9001a1648a96e2c

commit e6da60e36f6f43d439ac59c1e9001a1648a96e2c
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Apr 26 00:59:44 2018

[Extensions Bindings] DCHECK on invalid responses

Add native validation for bindings when passing the response back to
the caller that the response fits the values specified in the schema.
Do this by adding the expected callback signature to the type
reference map, and then matching the returned values against that
signature.

Given the performance cost, we don't want to check these values by
default. Instead, only do so if response validation is enabled. Outside
of testing, this is currently only true for debug builds; this matches
current behavior for JS bindings as well. However, this is easily
tweakable by a boolean, which could be linked to a finch feature if we
so desired.

Additionally, tweak matching against expected signatures for responses
to be stricter than signature matching for callers. This is because we
want to ensure that extensions are invoked with the appropriate
arguments at the appropriate indices, rather than needing them to do
our own complicated signature matching. This is a behavior change (and
and improvement) over current JS response validation, since it will
require that every response is truly valid, rather than being able to
be mutated into something valid.

Add unittests for all affected systems, including response validation,
signature matching, and callback signature parsing.

Bug:  829913 

Change-Id: Ib630b3bee935f54e252f66bba7bccf14724adfa9
Reviewed-on: https://chromium-review.googlesource.com/1011407
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553859}
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/BUILD.gn
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_binding.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_binding_unittest.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_binding_util.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_binding_util.h
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_bindings_system.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_request_handler.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_request_handler.h
[add] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_response_validator.cc
[add] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_response_validator.h
[add] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_response_validator_unittest.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_signature.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_signature.h
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_signature_unittest.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_type_reference_map.cc
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/bindings/api_type_reference_map.h
[modify] https://crrev.com/e6da60e36f6f43d439ac59c1e9001a1648a96e2c/extensions/renderer/native_extension_bindings_system_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment