1010634 - Enable more compiler warnings (-Wall) for security/certverifier

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: fix-MFBT-and-XPCOM-warnings.patch

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: fix-warnings-in-certverifier.patch

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8423235 [details] [diff] [review]
fix-warnings-in-certverifier.patch

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

r+ with nit addresed

::: security/certverifier/OCSPCache.cpp
@@ +107,5 @@
>  {
>    Clear();
>  }
>  
>  // Returns -1 if no entry is found for the given (cert, issuer) pair.

nit: fix comment

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8423233 [details] [diff] [review]
fix-MFBT-and-XPCOM-warnings.patch

>diff --git a/mfbt/MathAlgorithms.h b/mfbt/MathAlgorithms.h
>-    return 32 + CountLeadingZeroes32(uint32_t(u));
>+    return 32u + CountLeadingZeroes32(uint32_t(u));

I'm not a MFBT reviewer, but given that the few changes here are just adding "u" suffixes to numeric literals to make their sign match the type they're being added to, I think that's minimal/obviously-correct enough that I'm happy to r+.

>diff --git a/xpcom/glue/DeadlockDetector.h b/xpcom/glue/DeadlockDetector.h
>-    static const HashEntryArray::index_type NoIndex = HashEntryArray::NoIndex;
>+    // XXX: NoIndex is an *unsigned* -1.
>+    static const HashEntryArray::index_type NoIndex =
>+      static_cast<HashEntryArray::index_type>(HashEntryArray::NoIndex);

Why do we need this?

nsTArray::NoIndex should already have been converted to index_type, here:
 http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#754

We also directly return it from nsTArray methods that return index_type (e.g. IndexOf). If we need a static_cast here, why don't we also need one there?

>+++ b/xpcom/string/public/nsCharTraits.h
>@@ -161,17 +161,17 @@ struct nsCharTraits<char16_t>
>   }
> 
>   static
>   char_type*
>   copyASCII( char_type* s1, const char* s2, size_t n )
>   {
>     for (char_type* s = s1; n--; ++s, ++s2) {
>       NS_ASSERTION(!(*s2 & ~0x7F), "Unexpected non-ASCII character");
>-      *s = *s2;
>+      *s = static_cast<char_type>(*s2);

As above, why is this needed? (And what are the warnings that you're actually fixing here?)

Is this for an implicit char-to-char16_t promotion? I'd expect that sort of conversion shouldn't trigger a build warning... I thought MSVC only warned for that sort of thing when you're *losing* precision (and other compilers don't tend to warn on assignments between types at all).


>diff --git a/xpcom/string/public/nsReadableUtils.h b/xpcom/string/public/nsReadableUtils.h
> inline size_t Distance( const nsReadingIterator<char>& start, const nsReadingIterator<char>& end )
> {
>-  return end.get() - start.get();
>+  if (end.get() < start.get()) {
>+    MOZ_CRASH("invalid arguments to Distance");
>+  }
>+  return static_cast<size_t>(end.get() - start.get());
> }

This check adds some runtime overhead, and MOZ_CRASH() will crash release builds, which is potentially overkill, depending on what's about to happen next. (MOZ_CRASH() is pretty much only ever appropriate if we know that continuing to run is unsafe. I don't think we need that level of paranoia here, at least not in all cases.)

I wonder if this method should really just return ptrdiff_t?  That would be more correct (since we're basically subtracting two pointers). Do you know if its clients depend on the result being unsigned?

Alternately, if we keep it as size_t, I'd suggest just using an assertion instead of a crash, and adding documentation to nsReadableUtils.h to make it clear that these functions assert fatally in debug builds if start is after end.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8423235 [details] [diff] [review]
fix-warnings-in-certverifier.patch

> // Returns -1 if no entry is found for the given (cert, issuer) pair.
>-int32_t
>+bool
> OCSPCache::FindInternal(const CERTCertificate* aCert,
>                         const CERTCertificate* aIssuerCert,
>+                        /*out*/ size_t& index,
>                         const MutexAutoLock& /* aProofOfLock */)
> {

Drive-by nit on this other patch: this "index" arg probably wants to be called "aIndex", for consistency with the other args and with general moz style.

Comment 6

[Daniel Holbert [:dholbert]]

(The fix-MFBT-and-XPCOM-warnings.patch probably should get review from someone who's written/reviewed XPCOM string internals code, too (i.e. not me). Maybe ehsan?)

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> >diff --git a/xpcom/glue/DeadlockDetector.h b/xpcom/glue/DeadlockDetector.h
> >-    static const HashEntryArray::index_type NoIndex = HashEntryArray::NoIndex;
> >+    // XXX: NoIndex is an *unsigned* -1.
> >+    static const HashEntryArray::index_type NoIndex =
> >+      static_cast<HashEntryArray::index_type>(HashEntryArray::NoIndex);
> 
> Why do we need this?
> 
> nsTArray::NoIndex should already have been converted to index_type, here:
>  http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#754

That implicit conversion causes an implicit-signed-to-unsigned conversion warning with Visual Studio. I believe that Visual Studio has decided to make NoIndex signed again even after that conversion because enums are (always?) signed in MSVC. I will try to make the enum an unsigned type using "enum : HashEntryArray::index_type" though that will trigger its own warning that I'll have to suppress for MSVC2010 because it things typed enums are a MSVC language extension.

> We also directly return it from nsTArray methods that return index_type
> (e.g. IndexOf). If we need a static_cast here, why don't we also need one
> there?

I only fixed warnings that are output during the compilation of CertVerifier. I suspect that because this is a template class, the compiler is only complaining about things that are actually used by CertVerifier.

> >       NS_ASSERTION(!(*s2 & ~0x7F), "Unexpected non-ASCII character");
> >-      *s = *s2;
> >+      *s = static_cast<char_type>(*s2);
> 
> As above, why is this needed? (And what are the warnings that you're
> actually fixing here?)

The warnings in the patch I asked you to review are all "implicit conversion from signed type to unsigned type" or "implicit conversion from unsigned type to signed type." In this case, char_type may be unsigned and char is signed.

> Is this for an implicit char-to-char16_t promotion?

No, this is for signed char to unsigned char promotion. All I'm doing is making the implicit conversion to char_type explicit to suppress the warning.

> I'd expect that sort of
> conversion shouldn't trigger a build warning... I thought MSVC only warned
> for that sort of thing when you're *losing* precision (and other compilers
> don't tend to warn on assignments between types at all).

Implicit conversion between signed and unsigned is another case that MSVC warns about.

> >diff --git a/xpcom/string/public/nsReadableUtils.h b/xpcom/string/public/nsReadableUtils.h
> > inline size_t Distance( const nsReadingIterator<char>& start, const nsReadingIterator<char>& end )
> > {
> >-  return end.get() - start.get();
> >+  if (end.get() < start.get()) {
> >+    MOZ_CRASH("invalid arguments to Distance");
> >+  }
> >+  return static_cast<size_t>(end.get() - start.get());
> > }
> 
> This check adds some runtime overhead, and MOZ_CRASH() will crash release
> builds, which is potentially overkill, depending on what's about to happen
> next. (MOZ_CRASH() is pretty much only ever appropriate if we know that
> continuing to run is unsafe. I don't think we need that level of paranoia
> here, at least not in all cases.)

> I wonder if this method should really just return ptrdiff_t?  That would be
> more correct (since we're basically subtracting two pointers). Do you know
> if its clients depend on the result being unsigned?

I looked at the callers of this function and they all are incredible unsafe and/or do the completely wrong thing if end > begin. Example:

   void
   AppendUnicodeTo( const nsAString::const_iterator& aSrcStart,
                    const nsAString::const_iterator& aSrcEnd,
                    nsAString& aDest )
   {
     nsAString::iterator writer;
     uint32_t oldLength = aDest.Length();
     aDest.SetLength(oldLength + Distance(aSrcStart, aSrcEnd));
   }

> Alternately, if we keep it as size_t, I'd suggest just using an assertion
> instead of a crash, and adding documentation to nsReadableUtils.h to make it
> clear that these functions assert fatally in debug builds if start is after
> end.

I'm not sure that we should be too worried about the runtime overhead here. The overhead is very small, especially if we have some way to mark the condition as UNLIKELY like the Linux kernel has. Like I said above, very bad things happen if end < begin.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #7)
> > nsTArray::NoIndex should already have been converted to index_type, here:
> >  http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#754
> 
> That implicit conversion causes an implicit-signed-to-unsigned conversion
> warning with Visual Studio. I believe that Visual Studio has decided to make
> NoIndex signed again even after that conversion because enums are (always?)
> signed in MSVC. I will try to make the enum an unsigned type using "enum :
> HashEntryArray::index_type" though that will trigger its own warning that
> I'll have to suppress for MSVC2010 because it things typed enums are a MSVC
> language extension.

OK. Might be worth spinning this part off into a separate bug, and having someone like bsmedberg r+.

> > We also directly return it from nsTArray methods that return index_type
> > (e.g. IndexOf). If we need a static_cast here, why don't we also need one
> > there?
> 
> I only fixed warnings that are output during the compilation of
> CertVerifier. I suspect that because this is a template class, the compiler
> is only complaining about things that are actually used by CertVerifier.

Ah, OK.

In any case, this private const "NoIndex" variable in DeadlockDetector.h is only used once, here:
 http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/DeadlockDetector.h#273
Maybe we can just drop the variable entirely and replace its one usage with HashEntryArray::NoIndex?  I imagine that would keep you from getting the warning in CertVerifier code, too.


> > >+      *s = static_cast<char_type>(*s2);
> > 
> > As above, why is this needed? (And what are the warnings that you're
> > actually fixing here?)
> 
> The warnings in the patch I asked you to review are all "implicit conversion
> from signed type to unsigned type" or "implicit conversion from unsigned
> type to signed type." In this case, char_type may be unsigned and char is
> signed.

I don't see how char_type is unsigned, offhand... nsCharTraits.h defines char_type in two different places, like so:
 91    typedef char16_t char_type;
 317   typedef char           char_type;

Are you sure we're getting an unsigned char_type? And if so, how / from where?

> > This check adds some runtime overhead, and MOZ_CRASH() will crash release
> > builds, which is potentially overkill, depending on what's about to happen
> > next. (MOZ_CRASH() is pretty much only ever appropriate if we know that
> > continuing to run is unsafe. I don't think we need that level of paranoia
> > here, at least not in all cases.)
[...]
> I looked at the callers of this function and they all are incredible unsafe
> and/or do the completely wrong thing if end > begin. Example:
[...]
> I'm not sure that we should be too worried about the runtime overhead here.
> The overhead is very small, especially if we have some way to mark the
> condition as UNLIKELY like the Linux kernel has.

(We do -- MOZ_UNLIKELY. Still, it's an extra check that we don't necessarily need)

> Like I said above, very bad
> things happen if end < begin.

That same argument would also mean that we should add fatal MOZ_CRASH() to all operator[] / ElementAt() accessors, for out-of-range access, and to any number of other functions when they're given bogus inputs.

Unless we have reason to believe that the args are going to be user-controlled or problematic in some way, I don't think we should bother with opt-build sanity-checks & crashes in cases like this. MOZ_ASSERT() is sufficient to get debug-build checking (and fuzzers can help strengthen our confidence that the assertion holds).

The specific example (AppendUnicodeTo) doesn't inherently look problematic to me, as long as *its* caller is passing in sane start/end variables. (If anything, perhaps *that* code should be asserting, or have a MOZ_CRASH if we want to be extra-paranoid.)

Anyway -- unless we have examples where client code really has some shadily-sourced (say, independently script-controlled) start/end pointers, I don't think it's worth adding an explicit runtime check & crash here.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> > The warnings in the patch I asked you to review are all "implicit conversion
> > from signed type to unsigned type" or "implicit conversion from unsigned
> > type to signed type." In this case, char_type may be unsigned and char is
> > signed.
> 
> I don't see how char_type is unsigned, offhand... nsCharTraits.h defines
> char_type in two different places, like so:
>  91    typedef char16_t char_type;
>  317   typedef char           char_type;

...and the changes in your patch are all in the section where "typedef char16_t char_type" is in effect. So AFAICT, char_type should be char16_t -- not unsigned.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> (In reply to Daniel Holbert [:dholbert] from comment #8)
> > > The warnings in the patch I asked you to review are all "implicit conversion
> > > from signed type to unsigned type" or "implicit conversion from unsigned
> > > type to signed type." In this case, char_type may be unsigned and char is
> > > signed.
> > 
> > I don't see how char_type is unsigned, offhand... nsCharTraits.h defines
> > char_type in two different places, like so:
> >  91    typedef char16_t char_type;
> >  317   typedef char           char_type;
> 
> ...and the changes in your patch are all in the section where "typedef
> char16_t char_type" is in effect. So AFAICT, char_type should be char16_t --
> not unsigned.

On Windows, we have "typedef wchar_t char16_t;" and wchar_t *is* unsigned on Windows: "Microsoft implements wchar_t as a two-byte unsigned value." - http://msdn.microsoft.com/en-us/library/dh8che7s.aspx. Thus, we have signed/unsigned comparison. Again, note that my casts are just making the implicit conversion to char_type explicit.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #10)
> On Windows, we have "typedef wchar_t char16_t;" and wchar_t *is* unsigned on
> Windows: "Microsoft implements wchar_t as a two-byte unsigned value."

Oh, I assumed char16_t was signed, based on the name. Reading up on it a bit more, it looks like it is indeed.  Anyway, this is probably a sign that you want ehsan or someone who's worked more on our string code than I have to review this. :)

> Again, note that my casts are just making the
> implicit conversion to char_type explicit.

Right, I understand that. But when there are build warnings about type mismatches, there's always the possibility that the code should be rewritten to not need casts in the first place; and doing a paperover static_cast will stop us from discovering that, & potentially hide bugs (or just hardcode in something that's kinda broken). So I'm not generally not comfortable r+'ing a static_cast unless I'm sure there's no reasonably clean way to workaround the need for a cast.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Oh, I assumed char16_t was signed, based on the name. Reading up on it a bit
> more, it looks like it is indeed.

sorry, left out a word -- "looks like it is indeed *unsigned*".

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8423233 [details] [diff] [review]
fix-MFBT-and-XPCOM-warnings.patch

So it does look like we have assertions to check that the 'char' in question is nonnegative, in all of the cases that you touch, like:
> NS_ASSERTION(!(*s2 & ~0x7F), "Unexpected non-ASCII character");
so I think the static_cast should be safe (or we'll assert if it's not).  Plus, these functions are explicitly about comparing ASCII to UTF-16, so I guess we can't really get around the signedness difference there.  So given that, the nsCharTraits.h changes seem fine to me.

Marking f+ instead of r+, since
 a) per previous comments, I suspect the MOZ_CRASH wants to be MOZ_ASSERT, and the NoIndex could be handled more gracefully dropping the variable (or more completely by making a change in nsTArray, so that we don't have to use static_cast absolutely everywhere it's referenced).
 b) I don't think I have enough ownership over this string code to sign off on changes there. Maybe request review from ehsan after addressing part (a)?

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8423235 [details] [diff] [review]
fix-warnings-in-certverifier.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/6deed68c2358

I fixed the comment before checking in the patch.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: fix-warnings-in-MFBT-and-XPCOM.patch

Thanks for the feedback, dholbert. I fixed the NoIndex issue by making NoIndex a static const member instead of an enum. I also replaced the use of MOZ_CRASH with MOZ_ASSERT even though I disagree! :)

Ehsan, dholbert suggested you would be a better reviewer. These changes are all addressing signed/unsigned conversion/comparison warnings that are triggered during compilation of security/certverifier with higher warning levels.

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: fix-warnings-in-JS.patch

Luke, this patch replaces two implicit signed/unsigned conversions with explicit conversions, to make MSVC happier when compiling security/certverifier with higher warning levels enabled. It is concerning that we're even including (indirectly) JS headers from secuirty/certverifier but I guess we are.

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: fix-more-warnings-in-CertVerifier.patch

This resolves more warnings in security/certverifier. It is hard to test the "giant input" check because we'd more likely OOM, but I added that check just to be safe. I think the port range check actually belongs in the other bug but whatever.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: enable-WAll-with-quite-a-few-exceptions.patch

Unfortunately, I still had to re-disable quite a few warnings. But, this will catch implicit integer truncation and signed/unsigned mismatches and other potentially-serious issues, so it's better than what we had before.

Comment 19

[Camilo Viecco (:cviecco)]

Comment on attachment 8423721 [details] [diff] [review]
fix-more-warnings-in-CertVerifier.patch

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

r+ with nit addressed

::: security/certverifier/OCSPRequestor.cpp
@@ +42,5 @@
> +    PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
> +    return nullptr;
> +  }
> +  uint32_t urlLen = PL_strlen(url);
> +  if (urlLen > static_cast<uint32_t>(std::numeric_limits<int32_t>::max())) {

nice

@@ +93,5 @@
>      return nullptr;
>    }
>    if (port == -1) {
>      port = 80;
> +  } else if (port < 0 || port > 0xffff) {

port 0 is reserved, I would count it as invalid

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6deed68c2358

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 8423718 [details] [diff] [review]
fix-warnings-in-JS.patch

Hi Brian!

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: private-NSSErrorsService-destructor.patch

Comment 23

[Camilo Viecco (:cviecco)]

Comment on attachment 8431312 [details] [diff] [review]
private-NSSErrorsService-destructor.patch

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

::: security/manager/ssl/src/NSSErrorsService.h
@@ +31,5 @@
>  
>  private:
> +  // For XPCOM implementations that are not a base class for some other
> +  // class, it is good practice to make the destructor non-virtual and
> +  // private.  Then the only way to delete the object is via Release.

Would be nice to point here to the recent dev-platform thread / bug number / doc

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Camilo Viecco (:cviecco) from comment #23)
> >  private:
> > +  // For XPCOM implementations that are not a base class for some other
> > +  // class, it is good practice to make the destructor non-virtual and
> > +  // private.  Then the only way to delete the object is via Release.

One nit on the language here -- currently, it sounds like you're saying the "non-virtual" aspect is part of what gets us this no-deletes-outside-of-release safety benefit. But I don't think non-virtual really impacts that at all. Non-virtual just gives us a perf win (from not needing a vtable lookup at destruction time), IIUC.  So perhaps consider dropping the mention of "non-virtual", or reword to make it clearer that that's not part of the safety guarantee here.
 
> Would be nice to point here to the recent dev-platform thread / bug number /
> doc

I'm hoping this pattern will be picked up across the codebase (there are probably thousands of classes that want this treatment).  (briansmith++ for adding it here!).  Given that, IMHO, it's not necessarily useful for each individual instance of the pattern to link to the dev-platform thread/bug# -- if that starts happening, it'll get too verbose.

If you like, though, feel free to use the same one-liner comment that I used in all the instances of this pattern that I've added:
    // Private destructor, to discourage deletion outside of Release():
(no pressure though)

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

> If you like, though, feel free to use the same one-liner comment that I used
> in all the instances of this pattern that I've added:
>     // Private destructor, to discourage deletion outside of Release():
> (no pressure though)

I will update the comment. I filed bug 1018323 about making creating a macro for making this convention clearer.

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #25)
> > If you like, though, feel free to use the same one-liner comment that I used
> > in all the instances of this pattern that I've added:
> >     // Private destructor, to discourage deletion outside of Release():
> > (no pressure though)
> 
> I will update the comment. I filed bug 1018323 about making creating a macro
> for making this convention clearer.

Actually, I'm going to leave the comment as-is. I had to add the comment to explain why I'm disabling the MSVC warning, which is about the non-virtual aspect (the performance optimization), not the private aspect (the safety). In bug 1018323 we can work out what better, if any, convention to use.

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20f228f5a33d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1f531152b77
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab9390487d1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9809ef17342c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e307284f7fa0

Thanks for all the reviews!

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/20f228f5a33d
https://hg.mozilla.org/mozilla-central/rev/e1f531152b77
https://hg.mozilla.org/mozilla-central/rev/ab9390487d1f
https://hg.mozilla.org/mozilla-central/rev/9809ef17342c
https://hg.mozilla.org/mozilla-central/rev/e307284f7fa0

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix for comm-central

I got an error on comm-central building.

 0:55.01 /usr/bin/ld.gold.real: error: /home/zoe/hg/comm-central/objdir-thunderbird/mozilla/toolkit/library/build/../../components/places/Unified_cpp_components_places0.o: requires dynamic R_X86_64_PC32 reloc against 'nsTArray_Impl<nsRefPtr<nsNavHistoryQueryResultNode>, nsTArrayInfallibleAllocator>::NoIndex' which may overflow at runtime; recompile with -fPIC
 0:55.01 /usr/bin/ld.gold.real: error: read-only segment has dynamic relocations
 0:55.02 /home/zoe/hg/comm-central/mozilla/toolkit/components/places/nsNavHistoryResult.cpp:1849: error: undefined reference to 'nsTArray_Impl<nsRefPtr<nsNavHistoryQueryResultNode>, nsTArrayInfallibleAllocator>::NoIndex'
 0:55.02 /home/zoe/hg/comm-central/mozilla/toolkit/components/places/nsNavHistoryResult.cpp:1852: error: undefined reference to 'nsTArray_Impl<nsRefPtr<nsNavHistoryQueryResultNode>, nsTArrayInfallibleAllocator>::NoIndex'

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8432287 [details] [diff] [review]
Fix for comm-central

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

Note I am not a Toolkit reviewer, but this seems like a trivial change so r=briansmith.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Karl Tomlinson (:karlt)]

Thanks, hiro.  That workaround was also necessary for Firefox with gcc 4.7.3, but I don't know why given
"There can be more than one definition of a [...] static data member of a
class template [...] provided that each definition appears in a different
translation unit [...]"

Comment 33

[Carsten Book [:Tomcat]]

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