1318376 - clang-cl -Wc++11-narrowing error in ServiceWorkerPrivate.cpp

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[(Away)]

As of last night's m-c this is the only file for which clang-cl has to fall back to MSVC.

24:03.52 D:/build/msys/s/central/dom/workers/ServiceWorkerPrivate.cpp(1354,12):  error(clang): case value evaluates to -1, which cannot be narrowed to type 'uint32_t' (aka 'unsigned int') [-Wc++11-narrowing]
24:03.52       case nsIHttpChannel::REFERRER_POLICY_UNSET:
24:03.52            ^
24:03.52 1 error generated.
24:03.52 clang-cl.EXE: warning: falling back to C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\cl.exe [-Wfallback]

We recently worked around a similar issue in Breakpad in bug 1312313. See also LLVM bug number 30776.

Comment 1

[Andrew Overholt [:overholt]]

Thomas, what do you think here? Andrew, should we add a workaround as in bug 1312313?

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Changing the moz.build file seems like the simplest course of action, yes.  We have 3 different enums that carry the referrer policy values (nsIHttpChannel's (IDL-generated enum), mozilla::net::ReferrerPolicy whose enum values are defined in terms of nsIHttpChannel's, and the independently defined WebIDL-generated mozilla::dom::ReferrerPolicy), so I think it's preferable to avoid messing with the enum.

Having said that, because the DOM Cache only stores mozilla::dom::ReferrerPolicy which uses its own enum-scale, it's not the end of the world to change the enum.  Like, I don't see any references in netwerk/cache2 because that should just be storing the header string, not the parsed value.

One thing I don't understand is how ServiceWorkerPrivate is the only place seeing this.  While it's the only C++ usage of REFERRER_POLICY_UNSET, all the other C++ code uses the equivalent mozilla::net::RP_Unset value in the context of mozilla::net::ReferrerPolicy that don't generate the failure.  Unless the checks gives up on those files for other reasons?  Examples:
* switch: http://searchfox.org/mozilla-central/source/dom/fetch/InternalRequest.h#240
* if: http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#3520


== context brain dump

My understanding is that because of https://llvm.org/bugs/show_bug.cgi?id=27098 which blocks the mentioned https://llvm.org/bugs/show_bug.cgi?id=30776, the nsIHttpChannel definition:
  const unsigned long REFERRER_POLICY_UNSET = 4294967295; // UINT32_MAX
which gets mapped to be as follows in nsIHttpChannel.h:
 enum {
    REFERRER_POLICY_UNSET = 4294967295U,
 ...
 }
makes clang sad because in "MS" (enum) mode, they treat the enum as signed when they should instead promote it to unsigned.  And also, the "unsigned long" type gets converted to uint32_t by IDL compilation, so the use of uint32_t in ServiceWorkerPrivate.cpp is correct.

The problem here is nsIHttpChannel's choice of a constant >INT32_MAX.  The value is also reused in ReferrerPolicy.h via:
  enum ReferrerPolicy {
  ...
    RP_Unset                       = nsIHttpChannel::REFERRER_POLICY_UNSET,
  };

Comment 3

[Thomas Nguyen (:tnguyen)]

REFERRER_POLICY_UNSET might be used more widely as in bug 1304623
And the uint32_t value of referrer policy (by IDL compilation) is being used in several places
https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsIHttpChannel%3A%3AGetReferrerPolicy%28uint32_t+%2A%29%22
You may don't want to add the workaround to severals moz.build (and also have to fix more if someone will use this kind of policy later).
It's fine to me to reorganize referrer value more reasonable in 
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#67

Comment 4

[Thomas Nguyen (:tnguyen)]

REFERRER_POLICY_UNSET might be used more widely as in bug 1304623
And the uint32_t value of referrer policy (by IDL compilation) is being used in several places
https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsIHttpChannel%3A%3AGetReferrerPolicy%28uint32_t+%2A%29%22
You may not want to add the workaround to severals moz.build (and also have to fix more if someone will use this kind of policy later).
It's fine to me to reorganize referrer-policy* value more reasonable in 
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#67

Comment 5

[Thomas Nguyen (:tnguyen)]

Attachment: Set value of REFERRER_POLICY* more reasonable to avoid error conversion from unsigned to signed

MozReview-Commit-ID: 8QVSYTsV82G

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8812047 [details] [diff] [review]
Set value of REFERRER_POLICY* more reasonable to avoid error conversion from unsigned to signed

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

I think it should be fine to renumber

Comment 7

[Thomas Nguyen (:tnguyen)]

Attachment: Set value of REFERRER_POLICY* more reasonable to avoid error conversion from unsigned to signed

MozReview-Commit-ID: J4er0G5dwim

Comment 8

[Thomas Nguyen (:tnguyen)]

Thanks Patrick.
Updated commit message carried r+

Comment 9

[Thomas Nguyen (:tnguyen)]

try result 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=084c18efc1e0a17e081825d9a5db2c330c607b43

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79ca00668528
Set value of REFERRER_POLICY* more reasonable to avoid error conversion from unsigned to signed. r=mcmanus

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/79ca00668528