1155963 - Don't allow NS_NAMED_LITERAL_CSTRING(name, NS_LITERAL_CSTRING("value"))

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[[e-mail]]

The wide equivalent won't compile because of an extra MOZ_UTF16 macro.

Comment 1

[[e-mail]]

Attachment: Possible patch

I suppose I could #ifdef the change to only apply to narrow strings...

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8594318 [details] [diff] [review]
Possible patch

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

::: caps/nsPrincipal.cpp
@@ +633,5 @@
>    NS_ENSURE_SUCCESS(rv, false);
>  
>    // NOTE: This static whitelist is expected to be short. If that changes,
>    // we should consider a different representation; e.g. hash-set, prefix tree.
> +  const nsLiteralCString sFullDomainsOnWhitelist[] = {

Does the compiler actually complain if this is |static|?  GCC seems to be perfectly happy with it.

Please make this |static| if possible.

@@ +666,5 @@
>  // unprefixing for all its subdomains)
>  static bool
>  IsOnBaseDomainWhitelist(nsIURI* aURI)
>  {
> +  const nsLiteralCString sBaseDomainsOnWhitelist[] = {

Likewise for the |static|-ness here.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +580,5 @@
>      DeviceStorageTypeChecker* typeChecker
>        = DeviceStorageTypeChecker::CreateOrGet();
>      MOZ_ASSERT(typeChecker);
>  
> +    const nsLiteralString kMediaTypes[] = {

Likewise for the |static|-ness here.

Comment 3

[[e-mail]]

Are static constructors/destructors no longer frowned upon?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to [e-mail] from comment #3)
> Are static constructors/destructors no longer frowned upon?

Static constructors at global scope are frowned upon, but static constructors at function scope are OK, since they won't execute until the function is called.  (Indeed, that's how we got rid of a number of global-scope static constructors: moving them to function scope.)

Comment 5

[[e-mail]]

So, this only compiles under MSVC because it doesn't follow C++ rules on initialising arrays (they are copy-initialised so they need a copy constructor, sigh). I could add a default move constructor but I would have to #ifdef it out for MSVC as it doesn't understand that.

I looked into an alternative approach which was to ensure that NS_LITERAL_CSTRING was only passed actual literals, but I found a couple of places that forward it a template parameter:

> template <size_t LEN>
> nsresult Dispatch(const char (&taskThreadName)[LEN])
> {
>   static_assert(LEN <= 15,
>                 "Thread name must be no more than 15 characters");
>   return Dispatch(NS_LITERAL_CSTRING(taskThreadName));
> }
> 
> nsresult Dispatch(const nsACString& taskThreadName);

GetCachedStatement does something similar. Interestingly RegisterDefineDOMInterface does something similar, but as it's dealing with wide characters it has to use nsLiteralString directly, so maybe Dispatch and GetCachedStatement should be using nsLiteralCString.

Comment 6

[[e-mail]]

Attachment: 1155963.diff

Sorry about the scope creep, thus the second set of eyes.

Comment 7

[(no longer active)]

Comment on attachment 8600576 [details] [diff] [review]
1155963.diff

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

::: accessible/atk/Platform.cpp
@@ +360,5 @@
>      return sShouldEnable;
>  #endif
>  
>    //check gconf-2 setting
> +#define sGconfAccessibilityKey "/desktop/gnome/interface/accessibility"

Nit: please call this GCONF_A11Y_KEY or some such.

::: dom/indexedDB/ActorsParent.cpp
@@ +209,5 @@
>  
>  // The length of time that idle threads will stay alive before being shut down.
>  const uint32_t kConnectionThreadIdleMS = 30 * 1000; // 30 seconds
>  
> +#define kSavepointClause "SAVEPOINT sp;"

Here and elsewhere too.

::: toolkit/xre/nsUpdateDriver.cpp
@@ -93,5 @@
> -static const char kUpdaterApp[] = "updater.app";
> -#endif
> -#if defined(XP_UNIX) && !defined(XP_MACOSX)
> -static const char kUpdaterPNG[] = "updater.png";
> -#endif

Please don't change the conditions under which these names are defined.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8600576 [details] [diff] [review]
1155963.diff

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

Whoops, I r+'d this yesterday, but forgot to submit!

Comment 9

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17cfad44e12b

Comment 10

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

This apparently broke b2g builds, so backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e43f6dc95fc5

https://treeherder.mozilla.org/logviewer.html#?job_id=9863733&repo=mozilla-inbound

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/045afee26805

Comment 12

[[e-mail]]

(In reply to Pulsebot from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/045afee26805

(With bustage fix obviously. Also I forgot that pulsebot would spam the bug for me, so now there's an unnecssary extra comment.)

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/045afee26805