Closed Bug 1205942 Opened 5 years ago Closed 5 years ago

Fix and disallow warnings in ipc/chromium/

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(5 files)

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.
Summary: Fix and disallow warnings in ipc/chromium → Fix and disallow warnings in ipc/chromium/
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.
Attachment #8662750 - Flags: review?(jld)
Depends on: 1206558
Depends on: 1181026
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.
Attachment #8662750 - Flags: review?(jld) → review+
The warning is "the address of NuwaMarkCurrentThread() will always evaluate to
'true'".
Attachment #8663919 - Flags: review?(jld)
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.)
Attachment #8663919 - Flags: review?(jld) → review+
> (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.
(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.
https://hg.mozilla.org/mozilla-central/rev/b9eb41ac7569
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
Attachment #8668751 - Flags: review?(jld)
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().
Attachment #8668752 - Flags: review?(jld)
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.
Attachment #8668751 - Flags: review?(jld) → review+
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.
Attachment #8668752 - Flags: review?(jld) → review+
Comment on attachment 8668753 [details] [diff] [review]
(part 5) - Disallow compiler warnings in ipc/chromium

\o/
Attachment #8668753 - Flags: review?(jld) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.