Closed Bug 1413111 Opened 5 years ago Closed 5 years ago

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]

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

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.
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...
Great... this triggered when I landed my stuff:
https://treeherder.mozilla.org/logviewer.html#?job_id=141196420&repo=autoland
Summary: ayout/style/nsCSSRuleProcessor.cpp:1689:16: error: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Werror=strict-overflow] → 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]
Assignee: nobody → mh+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/#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 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
Attachment #8924035 - Flags: review+
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!)
Attachment #8924035 - Flags: review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/da60fbc99531
Use nsTSubstring::Split in nsCSSRuleProcessor::LangPseudoMatches instead of manually go through with FindChar. r=dbaron,njn
(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.
https://hg.mozilla.org/mozilla-central/rev/da60fbc99531
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.