1205942 - Fix and disallow warnings in ipc/chromium/

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nicholas Nethercote [inactive]]

We get lots of compile warnings in ipc/chromium/. Some of them are alarming. They should all be fixed, and the ALLOW_COMPILER_WARNING=True in ipc/chromium/moz.build should be removed.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Fix overflows in time_posix.cc

We get the following warnings with clang.

> ipc/chromium/src/base/time_posix.cc:103:57: error: overflow in expression; result is 0 with type 'long' [-Werror,-Winteger-overflow]
> ipc/chromium/src/base/time_posix.cc:106:58: error: overflow in expression; result is -1000 with type 'long' [-Werror,-Winteger-overflow]

This is a genuine bug. The upstream code in Chromium has changed and this patch
changes our code to be similar. I did tests and confirmed that instead of
getting 0 or -1 for |milliseconds|, we now get -2147483648000 or 2147483647999,
which is much better.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8662750 [details] [diff] [review]
(part 1) - Fix overflows in time_posix.cc

Review of attachment 8662750 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.  It might be nice to mention upstream commit 2a278516943eee02e0206506a4b907fc0b55f27b (or some reasonable prefix of that) in the commit message.  And hopefully we won't still be using this code in 2038.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9eb41ac7569

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Fix "always true" warning in child_thread.cc

The warning is "the address of NuwaMarkCurrentThread() will always evaluate to
'true'".

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8663919 [details] [diff] [review]
(part 2) - Fix "always true" warning in child_thread.cc

Review of attachment 8663919 [details] [diff] [review]:
-----------------------------------------------------------------

This is a little bit subtle — NuwaMarkCurrentThread is declared MFBT_API, which includes __attribute__((weak)) ifdef MOZ_GLUE_IN_PROGRAM, and weak symbols can be null, but B2G is not a MOZ_GLUE_IN_PROGRAM platform and Nuwa is B2G-only.  So that works.  r=me.

(Also, I see another assertion that NuwaMarkCurrentThread is non-null in js/src/vm/HelperThreads.cpp and I'm curious why it's not breaking the build, but that's outside the scope of this bug.)

Comment 6

[Nicholas Nethercote [inactive]]

> (Also, I see another assertion that NuwaMarkCurrentThread is non-null in
> js/src/vm/HelperThreads.cpp and I'm curious why it's not breaking the build,
> but that's outside the scope of this bug.)

I removed a similar assertion just recently in another bug: https://hg.mozilla.org/mozilla-central/rev/8dbe5f85a008

Even if the compiler is wrong and null is possible here, having a non-fatal assertion just prior to use isn't much good.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Even if the compiler is wrong and null is possible here, having a non-fatal
> assertion just prior to use isn't much good.

Agreed.

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/963e0aa6c030

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/963e0aa6c030

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove unused OS version detection functions from ipc/chromium/

The motivation here is to remove the GetVersionEx() calls in the
Windows-specific code, because that function is deprecated and causes warnings.
The non-Windows versions come along for the ride.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove GetWinVersion()

Like the last patch, the motivation is to remove a GetVersionEx() call which
triggers deprecation warnings.

Because Windows XP SP2 is the earliest Windows version we support, two of the
existing uses of GetWinVersion() could be removed, because they were checking
for XP or earlier. One other Vista check could be replaced with
mozilla::IsVistaOrLater().

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Disallow compiler warnings in ipc/chromium

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8668751 [details] [diff] [review]
(part 3) - Remove unused OS version detection functions from ipc/chromium/

Dead code removal in ipc/chromium: pretty much always a good thing.

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8668752 [details] [diff] [review]
(part 4) - Remove GetWinVersion()

Review of attachment 8668752 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.   Removing ipc/chromium stuff where we have mozilla:: equivalents is also nice to see.

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8668753 [details] [diff] [review]
(part 5) - Disallow compiler warnings in ipc/chromium

\o/

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/954cf73e65c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f889db067780
https://hg.mozilla.org/integration/mozilla-inbound/rev/360e120fb8d7

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/954cf73e65c5
https://hg.mozilla.org/mozilla-central/rev/f889db067780
https://hg.mozilla.org/mozilla-central/rev/360e120fb8d7