Update FlatBuffers to version 1.5.0.

Update FlatBuffers to version 1.5.0.

+ Fix the build issue on andrdoid.

BUG=699958

[pkalinnikov, 2017-03-09 11:38:05]

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

[commit-bot: I haz the power, 2017-03-09 11:38:18]

Dry run: 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, 2017-03-09 11:44:37]

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

[commit-bot: I haz the power, 2017-03-09 11:44:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[pkalinnikov, 2017-03-09 12:51:38]

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

[commit-bot: I haz the power, 2017-03-09 12:51:59]

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

[pkalinnikov, 2017-03-09 12:52:18]

Description was changed from

==========
Update FlatBuffers to version 1.5.0.

BUG=699958
==========

to

==========
Update FlatBuffers to version 1.5.0.

+ Fix the build issue on andrdoid.

BUG=699958
==========

[pkalinnikov, 2017-03-09 13:03:16]

pkalinnikov@chromium.org changed reviewers:
+ engedy@chromium.org

[pkalinnikov, 2017-03-09 13:03:17]

Balazs, can you take a quick look?

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:90: #  - grpc/tests/grpctest.cpp
I suppose this test should be added similarly to "src/tests/test.cpp".

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:92: #  -
tests/fuzzer/flatbuffers_parser_fuzzer.cc
These 2 fuzzer tests are not included into CMakeLists.txt, but have scripts to
run them. Do we need to test it somehow with our infrastructure?

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:93: #  - tests/monster_test.grpc.fb.cc
This one is probably not a test. Will delete it.

[pkalinnikov, 2017-03-09 13:16:25]

pkalinnikov@chromium.org changed reviewers:
+ palmer@chromium.org

[pkalinnikov, 2017-03-09 13:16:25]

palmer@: Hi Chris. This CL updates the FlatBuffers library from somewhere
between 1.3/1.4 to 1.5. Is it necessary to evaluate it for security once again?

[engedy, 2017-03-09 14:10:26]

https://codereview.chromium.org/2740103002/diff/20001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset.h (right):

https://codereview.chromium.org/2740103002/diff/20001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset.h:9: #define
FLATBUFFERS_CPP98_STL
I think it's frowned upon to define macros here, instead we should define it via
GN. Specifically, for all direct dependents of the build target that contains
the problematic include, we need to have this macro defined.

According to [1], you can define a configuration that adds this define, and add
this as the public_configs section of that build target (say `foo`) containing
the problematic header. Something like this:

config("foo_config") {
  defines = [ "FLATBUFFERS_CPP98_STL" ]
}

component("foo") {
  public_configs = [ ":foo_config" ]
}

[1]:
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...

[commit-bot: I haz the power, 2017-03-09 14:14:56]

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

[commit-bot: I haz the power, 2017-03-09 14:14:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2017-03-09 14:53:14]

LGTM % outstanding comments.

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:90: #  - grpc/tests/grpctest.cpp
On 2017/03/09 13:03:17, pkalinnikov wrote:
> I suppose this test should be added similarly to "src/tests/test.cpp".

Right. Note, however, that each of these exports a main() function, so it will
need to go into another test target (it looks like we missing a few targets,
actually).

I'd suggest filing a bug on it, and fixing this in a follow-up CL  so that we
can unblock the dependent CL.

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:92: #  -
tests/fuzzer/flatbuffers_parser_fuzzer.cc
On 2017/03/09 13:03:17, pkalinnikov wrote:
> These 2 fuzzer tests are not included into CMakeLists.txt, but have scripts to
> run them. Do we need to test it somehow with our infrastructure?

I believe this is exercised by ClusterFuzz outside of Chromium continuous build.
Looks like this has file been upstreamed from here:

https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/flatbuffers_ve...

Would also file a bug and figure out what to do about this later.

[pkalinnikov, 2017-03-09 17:36:58]

https://codereview.chromium.org/2740103002/diff/20001/components/subresource_...
File components/subresource_filter/core/common/indexed_ruleset.h (right):

https://codereview.chromium.org/2740103002/diff/20001/components/subresource_...
components/subresource_filter/core/common/indexed_ruleset.h:9: #define
FLATBUFFERS_CPP98_STL
On 2017/03/09 14:10:25, engedy (slow) wrote:
> I think it's frowned upon to define macros here, instead we should define it
via
> GN. Specifically, for all direct dependents of the build target that contains
> the problematic include, we need to have this macro defined.
> 
> According to [1], you can define a configuration that adds this define, and
add
> this as the public_configs section of that build target (say `foo`) containing
> the problematic header. Something like this:
> 
> config("foo_config") {
>   defines = [ "FLATBUFFERS_CPP98_STL" ]
> }
> 
> component("foo") {
>   public_configs = [ ":foo_config" ]
> }
> 
> [1]:
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...

Done.

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:90: #  - grpc/tests/grpctest.cpp
Filed crbug.com/700039 for this and the one below.

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:92: #  -
tests/fuzzer/flatbuffers_parser_fuzzer.cc
On 2017/03/09 14:53:14, engedy (slow) wrote:
> On 2017/03/09 13:03:17, pkalinnikov wrote:
> > These 2 fuzzer tests are not included into CMakeLists.txt, but have scripts
to
> > run them. Do we need to test it somehow with our infrastructure?
> 
> I believe this is exercised by ClusterFuzz outside of Chromium continuous
build.
> Looks like this has file been upstreamed from here:
> 
>
https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/flatbuffers_ve...
> 
> Would also file a bug and figure out what to do about this later.

Done.

https://codereview.chromium.org/2740103002/diff/20001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:93: #  - tests/monster_test.grpc.fb.cc
On 2017/03/09 13:03:17, pkalinnikov wrote:
> This one is probably not a test. Will delete it.

Done.

[pkalinnikov, 2017-03-09 17:37:04]

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

[commit-bot: I haz the power, 2017-03-09 17:37:34]

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

[engedy, 2017-03-09 17:39:32]

https://codereview.chromium.org/2740103002/diff/40001/third_party/flatbuffers...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/2740103002/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:92: # TODO(pkalinnikov): Add
"grpc/tests/grpctest.cpp" as well.
nit: Could you please reference https://crbug.com/NNNNNN here?

[commit-bot: I haz the power, 2017-03-09 17:45:59]

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

[commit-bot: I haz the power, 2017-03-09 17:46:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[pkalinnikov, 2017-03-09 20:54:08]

pkalinnikov@chromium.org changed reviewers:
+ wvo@google.com

[pkalinnikov, 2017-03-09 20:54:09]

wvo@: Hi Wouter. Can you please evaluate feasibility of defining
FLATBUFFERS_CPP98_STL from outside of flatbuffers.h?

Also, with FLATBUFFERS_CPP98_STL defined some of the tests don't compile because
they include the pre-generated "monster_test_generated.h" file. The latter was
apparently generated without the macro defined, which makes the file
incompatible with "flatbuffers.h" included from it. Should this be fixed?

[wvo, 2017-03-10 17:24:21]

FLATBUFFERS_CPP98_STL was a half-assed attempt at STLPort compatibility. It
currently is intended to work only for the minimal usage scenario of
flatbuffers.h + a default generated code file. For our use case at the time
(Android STLPort builds) this was sufficient.

It does not work for compiling the FlatBuffers tests, since the generated code
is compiled with --gen-object-api, which uses std::unique_ptr, std::function and
others. We don't really want to remove the use of these types in that (optional)
API, so the only way to fully support the tests is thus to supply a set of own
implementations of these classes whenever running under FLATBUFFERS_CPP98_STL.
This is something we can do, but I was trying to avoid it.

Another way to fix it would be to define a second test target in CMakeLists.txt
that generates the code from scratch without --gen-object-api, and #ifdef out
the object API tests in test.cpp.

Yes you should be able to define FLATBUFFERS_CPP98_STL externally if your
environment doesn't define _STLPORT_VERSION, but that wouldn't change much.

I can work on either of those solutions, but I'm OOO for another 2 weeks.. if
this is urgent, one of my team members can pick it up. Of course to quickly
un-break this build, disabling building the test exe would work, or reverting to
an older version of FlatBuffers.

[engedy, 2017-04-04 08:53:43]

Pavel, what's the plan here?

[palmer, 2017-04-04 17:45:54]

lgtm

[wvo, 2017-04-05 20:55:40]

I'm back, so let me know if you need me to implement either of those suggested
solutions.

[pkalinnikov, 2017-06-09 06:52:29]

This CL was replaced by https://codereview.chromium.org/2923203002/. Closing.