1413111 - layout/style/nsCSSRuleProcessor.cpp:1689:16: error: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Werror=strict-overflow]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Hommey [:glandium] OoO until Jun 18]

I got this error on a PGO build on try with unrelated changes (I've only been touching mozjemalloc recently):

[task 2017-10-31T07:45:56.635Z] 07:45:56     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/layout/style/Unified_cpp_layout_style4.cpp:47:0:
[task 2017-10-31T07:45:56.635Z] 07:45:56     INFO -  /builds/worker/workspace/build/src/layout/style/nsCSSRuleProcessor.cpp: In static member function 'static bool nsCSSRuleProcessor::LangPseudoMatches(const mozilla::dom::Element*, const nsAtom*, bool, const char16_t*, const nsIDocument*)':
[task 2017-10-31T07:45:56.635Z] 07:45:56     INFO -  /builds/worker/workspace/build/src/layout/style/nsCSSRuleProcessor.cpp:1689:16: error: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Werror=strict-overflow]
[task 2017-10-31T07:45:56.639Z] 07:45:56     INFO -     while (begin < len) {
[task 2017-10-31T07:45:56.639Z] 07:45:56     INFO -            ~~~~~~^~~~~

Fortunately, we fail the build when GCC would want to optimize it away.

The code was added in bug 1370802. CCing author and reviewer.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

I mean, I don't think gcc can optimize it away, can it? Though that warning is nice... I guess it can overflow if the string length is INT32_MAX...

Comment 2

[Mike Hommey [:glandium] OoO until Jun 18]

Great... this triggered when I landed my stuff:
https://treeherder.mozilla.org/logviewer.html#?job_id=141196420&repo=autoland

Comment 3

[Mike Hommey [:glandium] OoO until Jun 18]

Attachment: Bug 1413111 - Use nsTSubstring::Split in nsCSSRuleProcessor::LangPseudoMatches instead of manually go through with FindChar.

Review commit: https://reviewboard.mozilla.org/r/195252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195252/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8924035 [details]
Bug 1413111 - Use nsTSubstring::Split in nsCSSRuleProcessor::LangPseudoMatches instead of manually go through with FindChar.

https://reviewboard.mozilla.org/r/195252/#review200296


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/style/nsCSSRuleProcessor.cpp:1687
(Diff revision 1)
>    nsAutoString language;
>    aDocument->GetContentLanguage(language);
>  
>    nsDependentString langString(aString);
>    language.StripWhitespace();
> -  int32_t begin = 0;
> +  for (auto lang : language.Split(char16_t(','))) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

  for (auto lang : language.Split(char16_t(','))) {
            ^
       const  &

Comment 5

[Mike Hommey [:glandium] OoO until Jun 18]

Comment on attachment 8924035 [details]
Bug 1413111 - Use nsTSubstring::Split in nsCSSRuleProcessor::LangPseudoMatches instead of manually go through with FindChar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195252/diff/1-2/

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8924035 [details]
Bug 1413111 - Use nsTSubstring::Split in nsCSSRuleProcessor::LangPseudoMatches instead of manually go through with FindChar.

https://reviewboard.mozilla.org/r/195252/#review200312

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8924035 [details]
Bug 1413111 - Use nsTSubstring::Split in nsCSSRuleProcessor::LangPseudoMatches instead of manually go through with FindChar.

https://reviewboard.mozilla.org/r/195252/#review200314

r=dbaron conditional on tests passing in the non-stylo configuration (although I suppose we might not even have any tests for the Content-Language bit!)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da60fbc99531
Use nsTSubstring::Split in nsCSSRuleProcessor::LangPseudoMatches instead of manually go through with FindChar. r=dbaron,njn

Comment 9

[Mike Hommey [:glandium] OoO until Jun 18]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #7)
> (although I suppose we might not even have any tests for the
> Content-Language bit!)

For the record, as mentioned on irc, amazingly, there are Content-Language tests with and without commas, in the web-platform tests.

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/da60fbc99531