Bugzilla

(In reply to ISHIKAWA, Chiaki from comment #4)
 ...
> This patch here has an interesting implication. There is a discussion of what should happen and does not happen
> when the incoming message somehow lacks "Date:" header line or something to that effect..
> 
> There is a code that is protected by checking of mDatabase.
> if (mDatabase) mDatabase->SetLastUseTime(PR_Now());
> 
> I think failure to set |mDatabase| to a valid value may have been the cause of mDatabase->SetLastUsetime(...)  not being called and thus
> there is a timestamp issue of a message that comes in without "Date:" header line.
> I was about to check this issue to land this patch and clear my local patch queues (I am still using "outdated" HG MQUEUE extension which seems to have been discouraged for a long time.).
> Then I hit the build issue.

I checked, and my particular worry/attention on the Date issue of a new incoming message without Date: header was unfounded.
Without the patch, i.e., the distributed version of TB under linux, 78.13.0 (64-bit) properly added Date: header with the timestamp of PR_Now(), when an incoming messaged does not have a Date: header.
The code in question is here.
https://searchfox.org/comm-central/source/mailnews/base/src/nsMsgDBFolder.cpp#979
```
NS_IMETHODIMP
nsMsgDBFolder::GetMsgDatabase(nsIMsgDatabase** aMsgDatabase) {
  NS_ENSURE_ARG_POINTER(aMsgDatabase);
  GetDatabase();
  if (!mDatabase) return NS_ERROR_FAILURE;
  NS_ADDREF(*aMsgDatabase = mDatabase);
  mDatabase->SetLastUseTime(PR_Now());
  return NS_OK;
}
```
So at least here, the issue of mDatabase being null was not real in my limited testing.
But as I noted in  the original bug, there WERE places where mDatabase was null in the past and |if (mDatabase) { ... }| pattern is found in many places of TB code  base.
 
I inserted check to dump whether mDatabase is non null (TRUE) or null (FALSE) on the  lines where nullness of mDatabase is checked in key subdirectories such as local mail handling, pop3, etc.

In this particular case, my patch does show that mDatabase is not null in a few test cases.
```
{debug}: nsMsgLocalMailFolder::GetDatabaseWOReparse(nsIMsgDatabase **aDatabase) - mDdatabase = 0x0x5586bd6ffbe0
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:246
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:257
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:995 <---
{debug}: nsMsgLocalMailFolder::GetDatabaseWOReparse(nsIMsgDatabase **aDatabase) - mDdatabase = 0x(nil)
check_mDatabase_bool: FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:246
(debug): mDatabase = 0x(nil) on etry to  nsMsgLocalMailFolder::OpenDatabase()
(debug): nsMsgLocalMailFolder::OpenDatabase() rv = msgDBService->OpenFolderDB(this, true, getter_AddRefs(mDatabase)):
rv = 0x00000000, NS_MSG_ERROR_FOLDER_SUMMARY_MISSING =0x80550006
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:248
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:257
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:995 <---

<--- marked part is where the check was made in my locally patched file.
mDatabase was true and thus proper date (PR_Now()) is added to the messages coming in without Date: header line.

```

Given that my patch basically corrects the fundamental oversight problem (which other parts of the code base tried to mitigate by adding |if (mDatabase)| so far), and my local runs my tryserver runs in the past several months with this patch did not produce ill-effect (no new issues were found), I propose to incorporate this patch in to C-C source tree so that future DB maintainer won't forget to take care of this mDatabase handling before/after |UpdateNewMessages()|. 
At least a clear comment and proper fix should be in order to avoid any misunderstanding int the future.

I am requesting review from Ben.


To me, TB functions with so many bandage |if (mDatabase)| fix without visible serious problems is a wonder.

BTW, sending a mail message without "Date:" header line can be done easily under linux like this.
Below I am sending an email to a local user |ishikawa| and only write From: and Subject: header lines.
```
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ /usr/sbin/sendmail ishikawa
From: ishikawa
Subject: test without date header

test
.
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ 
```

(In reply to ISHIKAWA, Chiaki from comment #4)
 ...
> This patch here has an interesting implication. There is a discussion of what should happen and does not happen
> when the incoming message somehow lacks "Date:" header line or something to that effect..
> 
> There is a code that is protected by checking of mDatabase.
> if (mDatabase) mDatabase->SetLastUseTime(PR_Now());
> 
> I think failure to set |mDatabase| to a valid value may have been the cause of mDatabase->SetLastUsetime(...)  not being called and thus
> there is a timestamp issue of a message that comes in without "Date:" header line.
> I was about to check this issue to land this patch and clear my local patch queues (I am still using "outdated" HG MQUEUE extension which seems to have been discouraged for a long time.).
> Then I hit the build issue.

I checked, and my particular worry/attention on the Date issue of a new incoming message without Date: header was unfounded.
Without the patch, i.e., the distributed version of TB under linux, 78.13.0 (64-bit) properly added Date: header with the timestamp of PR_Now(), when an incoming messaged does not have a Date: header.
The code in question is here.
https://searchfox.org/comm-central/source/mailnews/base/src/nsMsgDBFolder.cpp#979
```
NS_IMETHODIMP
nsMsgDBFolder::GetMsgDatabase(nsIMsgDatabase** aMsgDatabase) {
  NS_ENSURE_ARG_POINTER(aMsgDatabase);
  GetDatabase();
  if (!mDatabase) return NS_ERROR_FAILURE;
  NS_ADDREF(*aMsgDatabase = mDatabase);
  mDatabase->SetLastUseTime(PR_Now());
  return NS_OK;
}
```
So at least here, the issue of mDatabase being null was not real in my limited testing.
But as I noted in  the original bug, there WERE places where mDatabase was null in the past and |if (mDatabase) { ... }| pattern is found in many places of TB code  base.
 
I inserted check to dump whether mDatabase is non null (TRUE) or null (FALSE) on the  lines where nullness of mDatabase is checked in key subdirectories such as local mail handling, pop3, etc.

In this particular case, my patch does show that mDatabase is not null in a few test cases.
```
{debug}: nsMsgLocalMailFolder::GetDatabaseWOReparse(nsIMsgDatabase **aDatabase) - mDdatabase = 0x0x5586bd6ffbe0
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:246
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:257
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:995 <---
{debug}: nsMsgLocalMailFolder::GetDatabaseWOReparse(nsIMsgDatabase **aDatabase) - mDdatabase = 0x(nil)
check_mDatabase_bool: FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:246
(debug): mDatabase = 0x(nil) on etry to  nsMsgLocalMailFolder::OpenDatabase()
(debug): nsMsgLocalMailFolder::OpenDatabase() rv = msgDBService->OpenFolderDB(this, true, getter_AddRefs(mDatabase)):
rv = 0x00000000, NS_MSG_ERROR_FOLDER_SUMMARY_MISSING =0x80550006
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:248
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:257
check_mDatabase_bool: TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:995 <---

<--- marked part is where the check was made in my locally patched file.
mDatabase was not null and thus proper date (PR_Now()) is added to the messages coming in without Date: header line.

```

Given that my patch basically corrects the fundamental oversight problem (which other parts of the code base tried to mitigate by adding |if (mDatabase)| so far), and my local runs and  my tryserver runs in the past several months with this patch did not produce ill-effect (no new issues were found), I propose to incorporate this patch in to C-C source tree so that future DB maintainer won't forget to take care of this mDatabase handling before/after |UpdateNewMessages()|. 
At least a clear comment and proper fix should be in order to avoid any misunderstanding in the future.

I am requesting review from Ben.


To me, TB functions with so many bandage |if (mDatabase)| fix without visible serious problems is a wonder.

BTW, sending a mail message without "Date:" header line can be done easily under linux like this.
Below I am sending an email to a local user |ishikawa| and only write From: and Subject: header lines.
```
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ /usr/sbin/sendmail ishikawa
From: ishikawa
Subject: test without date header

test
.
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ 
```