New issue
Advanced search Search tips

Issue 845816 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocking:
issue 842698



Sign in to add a comment

webkit_unittests ProtocolParserTest.Reading fails on Android

Project Member Reported by h...@chromium.org, May 23 2018

Issue description

Not in all Android build modes, but some of them.

From https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/283

[ RUN      ] ProtocolParserTest.Reading
../../third_party/blink/renderer/core/inspector/protocol_parser_test.cc:124: Failure
Expected equality of these values:
  Value::TypeDouble
    Which is: 3
  root->type()
    Which is: 2
Stack trace:
../../third_party/blink/renderer/core/inspector/protocol_parser_test.cc:127: Failure
Expected equality of these values:
  2147483648.0
    Which is: 2147483648
  double_val
    Which is: 2147483647
Stack trace:
../../third_party/blink/renderer/core/inspector/protocol_parser_test.cc:130: Failure
Expected equality of these values:
  Value::TypeDouble
    Which is: 3
  root->type()
    Which is: 2
Stack trace:
../../third_party/blink/renderer/core/inspector/protocol_parser_test.cc:133: Failure
Expected equality of these values:
  -2147483649.0
    Which is: -2147483649
  double_val
    Which is: -2147483648
Stack trace:
[  FAILED  ] ProtocolParserTest.Reading (211 ms)


I know what's going on here :-) I'm pretty sure it's the same as https://bugs.chromium.org/p/chromium/issues/detail?id=831145#c48 but we hadn't noticed because this test was excluded on Android.
 

Comment 1 by h...@chromium.org, May 23 2018

Cc: thakis@chromium.org pfeldman@chromium.org
Status: Started (was: Assigned)
Patch: https://chromium-review.googlesource.com/#/c/deps/inspector_protocol/+/1069133
Project Member

Comment 2 by bugdroid1@chromium.org, May 23 2018

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

commit 5d675f772503003ade32a3a0ac32a69c97a2384b
Author: Hans Wennborg <hans@chromium.org>
Date: Wed May 23 10:14:17 2018

Android test exclusions: don't exclude webkit_unit_tests

These tests do pass on some bots, e.g. [1], and it's not clear why they
were being excluded. If there's a legitimate reason for them not to run
on Android, let's figure it out and get a bug filed.

Sheriffs: if this causes the tests to start failing on Android bots,
please file a bug with information about the error, put the exclusion
back and reference the bug.

Disable ProtocolParserTest.Reading on Android until it's fixed,
see the third bug.

(The CrOS failure was fixed a few months ago, see  crbug.com/795645 )

Bug:  842698 ,  795645 ,  845816 

 [1] https://ci.chromium.org/buildbot/chromium.clang/ToTAndroid/3343

R=thakis@chromium.org

Change-Id: I4c683170508d431864880774543f942cf7446b88
Reviewed-on: https://chromium-review.googlesource.com/1068873
Commit-Queue: Hans Wennborg <hans@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561021}
[modify] https://crrev.com/5d675f772503003ade32a3a0ac32a69c97a2384b/testing/buildbot/chromium.android.fyi.json
[modify] https://crrev.com/5d675f772503003ade32a3a0ac32a69c97a2384b/testing/buildbot/chromium.android.json
[modify] https://crrev.com/5d675f772503003ade32a3a0ac32a69c97a2384b/testing/buildbot/chromium.chromiumos.json
[modify] https://crrev.com/5d675f772503003ade32a3a0ac32a69c97a2384b/testing/buildbot/test_suite_exceptions.pyl
[modify] https://crrev.com/5d675f772503003ade32a3a0ac32a69c97a2384b/third_party/blink/renderer/core/inspector/protocol_parser_test.cc

Project Member

Comment 3 by bugdroid1@chromium.org, May 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/deps/inspector_protocol/+/ecc514d490d9d8c339f7cb2a887e47c313a5ada1

commit ecc514d490d9d8c339f7cb2a887e47c313a5ada1
Author: Hans Wennborg <hans@chromium.org>
Date: Wed May 23 10:10:47 2018

Parser.cpp: Fix undefined behaviour in float conversion

The code was trying to check whether a double value could be represented
as an integer by casting it and comparing the result before and after.

However, casting a double value that's out-of-range of the integer is
undefined behaviour, and so in this case Clang on some architectures
would effectively optimize away the range check.

To do this properly, the code has to check that the value will fit in
the integer before casting.

Bug:  chromium:845816 
Change-Id: I57a723b6e40a1366355030590e70fff19a1159eb
[modify] https://crrev.com/ecc514d490d9d8c339f7cb2a887e47c313a5ada1/lib/Parser_cpp.template

Project Member

Comment 4 by bugdroid1@chromium.org, May 25 2018

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

commit 0a850b216acedca65ed657a8c5909ad77b415d6b
Author: Hans Wennborg <hans@chromium.org>
Date: Fri May 25 10:45:13 2018

Roll third_party/inspector_protocol 4d389ba..f58d066

This roll includes:

- f58d066 Add shebangs to pacify chromium presubmit error
- ecc514d Parser.cpp: Fix undefined behaviour in float conversion
- 59ca26e [inspector_protocol] Node.js is finally ready for transition..
- dd90116 [inspector_protocol] node uses script names inside own repository

Bug:  845816 
Change-Id: I3e6ddda9af210e1bfa6350d0999ce2f0ee170a44
Reviewed-on: https://chromium-review.googlesource.com/1070275
Commit-Queue: Hans Wennborg <hans@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561824}
[add] https://crrev.com/0a850b216acedca65ed657a8c5909ad77b415d6b/third_party/inspector_protocol/ConvertProtocolToJSON.py
[modify] https://crrev.com/0a850b216acedca65ed657a8c5909ad77b415d6b/third_party/inspector_protocol/README.chromium
[add] https://crrev.com/0a850b216acedca65ed657a8c5909ad77b415d6b/third_party/inspector_protocol/convert_to_protocol_json.py
[modify] https://crrev.com/0a850b216acedca65ed657a8c5909ad77b415d6b/third_party/inspector_protocol/lib/Parser_cpp.template

Project Member

Comment 5 by bugdroid1@chromium.org, May 25 2018

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

commit 868d1b2df5fb3b88564ef0308d2e64b525553c82
Author: Hans Wennborg <hans@chromium.org>
Date: Fri May 25 17:51:33 2018

Re-enable ProtocolParserTest.Reading

This should be fixed after crrev.com/561824

Bug:  845816 
Change-Id: I1f12642c2ab8bea9f56e1c15d23046074ccbe62e
Reviewed-on: https://chromium-review.googlesource.com/1073368
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561936}
[modify] https://crrev.com/868d1b2df5fb3b88564ef0308d2e64b525553c82/third_party/blink/renderer/core/inspector/protocol_parser_test.cc

Comment 6 by h...@chromium.org, May 28 2018

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, May 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/deps/inspector_protocol/+/7efd53047e3df9993c0c1653bb674f2de9b93053

commit 7efd53047e3df9993c0c1653bb674f2de9b93053
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Mon May 28 11:45:04 2018

IWYU: INT_MAX and INT_MIN need <climits>

Follow-up to ecc514d490 ("Parser.cpp: Fix undefined behaviour in float
conversion"), which broke the libstdc++ build:

    gen/chrome/browser/devtools/protocol/protocol.cc: In function ‘std::unique_ptr<protocol::Value> protocol::{anonymous}::buildValue(const Char*, const Char*, const Char**, int)’:
    gen/chrome/browser/devtools/protocol/protocol.cc:1304:22: error: ‘INT_MIN’ was not declared in this scope
             if (value >= INT_MIN && value <= INT_MAX && static_cast<int>(value) == value)
                          ^~~~~~~
    gen/chrome/browser/devtools/protocol/protocol.cc:1304:22: note: suggested alternative: ‘WINT_MIN’
             if (value >= INT_MIN && value <= INT_MAX && static_cast<int>(value) == value)
                          ^~~~~~~
                          WINT_MIN
    gen/chrome/browser/devtools/protocol/protocol.cc:1304:42: error: ‘INT_MAX’ was not declared in this scope
             if (value >= INT_MIN && value <= INT_MAX && static_cast<int>(value) == value)
                                              ^~~~~~~
    gen/chrome/browser/devtools/protocol/protocol.cc:1304:42: note: suggested alternative: ‘WINT_MAX’
             if (value >= INT_MIN && value <= INT_MAX && static_cast<int>(value) == value)
                                              ^~~~~~~
                                              WINT_MAX

Bug: chromium:819294,  chromium:845816 
Change-Id: Iabcece0e83cf0ab26d6bfda5a63239bfd850a33a
[modify] https://crrev.com/7efd53047e3df9993c0c1653bb674f2de9b93053/lib/Protocol_cpp.template

Comment 8 by thakis@chromium.org, May 29 2018

Blocking: 842698

Sign in to add a comment