Fix invalid casts from ComplexFeature to SimpleFeature.

Fix invalid casts from ComplexFeature to SimpleFeature.

UBSan and CFI made the following invalid cast:
../../tools/json_schema_compiler/test/features_generation_unittest.cc:102:12: runtime error: downcast of address 0x1f55b1c64040 which does not point to an object of type 'APIFeature' (aka 'extensions::SimpleFeat
ure')
0x1f55b1c64040: note: object is of type 'extensions::ComplexFeature'

It is currently impossible to catch regressions like these with the CQ, but there is a work on getting a UBSan Vptr trybot there.

BUG=612594, 670404
Committed: https://crrev.com/f37d4947c6fd5ccb4189f2e24a6f0031ee37676d
Cr-Commit-Position: refs/heads/master@{#435713}

[krasin1, 2016-12-01 18:44:11]

krasin@chromium.org changed reviewers:
+ tbarzic@chromium.org

[tbarzic, 2016-12-01 19:02:29]

+rdevlin.cronin, who has OWNER powers for this.

I think the real issue here is that APIFeature overrides SimpleFeature, even
though there are complex API features  (e.g. the ones used in web UI)

https://codereview.chromium.org/2542163002/diff/1/tools/json_schema_compiler/...
File tools/json_schema_compiler/test/features_generation_unittest.cc (right):

https://codereview.chromium.org/2542163002/diff/1/tools/json_schema_compiler/...
tools/json_schema_compiler/test/features_generation_unittest.cc:99:
fprintf(stderr, "GetAPIFeature(\"%s\")\n", name.c_str());
remove :)

https://codereview.chromium.org/2542163002/diff/1/tools/json_schema_compiler/...
tools/json_schema_compiler/test/features_generation_unittest.cc:292:
static_cast<ComplexFeature*>(provider.GetFeature("complex_alias"));
I don't think cast is needed if provider.GetFeature is used.

[tbarzic, 2016-12-01 19:03:06]

tbarzic@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[tbarzic, 2016-12-01 19:03:06]

adding rdevlin for real

[krasin1, 2016-12-01 19:06:23]

>I think the real issue here is that APIFeature overrides SimpleFeature, even
though there are complex API features

could be. I have used the code pattern that already exists in the test, but that
does not make your statement less true.
I would appreciate if the code owners give a hint of what needs to be fixed, as
there are at least three bots are broken now due to this:

https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr Linux
https://build.chromium.org/p/chromium.fyi/builders/CFI Linux Full
https://build.chromium.org/p/chromium.fyi/builders/CFI Linux ToT

[Devlin, 2016-12-01 19:34:14]

On 2016/12/01 19:06:23, krasin1 wrote:
> >I think the real issue here is that APIFeature overrides SimpleFeature, even
> though there are complex API features
> 
> could be. I have used the code pattern that already exists in the test, but
that
> does not make your statement less true.
> I would appreciate if the code owners give a hint of what needs to be fixed,
as
> there are at least three bots are broken now due to this:
> 
> https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr Linux
> https://build.chromium.org/p/chromium.fyi/builders/CFI Linux Full
> https://build.chromium.org/p/chromium.fyi/builders/CFI Linux ToT

APIFeature overriding SimpleFeature shouldn't be a problem since in non-test
code, everything is returned as a Feature* and not cast in any direction.  The
problem here is that the tests incorrectly cast a complex feature to an API
feature, even though GetAPIFeature should only be used for things that we know
are of type APIFeature.  Naming confusion aside, there shouldn't be a problem
beyond fixing the bad casts in the test (which this cl does).

LGTM.

[krasin1, 2016-12-01 19:36:48]

On 2016/12/01 19:34:14, Devlin wrote:
> On 2016/12/01 19:06:23, krasin1 wrote:
> > >I think the real issue here is that APIFeature overrides SimpleFeature,
even
> > though there are complex API features
> > 
> > could be. I have used the code pattern that already exists in the test, but
> that
> > does not make your statement less true.
> > I would appreciate if the code owners give a hint of what needs to be fixed,
> as
> > there are at least three bots are broken now due to this:
> > 
> > https://build.chromium.org/p/chromium.fyi/builders/UBSanVptr Linux
> > https://build.chromium.org/p/chromium.fyi/builders/CFI Linux Full
> > https://build.chromium.org/p/chromium.fyi/builders/CFI Linux ToT
> 
> APIFeature overriding SimpleFeature shouldn't be a problem since in non-test
> code, everything is returned as a Feature* and not cast in any direction.  The
> problem here is that the tests incorrectly cast a complex feature to an API
> feature, even though GetAPIFeature should only be used for things that we know
> are of type APIFeature.  Naming confusion aside, there shouldn't be a problem
> beyond fixing the bad casts in the test (which this cl does).
> 
> LGTM.

Thank you for the clarification!

[krasin1, 2016-12-01 19:36:54]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2016-12-01 19:37:14]

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

[krasin1, 2016-12-01 19:43:37]

Description was changed from

==========
Fix invalid casts from ComplexFeature to SimpleFeature.

UBSan and CFI made the following invalid cast:
../../tools/json_schema_compiler/test/features_generation_unittest.cc:102:12:
runtime error: downcast of address 0x1f55b1c64040 which does not point to an
object of type 'APIFeature' (aka 'extensions::SimpleFeat
ure')
0x1f55b1c64040: note: object is of type 'extensions::ComplexFeature'

It is currently impossible to catch regressions like these with the CQ, but
there is a work on getting a UBSan Vptr trybot there.

BUG=612594
==========

to

==========
Fix invalid casts from ComplexFeature to SimpleFeature.

UBSan and CFI made the following invalid cast:
../../tools/json_schema_compiler/test/features_generation_unittest.cc:102:12:
runtime error: downcast of address 0x1f55b1c64040 which does not point to an
object of type 'APIFeature' (aka 'extensions::SimpleFeat
ure')
0x1f55b1c64040: note: object is of type 'extensions::ComplexFeature'

It is currently impossible to catch regressions like these with the CQ, but
there is a work on getting a UBSan Vptr trybot there.

BUG=612594,670404
==========

[Nico, 2016-12-01 19:44:25]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2016-12-01 19:45:04]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-12-01 19:45:05]

(i unchecked cq since this still has the debug printf in it that tbarzic
commented on; not sure if you saw his comments. if the plan was to land and then
address comments in a follow-up, please re-check cq.)

[krasin1, 2016-12-01 20:41:13]

https://codereview.chromium.org/2542163002/diff/1/tools/json_schema_compiler/...
File tools/json_schema_compiler/test/features_generation_unittest.cc (right):

https://codereview.chromium.org/2542163002/diff/1/tools/json_schema_compiler/...
tools/json_schema_compiler/test/features_generation_unittest.cc:99:
fprintf(stderr, "GetAPIFeature(\"%s\")\n", name.c_str());
On 2016/12/01 19:02:29, tbarzic wrote:
> remove :)

Done.

https://codereview.chromium.org/2542163002/diff/1/tools/json_schema_compiler/...
tools/json_schema_compiler/test/features_generation_unittest.cc:292:
static_cast<ComplexFeature*>(provider.GetFeature("complex_alias"));
On 2016/12/01 19:02:29, tbarzic wrote:
> I don't think cast is needed if provider.GetFeature is used.

Done.

[krasin1, 2016-12-01 20:41:43]

On 2016/12/01 19:45:05, Nico wrote:
> (i unchecked cq since this still has the debug printf in it that tbarzic
> commented on; not sure if you saw his comments. if the plan was to land and
then
> address comments in a follow-up, please re-check cq.)

Thank you, Nico! Somehow I missed the debug printf and tbarzic's comments about
this.

[krasin1, 2016-12-01 20:41:48]

The CQ bit was checked by krasin@chromium.org

[krasin1, 2016-12-01 20:41:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2542163002/#ps40001
(title: "Remove unnecessary casts")

[commit-bot: I haz the power, 2016-12-01 20:42:09]

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-01 21:15:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480624908149260,
"parent_rev": "d1f0a32d7218f5a2ddc5a6db16e3e46a982a4989", "commit_rev":
"f5895ea0a97d45be1761cc33f7bb872a58a5f472"}

[commit-bot: I haz the power, 2016-12-01 21:15:57]

Description was changed from

==========
Fix invalid casts from ComplexFeature to SimpleFeature.

UBSan and CFI made the following invalid cast:
../../tools/json_schema_compiler/test/features_generation_unittest.cc:102:12:
runtime error: downcast of address 0x1f55b1c64040 which does not point to an
object of type 'APIFeature' (aka 'extensions::SimpleFeat
ure')
0x1f55b1c64040: note: object is of type 'extensions::ComplexFeature'

It is currently impossible to catch regressions like these with the CQ, but
there is a work on getting a UBSan Vptr trybot there.

BUG=612594,670404
==========

to

==========
Fix invalid casts from ComplexFeature to SimpleFeature.

UBSan and CFI made the following invalid cast:
../../tools/json_schema_compiler/test/features_generation_unittest.cc:102:12:
runtime error: downcast of address 0x1f55b1c64040 which does not point to an
object of type 'APIFeature' (aka 'extensions::SimpleFeat
ure')
0x1f55b1c64040: note: object is of type 'extensions::ComplexFeature'

It is currently impossible to catch regressions like these with the CQ, but
there is a work on getting a UBSan Vptr trybot there.

BUG=612594,670404
==========

[commit-bot: I haz the power, 2016-12-01 21:15:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-01 21:18:08]

Description was changed from

==========
Fix invalid casts from ComplexFeature to SimpleFeature.

UBSan and CFI made the following invalid cast:
../../tools/json_schema_compiler/test/features_generation_unittest.cc:102:12:
runtime error: downcast of address 0x1f55b1c64040 which does not point to an
object of type 'APIFeature' (aka 'extensions::SimpleFeat
ure')
0x1f55b1c64040: note: object is of type 'extensions::ComplexFeature'

It is currently impossible to catch regressions like these with the CQ, but
there is a work on getting a UBSan Vptr trybot there.

BUG=612594,670404
==========

to

==========
Fix invalid casts from ComplexFeature to SimpleFeature.

UBSan and CFI made the following invalid cast:
../../tools/json_schema_compiler/test/features_generation_unittest.cc:102:12:
runtime error: downcast of address 0x1f55b1c64040 which does not point to an
object of type 'APIFeature' (aka 'extensions::SimpleFeat
ure')
0x1f55b1c64040: note: object is of type 'extensions::ComplexFeature'

It is currently impossible to catch regressions like these with the CQ, but
there is a work on getting a UBSan Vptr trybot there.

BUG=612594,670404

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

[commit-bot: I haz the power, 2016-12-01 21:18:10]

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