1611567 - C-C TB uninitialized memory access during xpcshell-test, comm/mailnews/base/test/unit/test_postPluginFilters.js, Uninitialised value was created by a stack allocation at ... nsMsgDatabase::SetStringPropertyByHdr(nsIMsgDBHdr*, char const*, char const*)

Status: RESOLVED FIXED

(Thunderbird :: Filters, defect)

Description

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: valgrind log that show uninitialized memory created in |nsMsgDatabase::SetStringPropertyByHdr()| is accessed.

Here is yet another one of the uninitialized memory accesses during a run of C-C TB
xpcshell-test under valgrind.

I have a feeling that this problem will result in
incorrect processing of filters, but I am not sure. Anything goes actually.

I am attaching a log from serialized verbose run of xpcshell-test.
The line numbers in the log may be slightly off due to local changes.

It contains a lot of local debug dump, but you can discern the valgrind output.
(I am running xpcshell test by saving xpcshell binary into a file named |xpcshell-bin| and then create a binary named |xpcshell| that actually runs |xpcshell-bin| under valgrind.
Lines such as the following are from this wrapper:

73:41.81 pid:462226 sizeof(prepargs)=120
73:41.81 pid:462226 argc=31
73:41.82 pid:462226 finalargs[  0] = /usr/local/bin/valgrind
73:41.82 pid:462226 finalargs[  1] = --track-origins=yes
73:41.82 pid:462226 finalargs[  2] = --trace-children=yes
...

The test that triggers the uninitialized errors is
comm/mailnews/base/test/unit/test_postPluginFilters.js

All uninitializsed value(s) were created in the following function.

Uninitialised value was created by a stack allocation
    at 0x59E22AA: nsMsgDatabase::SetStringPropertyByHdr(nsIMsgDBHdr*, char const*, char const*) (nsMsgDatabase.cpp:2164)

The first use of the value seems to be:

72:48.56 pid:546339 ==546339== Conditional jump or move depends on uninitialised value(s)
72:48.56 pid:546339 ==546339==	  at 0x6D66962: XPCConvert::NativeData2JS(JSContext*, JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, unsigned int, nsresult*) (Value.h:996)
--

The function in Value.h:996 is:

static constexpr Value NumberValue(uint32_t i) {
  return i <= JSVAL_INT_MAX ? Int32Value(int32_t(i))
			    : Value::fromDouble(double(i)); <--- line 996
}

So this means an integer value (in C++) is being converted to a value in JS world.

Looking at the stack, it seems that the call sequence was triggered by some type of
function that is invoked at "On..."?

Anyway, what (unsigned) integer value do we create in SetStringPropertyByHdr() that are not initialized?

The function in full.

NS_IMETHODIMP nsMsgDatabase::SetStringPropertyByHdr(nsIMsgDBHdr *msgHdr,
						    const char *aProperty,
						    const char *aValue) {
  // don't do notifications if message not yet added to database.
  // Ignore errors (consequences of failure are minor).
  bool notify = true;
  nsMsgKey key = nsMsgKey_None;
  msgHdr->GetMessageKey(&key);
  ContainsKey(key, &notify);

  nsCString oldValue;
  nsresult rv = msgHdr->GetStringProperty(aProperty, getter_Copies(oldValue));
  NS_ENSURE_SUCCESS(rv, rv);

  // if no change to this string property, bail out
  if (oldValue.Equals(aValue)) return NS_OK;

  // Precall OnHdrPropertyChanged to store prechange status
  nsTArray<uint32_t> statusArray(m_ChangeListeners.Length());
  uint32_t status;
  nsCOMPtr<nsIDBChangeListener> listener;
  if (notify) {
    nsTObserverArray<nsCOMPtr<nsIDBChangeListener> >::ForwardIterator listeners(
	m_ChangeListeners);
    while (listeners.HasMore()) {
      listener = listeners.GetNext();
      listener->OnHdrPropertyChanged(msgHdr, true, &status, nullptr);
      // ignore errors, but append element to keep arrays in sync
      statusArray.AppendElement(status);
    }
  }

  rv = msgHdr->SetStringProperty(aProperty, aValue);
  NS_ENSURE_SUCCESS(rv, rv);

  // Postcall OnHdrPropertyChanged to process the change
  if (notify) {
    // if this is the junk score property notify, as long as we're not going
    // from no value to non junk
    if (!strcmp(aProperty, "junkscore") &&
	!(oldValue.IsEmpty() && !strcmp(aValue, "0")))
      NotifyJunkScoreChanged(nullptr);

    nsTObserverArray<nsCOMPtr<nsIDBChangeListener> >::ForwardIterator listeners(
	m_ChangeListeners);
    for (uint32_t i = 0; listeners.HasMore(); i++) {
      listener = listeners.GetNext();
      status = statusArray[i];
      listener->OnHdrPropertyChanged(msgHdr, false, &status, nullptr);
      // ignore errors
    }
  }

  return NS_OK;
}

I checked |nsCString oldValue| is innocent. Even if I initialize it by the following, the
uninitialized access persists. Besides such usage of nsCString permeats C-C code and they all work. So this particular declaration should not be an exception.

  nsCString oldValue("");

Looking at the code I notice two unsigned scalar values that are not initialized. (Access to an array ELEMENT is unsigned scalar value, so I count the array as part of investigation.)

nsTArray<uint32_t> statusArray(m_ChangeListeners.Length());
unit32_t status;

|status| ought to be set in the first call to

      listener->OnHdrPropertyChanged(msgHdr, true, &status, nullptr);

and is used on the second call to

      status = statusArray[i];
      listener->OnHdrPropertyChanged(msgHdr, false, &status, nullptr);

So |statusArray[i]| used in the second while-loop is uninitialized?
But |statusArray| is populated in the first call.

Aha, I think I found the culprit.
The following declaration seems to be incorrect.

  nsTArray<uint32_t> statusArray(m_ChangeListeners.Length());

This should be

  nsTArray<uint32_t> statusArray(0);

because the value of returned |status| is APPENDED to the array by

      statusArray.AppendElement(status);

So I take that the first |m_ChangeListeners.Length()| elements of |statusArray| stay uninitialized in the current code, and then in the second loop is referenced by

   for (uint32_t i = 0; listeners.HasMore(); i++) {
      listener = listeners.GetNext();
      status = statusArray[i];   <--- Here! From 0..listeners.Length() - 1, they are uninitialised?
      listener->OnHdrPropertyChanged(msgHdr, false, &status, nullptr);

I thought we can modify the first loop that runs through the listeners and sets value to statusArray to use |statusArray[i++] = status| or some such.
But given the style of using |.Next()| to loop through objects, I think simply creating an array object of length 0 is better.

What do people think of the analysis above?

CAVEAT EMPTOR: there seems to be an implicit assumption that the number of listeners DO NOT CHANGE AT ALL through this code, meaning TB's execution after this function is first called. Is this true and dependable?

BTW, I am runnintg the xpcshell-test under valgrind for another bug/fix.
It takes about 13 hours on my PC and would not want to terminate it to rebuild TB (and replace the currently running binary) since it is into about fifth hour of execution now.
(Actually, not exactly the WHOLE xpcshell-test. Some tests time out due to slowdown caused by valgrind . Although I tried to lengthen the timeout, I still cannot run some tests yet.)

I will test the proposed change of declaring the array of 0-length tomorrow. Since this uninitialized memory access happens only in one test, the valgrind verification would not take that long.

In the meantime, if someone finds the different cause (i.e., my analysis is wrong), please let me know.

TIA

Comment 1

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

I am not exactly sure of what |plugin| mean?
I searched for "plugin filter" and there are some hits in bugzilla.

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=plugin%20filter&list_id=15084149

Reading some old bug's comments, I have a feeling that this is the normal filter action...
So the bug reported here may contribute to some random flaky behavior of filtering. (?)

Comment 2

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Just doing

  nsTArray<;uint32_t> statusArray(0);

is not enough.

This suggests result returned by the first call of OnHdrPropertyChanged() may be uninitialized (!?)
I am investigating more.

Comment 3

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Something is very wrong. The first call to OnHdrPropertyChanged(msgHdr, false, &status, nullptr)
does not seem to return a value in |status|.
And it is not failing as a function call.
That is all for today.

Comment 4

[:aceman]

(In reply to ISHIKAWA, Chiaki (may be slow to respond until Jan 4.) from comment #0)

Aha, I think I found the culprit.
The following declaration seems to be incorrect.

  nsTArray<uint32_t> statusArray(m_ChangeListeners.Length());

This should be

  nsTArray<uint32_t> statusArray(0);

Doesn't this just allocate memory for the m_ChangeListeners.Length() array members, but does not create the members themselves? Thus nsTArray.Length() is still 0 ? Can you find the implementation of the naTArray constructor?

(In reply to ISHIKAWA, Chiaki (may be slow to respond until Jan 4.) from comment #3)

Something is very wrong. The first call to OnHdrPropertyChanged(msgHdr, false, &status, nullptr)
does not seem to return a value in |status|.
And it is not failing as a function call.

Do you mean the case of OnHdrPropertyChanged(msgHdr, true, &status, nullptr) ?

is 'status' undefined for every listener? Can you check the return value of OnHdrPropertyChanged() (the prechange='true' case).

See https://searchfox.org/comm-central/source/mailnews/db/msgdb/public/nsIDBChangeListener.idl#86 , the comment says something about undefined 'status' argument.

Maybe when 'status' is undefined we shouldn't call OnHdrPropertyChanged(msgHdr, false, &status, nullptr) in such case?

Comment 5

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to :aceman from comment #4)

(In reply to ISHIKAWA, Chiaki (may be slow to respond until Jan 4.) from comment #0)

Aha, I think I found the culprit.
The following declaration seems to be incorrect.

  nsTArray<uint32_t> statusArray(m_ChangeListeners.Length());

This should be

  nsTArray<uint32_t> statusArray(0);

Doesn't this just allocate memory for the m_ChangeListeners.Length() array members, but does not create the members themselves? Thus nsTArray.Length() is still 0 ? Can you find the implementation of the naTArray constructor?

nsTarray seems to be defined in comm-central/mozilla/xpcom/ds/nsTArray.h

I have not figured out the code yet. Maybe it would be easier to just print statusArray.Length() after the declaration.

(In reply to ISHIKAWA, Chiaki (may be slow to respond until Jan 4.) from comment #3)

Something is very wrong. The first call to OnHdrPropertyChanged(msgHdr, false, &status, nullptr)
does not seem to return a value in |status|.
And it is not failing as a function call.

Do you mean the case of OnHdrPropertyChanged(msgHdr, true, &status, nullptr) ?

You are right. My copy&paste mistake.
listener->OnHdrPropertyChanged(msgHdr, true, &status, nullptr);
did NOT return value(!).
That is |status| remained undefined UNLESS we set 0 or something to it after declaration.

is 'status' undefined for every listener? Can you check the return value of OnHdrPropertyChanged() (the prechange='true' case).

Yes, it is undefined for listener(s). [I am not sure what you mean by "every". In a test, there were two listeners. I am attaching a dump.
I set 0xa5a5a5a5a5a5a5 to |status| in the loop before the call to OnHdrProperyChanged().
There are two listeners and so two calls and each time the value is dumped, the value did not change.
This is how I inserted dump statements.

  // Precall OnHdrPropertyChanged to store prechange status
  nsTArray<uint32_t> statusArray(0);
  uint32_t status = 0xa5a5a5a5a5a5a5a5;
  nsCOMPtr<nsIDBChangeListener> listener;
  if (notify) {
    nsTObserverArray<nsCOMPtr<nsIDBChangeListener> >::ForwardIterator listeners(
        m_ChangeListeners);
    while (listeners.HasMore()) {
      listener = listeners.GetNext();
      status = 0xa5a5a5a5a5a5a5a5;
      nsresult rv = listener->OnHdrPropertyChanged(msgHdr, true, &status, nullptr);
      // ignore errors, but append element to keep arrays in sync
#ifdef DEBUG
      fprintf(stderr,"{debug} rv =%" PRIx32 ", returned status = %" PRIu32 "\n", 
              static_cast<uint32_t>(rv), status);
#endif

      statusArray.AppendElement(status);
    }
  }

  rv = msgHdr->SetStringProperty(aProperty, aValue);
  NS_ENSURE_SUCCESS(rv, rv);

  // Postcall OnHdrPropertyChanged to process the change
  if (notify) {
    // if this is the junk score property notify, as long as we're not going
    // from no value to non junk
    if (!strcmp(aProperty, "junkscore") &&
        !(oldValue.IsEmpty() && !strcmp(aValue, "0")))
      NotifyJunkScoreChanged(nullptr);

    nsTObserverArray<nsCOMPtr<nsIDBChangeListener> >::ForwardIterator listeners(
        m_ChangeListeners);
    for (uint32_t i = 0; listeners.HasMore(); i++) {
      listener = listeners.GetNext();
      status = statusArray[i];
#ifdef DEBUG
      fprintf(stderr,"{debug} returned statusArray[i=%d] = status = %" PRIu32 "\n", i, status);
#endif
      listener->OnHdrPropertyChanged(msgHdr, false, &status, nullptr);
      // ignore errors
    }
  }

0xA5A5A5A5 is 2779096485.
Actually 0xA5A5A5A5A5A5A5A5 is too large to store in a 32-bit entity (!) :-)

There are a outut printed in a test,
"comm/mailnews/base/test/unit/test_postPluginFilters.js", for example
as folows.:

 1:26.11 pid:583693 {debug} rv =0, returned status = 2779096485
 1:26.11 pid:583693 {debug} rv =0, returned status = 2779096485
 1:26.11 pid:583693 {debug} returned statusArray[i=0] = status = 2779096485
 1:26.11 pid:583693 {debug} returned statusArray[i=1] = status = 2779096485
 1:26.12 pid:583693 {debug} rv =0, returned status = 2779096485
 1:26.12 pid:583693 {debug} rv =0, returned status = 2779096485
 1:26.12 pid:583693 {debug} returned statusArray[i=0] = status = 2779096485
 1:26.12 pid:583693 {debug} returned statusArray[i=1] = status = 2779096485
 1:26.12 pid:583693 {debug} rv =0, returned status = 2779096485
 1:26.12 pid:583693 {debug} rv =0, returned status = 2779096485
 1:26.12 pid:583693 {debug} returned statusArray[i=0] = status = 2779096485
 1:26.12 pid:583693 {debug} returned statusArray[i=1] = status = 2779096485

There are a few more output. So I am attaching the full log for this test as separate file.

See https://searchfox.org/comm-central/source/mailnews/db/msgdb/public/nsIDBChangeListener.idl#86 , the comment says something about undefined 'status' argument.

Maybe when 'status' is undefined we shouldn't call OnHdrPropertyChanged(msgHdr, false, &status, nullptr) in such case?

But how can we find out how |status| is undefined?
This depend on the range of the value over which |status| can change legitimately.
Do we use a specific value as the indicator that |status| is undefined?

Note that there are several different implementations of |OnHdrPropertyChanged()| that seem to override the base class's definitions.
Some of them seem to be fully implemented. My browsing revealed that at least one of them is NOT implemented to return the value in |status| AT ALL.
Maybe we should investigate what these implementations are supposed to do and check their implementations.

Comment 6

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: log1171-xpcshell-memcheck.txt: test log for "comm/mailnews/base/test/unit/test_postPluginFilters.js"

valgrind monitoring of xpcshell-test for
"comm/mailnews/base/test/unit/test_postPluginFilters.js"

The interesting part starts at line 1262.
Everything before it is a dump to identify the environment on my PC
when the test was run.
First dump of |status| occurs at the line 1552.
Second dump of |status| occurs at the line 1777.

You can see that |status| remains to be the initialized value from the caller and never
changed inside |OnHdrPropertyChanged(msgHdr, true, &status, nullptr)|. :-(

At least my initialization of |status| now avoids the uninitialized memory access.
But whether the non-zero 0xa5a5a5a5 is a good initialization value or not is something we have to figure out. Maybe a zero seems to be a good candidate, but I am not so sure.

Comment 7

[:aceman]

Can you add rv= OnHdrPropertyChanged(msgHdr, true, &status, nullptr) and see whether 'rv' is a failure?

Comment 8

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to :aceman from comment #7)

Can you add rv= OnHdrPropertyChanged(msgHdr, true, &status, nullptr) and see whether 'rv' is a failure?

I think I have done that and that is visible in comment 5 and the log1171 attachment.
They seem to succeed, i.e., 0. (!)

Comment 9

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Failure from xpcshell-test if |status| is set to 0xa5a5a5a5a5.

Initializing status = 0xa5a5a5a5a5a5a5a5; has an interesting effect of
failing filter-related tests. Relevant error lines from the log is attached.
So I am initializing the variable to 0 both at the declaration and within the while loop, and continue testing locally.
With 0, the test seems to run just fine...

Comment 10

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Creating 0-length nsTarray and initialize |status| to 0 before use.

Here is the local patch I am testing.
With this patch, I see something like the following in the xpcshell-test log (we MUST add --verbose option to see the log AND if you run the whole tests, add --serialize so that the output from different test is not mixed...)

 0:58.87 pid:634616 {debug} rv =0, returned status = 0, statusArray.Length()=0
 0:58.87 pid:634616 {debug} rv =0, returned status = 0, statusArray.Length()=1
 0:58.87 pid:634616 {debug} returned statusArray[i=0] = status = 0
 0:58.87 pid:634616 {debug} returned statusArray[i=1] = status = 0
 0:58.87 pid:634616 {debug} rv =0, returned status = 0, statusArray.Length()=0
 0:58.87 pid:634616 {debug} rv =0, returned status = 0, statusArray.Length()=1
 0:58.87 pid:634616 {debug} returned statusArray[i=0] = status = 0
 0:58.87 pid:634616 {debug} returned statusArray[i=1] = status = 0
 0:58.87 pid:634616 received folder event FiltersApplied folder Inbox

You can see that the length grows as we append element. So I think the original code IS incorrect (!)

And another problem. The NEXT function in the source file that follows the function under discussion

NS_IMETHODIMP
nsMsgDatabase::SetUint32PropertyByHdr(nsIMsgDBHdr *aMsgHdr,
                                      const char *aProperty, uint32_t aValue) 

https://searchfox.org/comm-central/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#2219

has the same construct (I think it is a copy&paste job) and so it is also INCORRECT, I think.

The problem is compounded that it is NOT flagged as accessing uninitialized memory.
This means that we DO NOT HAVE A TEST that exercises this function in xpcshell-test (and probably in |make mozmill| now mochitest. I have not noticed the uninitialized memory access of this function before.)

Aceman, can you think of a simple test that exercises this function, SetUint32PropertyByHdr? Maybe tweaking an existing test?
It is called here.
https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#3364

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to ISHIKAWA, Chiaki from comment #10)

..
Aceman, can you think of a simple test that exercises this function, SetUint32PropertyByHdr? Maybe tweaking an existing test?
It is called here.
https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#3364

Comment 12

[Wayne Mery (:wsmwk)]

I have a feeling that this problem will result in incorrect processing of filters, but I am not sure.

Do you have an updated opinion?

Comment 13

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Wayne Mery (:wsmwk) from comment #12)

I have a feeling that this problem will result in incorrect processing of filters, but I am not sure.

Do you have an updated opinion?

Hard to tell.

However, I will update the patch to reflect the new directory structure if necessary, and try to have this landed.
Hopefully this coming weekend.
Famous Last Words.

Comment 14

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: create-0-length-statusArray-Rev01.patch modified for subdirectory shuffle, etc. It works with the current M-C/C-C.

The current, but a verbose patch candidate.
It also takes care of |SetUint32PropertyByHdr()|.
I have used this for local test and tyrserver submission for several months.
I moved this near the top of my local patch queue to land it.

I will remove the DEBUG output in the next round.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9188697 [details] [diff] [review]
create-0-length-statusArray-Rev01.patch modified for subdirectory shuffle, etc. It works with the current M-C/C-C.

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

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +2283,2 @@
>        // ignore errors, but append element to keep arrays in sync
> +      (void) rv;    // shut up unused variable compiler warning 

you should use DebugOnly<nsresult> rv =

@@ +2286,5 @@
> +#ifdef __linux__
> +      // A mess.
> +      // PRIu64 seems to be for long unsigned int under linux (gcc9)
> +      // But under OSX and windows,
> +      // it seems to be for unsigned long long (Feb. 2020)

not sure how much debug spew we want to add, but please remove long comments like these, which is likely to change much over time

Comment 16

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Patch take two.

Modified patch.

No additional output.

No use of rv. Simplified the code.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9189442 [details] [diff] [review]
Patch take two.

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

Please make the changes mentioned here and let benc review

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +2266,5 @@
>    // if no change to this string property, bail out
>    if (oldValue.Equals(aValue)) return NS_OK;
>  
>    // Precall OnHdrPropertyChanged to store prechange status
> +  // We grow it as necessary.

Not needed, it's obvious from the code later.

@@ +2277,5 @@
>      while (listeners.HasMore()) {
>        listener = listeners.GetNext();
> +      // initialize |status| because some implementations of
> +      // OnHdrPropertyChanged does not sets the value.
> +      status = 0;

please just declare it here as well, then we don't even need the comment.

uint32_t status = 0;

... and also below in the similar case. Declare where need is just good practice (would have prevented this bug!), and here the results are not re-used.

Comment 18

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Thank you for the comment. I will respin the patch.

Comment 19

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Modified patch. Take three.

I reflected the comment from Magnus, but
I kpet the comment where he thought we may not need it.

please just declare it here as well, then we don't even need the comment.

Well, that the function being called did NOT return value in some cases, and DOES return a value
in other cases was confusing enough (possibly the reason for the uninitialized value references later on) and that was why I felt it was necessary to alert the future maintainer/developer in the first place. So I kept the comment.

Comment 20

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Modified patch. Take four

Modified because a declaration for |status| was missing.
I could not compile TB for a couple of days due to header issues and a compiler issue and could not figure it out.

Comment 21

[Ben Campbell]

Comment on attachment 9189774 [details] [diff] [review]
Modified patch. Take four

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

Looks good to me (with a quick disclaimer: I haven't dug into the original valgrind report in any detail - I'm just reviewing the patch in isolation rather than looking at the bigger picture :- )

I think the _real_ issue the fact that some of the onHdrPropertyChanged() implementations leave the status value unchanged. I just wrote it up as Bug 1679349.
(but I still think this patch should be landed)

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +2266,5 @@
>    // if no change to this string property, bail out
>    if (oldValue.Equals(aValue)) return NS_OK;
>  
>    // Precall OnHdrPropertyChanged to store prechange status
> +  nsTArray<uint32_t> statusArray(0);

The parameter is a capacity hint rather than length, so I think the original m_ChangeListeners.Length() makes sense - we know what size we the array will end up as. 0 will still work fine, but causes a little more memory reallocation as items are appended.

Comment 22

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Ben Campbell from comment #21)

Comment on attachment 9189774 [details] [diff] [review]
Modified patch. Take four

Review of attachment 9189774 [details] [diff] [review]:

Looks good to me (with a quick disclaimer: I haven't dug into the original
valgrind report in any detail - I'm just reviewing the patch in isolation
rather than looking at the bigger picture :- )

Thank you for the review.

I think the real issue the fact that some of the onHdrPropertyChanged()
implementations leave the status value unchanged. I just wrote it up as Bug
1679349.

That is something I could not figure out. I hope it will get more attention in the new bugzilla.

(but I still think this patch should be landed)

Agreed. Otherwise, we will be doing something funny with uninitialized value.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +2266,5 @@

// if no change to this string property, bail out
if (oldValue.Equals(aValue)) return NS_OK;

// Precall OnHdrPropertyChanged to store prechange status

• nsTArray<uint32_t> statusArray(0);

The parameter is a capacity hint rather than length, so I think the original
m_ChangeListeners.Length() makes sense - we know what size we the array will
end up as. 0 will still work fine, but causes a little more memory
reallocation as items are appended.

Well, I hope I can land this patch as is. We have spent enough time already.
The issue is if we declare the size, I think we should NOT use |.Append()| simply but maybe use
|statusArray[j++] = status| construct instead of |statusArray.AppendElement(status);|
No?
I thought |statusArray.AppendElement(status);| appends value to the last element of already declared array with size.
as in
at the initial loop
|@0 | ... |@(declared_size - 1)|
becomes
|@0 | ... |@(declared_size - 1)| | status|
and subsequently appended at the end
|@0| ... |last element| |status|

If it is OK, I would place checkin-needed-tb keyword.

Comment 23

[Ben Campbell]

Attachment: 1611567-take-five.patch

(In reply to ISHIKAWA, Chiaki from comment #22)

Sorry about the slow response but I missed your comment!

The issue is if we declare the size, I think we should NOT use |.Append()| simply but maybe use
|statusArray[j++] = status| construct instead of |statusArray.AppendElement(status);|
No?

Ahh, I see the confusion. Size (.Length()) and Capacity are different things.
.Length() gives the number of elements in the array, as you'd expect.
Capacity is how many elements the array can hold before it needs to reallocate memory.
So, with this line:

nsTArray<uint32_t> statusArray(42);

statusArray.Length() will return 0, and you can call .AppendElement() 42 times before the array will need to reallocate memory to accommodate more elements.

In this case, pre-sizing the array and using |statusArray[j++] = status| would be fine too. But using the capactity hint and AppendElement() is cleaner for any types that have constructors/destructors, so I tend to use that pattern everywhere. For plain-old-datatypes like int, I'd imagine they should compile down to equivalent the same code.

If it is OK, I would place checkin-needed-tb keyword.

I just tweaked your patch to add the nsTArray<uint32_t> statusArray(m_ChangeListeners.Length()); and I'll set it the checkin flag.

Thanks!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/21d2344dd2ae
avoid uninitialized access to |status|. r=benc

Comment 25

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Ben Campbell from comment #23)

Created attachment 9192558 [details] [diff] [review]
1611567-take-five.patch

(In reply to ISHIKAWA, Chiaki from comment #22)

Sorry about the slow response but I missed your comment!

The issue is if we declare the size, I think we should NOT use |.Append()| simply but maybe use
|statusArray[j++] = status| construct instead of |statusArray.AppendElement(status);|
No?

Ahh, I see the confusion. Size (.Length()) and Capacity are different things.
.Length() gives the number of elements in the array, as you'd expect.
Capacity is how many elements the array can hold before it needs to reallocate memory.
So, with this line:

nsTArray<uint32_t> statusArray(42);

statusArray.Length() will return 0, and you can call .AppendElement() 42 times before the array will need to reallocate memory to accommodate more elements.

In this case, pre-sizing the array and using |statusArray[j++] = status| would be fine too. But using the capactity hint and AppendElement() is cleaner for any types that have constructors/destructors, so I tend to use that pattern everywhere. For plain-old-datatypes like int, I'd imagine they should compile down to equivalent the same code.

If it is OK, I would place checkin-needed-tb keyword.

I just tweaked your patch to add the nsTArray<uint32_t> statusArray(m_ChangeListeners.Length()); and I'll set it the checkin flag.

Thanks!

I see. Thank you!
Maybe the uninitialized data that I talked about may come from the uninitialized |status| from a few routines.　Yes, I was confused. :-)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/49722c3c942b
followup - apply clang-format. rs=clang-format

Comment 27

[Magnus Melin [:mkmelin]]

Please remember to run clang-format for patches:
./mach clang-format -p comm/mailnews

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9192558 [details] [diff] [review]
1611567-take-five.patch

[Approval Request Comment]
Avoid potential memory corruption in unknown situations.

Comment 29

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Magnus Melin [:mkmelin] from comment #27)

Please remember to run clang-format for patches:
./mach clang-format -p comm/mailnews

Oops, sorry. I have not run it...
Next time, I will do it. (I think there is an automatic check in tryserver and I should have realized this.)

Comment 30

[Magnus Melin [:mkmelin]]

No, the try server only does js.
For m-c they have a phabricator bot that checks stuff when you upload, but not yet for c-c.

Comment 31

[Wayne Mery (:wsmwk)]

Comment on attachment 9192558 [details] [diff] [review]
1611567-take-five.patch

[Triage Comment]
Approved for esr78

Comment 32

[Rob Lemley [:rjl]]

Thunderbird 78.6.1:
https://hg.mozilla.org/releases/comm-esr78/rev/686c0bb2272c
https://hg.mozilla.org/releases/comm-esr78/rev/34af00ae3973