mDatabase reset to null unexpectedly
Categories
(MailNews Core :: Networking: IMAP, defect, P2)
Tracking
(Not tracked)
People
(Reporter: ishikawa, Assigned: ishikawa, NeedInfo)
References
Details
Attachments
(2 files)
+++ This bug was initially created as a clone of Bug #617945 +++
I am creating a clone to take care of the issues where mDatabase was reset to null unexpectedly.
https://bugzilla.mozilla.org/show_bug.cgi?id=617945#c9
"We've had many cases now of where mDatabase mysteriously disappears. A local reference prevents the crash until we discover the root cause."
I think I hit upon a possible root cause. (There may be others.)
https://bugzilla.mozilla.org/show_bug.cgi?id=617945#c18
Namely, the call to UpdateNewMessages/UpdateSummaryTotal may set to |mDatabase| to null unexpectedly and we should save/restore |mDatabase| appropriately.
The proper fix would be save/restore mDatabase where appropriate. (There are legitimate cases where mDatabase may be initialied to null for good reason.)
Here is the first patch to take care of the one place where
save/restore did not happen. (There are about a dozen places where the save/restore probably is needed.)
I was afraid of side effect of fixing the bug. but I don't see much ill-effect with the patch proposed here. It seems that there are enough safety/sanity checks to deal with mDatabase being null (and that the processing where mDatabase was not null got tested very well before that, it seems).
https://hg.mozilla.org/try-comm-central/rev/3dc4a503d821cef7fef528b7a2b7516417dc8171
|
Assignee | |
Comment 1•4 years ago
|
||
This TRUE/FALSE checks of |mDatabase| during mochitest run on tree-comm-central server.
(This is in contrast to https://bugzilla.mozilla.org/show_bug.cgi?id=617945#c25 )
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ grep check_m /tmp/bt*.log | grep TRUE | cut --delimiter=" " --fields=13- | sort | uniq -c
114 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:1020
267 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:1117
204 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:1391
15 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:232
114 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:261
16 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:3866
391 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:387
392 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:393
392 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:399
141 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:408
515 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:497
16 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:5121
2 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:5153
8 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:858
1690 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:866
1289 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:952
67 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapMailFolder.cpp:578
333 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:1228
43380 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:236
1255 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:238
44635 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:247
513 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:266
1 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:286
1 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:319
162 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:327
390 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:364
391 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:388
9998 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:873
1022 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:878
34 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/news/src/nsNewsFolder.cpp:219
59 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/news/src/nsNewsFolder.cpp:558
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ grep check_m /tmp/bt*.log | grep FALSE | cut --delimiter=" " --fields=13- | sort | uniq -c
776 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:232
86 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:261
395 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:3866
1 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:387
34 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:408
141 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:866
27 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/imap/src/nsImapMailFolder.cpp:578
1542 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:236
287 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:238
287 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:247
162 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:266
1 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:364
1677 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:873
655 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:878
25 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/news/src/nsNewsFolder.cpp:219
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$
I don't see drastic changes in the # of TRUE/FALSE cases. They increase/decrease in some places.
|
|
Assignee | |
Comment 2•4 years ago
|
||
This is TRUE/FALSE checks of |mDatabase| during xpcshell test run locally.
(I need to find out how I can enable --verbose --sequential to |mach| on try-comm-central server during xpcshell test.)
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ grep check_m /FF-NEW/log1304-xpctest-mDatabase-check.txt | grep TRUE | cut --delimiter=" " --fields=5- | sort | uniq -c
834 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:1020
1649 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:1047
136 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:1117
36 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:1391
1 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:1440
1050 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:232
828 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:261
205 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:3866
36 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:387
36 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:393
36 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:399
141 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:408
2 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:4238
8 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:4254
2 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:4283
5 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:478
156 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:497
37 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:5121
17 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:5153
1 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:768
4046 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:858
2008 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:866
1031 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:952
3 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:1221
21 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2050
574 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2365
574 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2375
259 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2427
259 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2452
259 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2456
3 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2499
574 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2534
607 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2639
846 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2690
846 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2772
846 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2868
20 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:3489
4 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:3627
844 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:3967
846 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:4038
407 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:4262
200 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:4580
574 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:5677
56 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:5696
20169 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:578
73 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:6770
6 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:8278
19 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:8529
9 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:8544
1 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/jsaccount/src/JaMsgFolder.cpp:65
1433 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:1228
2 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:1673
38055 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:236
482 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:238
38537 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:247
128 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:266
5 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:286
4 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:2872
5 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:319
58 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:327
37 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:364
37 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:388
11732 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:873
2898 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:878
8 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp:1527
452 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp:219
479 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp:558
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ grep check_m /FF-NEW/log1304-xpctest-mDatabase-check.txt | grep FALSE | cut --delimiter=" " --fields=5- | sort | uniq -c
6022 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:232
386 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:261
2506 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:3866
38 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:408
655 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:866
1517 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:578
1 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/jsaccount/src/JaMsgFolder.cpp:32
1 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:1228
512 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:236
30 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:238
30 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:247
58 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:266
1 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:364
1 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:388
6486 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:873
3588 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:878
133 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp:219
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$
|
||
Updated•4 years ago
|
|
|
||
Comment 3•3 years ago
|
||
Is this ready to be reviewed and land?
Is it really an imap issue?
|
|
Assignee | |
Comment 4•3 years ago
|
||
Landing is yes.
I am not if this is purely related to an imap issue.
It is only that I had problems compiling TB of late locally.
-
Could not even run |mach build| successfully.
I figured out that I needed to add "--disable-bootstrap| to my mozconfig.
(Problem is that configure process invoked even AFTER |mach configure| because of some file timestamp issue.
I think I mentioned it somewhere in the bugzilla.)
Comparison of timestamp of config.status and .mozconfig.json is done and config.status is older than .mozconfig.json and configuration is invoked
automagically (which is unnecessary IMHO) even after |mach configure.|
I had no way of passing --disable-bootstrap as in |mach configure --disable-bootstrap| then.
|mach build --disable-bootstrap| fails since build does not understand --disable-bootstrap, etc. -
Finally I figured out the above and then I had a file that had unused variable issue.
I use GCC and set the strict compiler check and warnings are treated as errors.
I tried to figure out where and then I realize that the variables are referenced in trace macros. Obviously macros are turned into null operation and I was trying to figure out what is causing the macro not to expand to executable statements.
mozilla/third_party/libwebrtc/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc
Well, the switch to a different method of support/discussion early this year meant I was not sure where to report or discuss the above issue and thus took much longer to figure out.
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.
What venue or where is exactly the proper method to ask and/or discuss build issues these days?
(Sorry my patch submission is slow. I am so side-tracked by the bad handling of Covid-19 outbreak by the Japanese government which is holding a big sport event albeit strong opposition of its civilian population. I am working from home for the last few months basically but such caution seems to be a mere leaf in the strong wind when the government is ignoring all the cautions from the medical and scientific community. I know bugzilla is for technical discussion, but when a patch submission process is really interrupted by social issues, I have to mention it. I could use my spare hours to patch production but no, it has been used for discussion and posting with concerned people on SNS.)
|
|
Assignee | |
Comment 5•3 years ago
|
||
BTW, there could be a couple of places where mDatabase needed to be set to a valid value (or rather saved/restored as in the patch of this bugzilla entry) , but
not all the places (several) which I suspected we needed to do that actually need to that.
IIRC, there is at least a place where |mDatabase| is intentionally set to null and thus the special handling is not necessary there, for example.
|
|
Assignee | |
Comment 6•3 years ago
|
||
My plan is to just check if the patch here does change the behavior of message with no Date: line processing.
That is, if the line in the patch is triggered.
Unfortunately, the standard mochitest does not seem to have such a test that uses a message with no Date: header line (?).
I was going to see how such a message is handled before/after the patch.
But it took me a bit of time to re-create the qpop server and the like. (Actually, I think I thought davecot server's qpop feature for local testing. I have not seen the trace of it under my current linux installation.)
Testing TB with a real qpop3 server, etc. takes time. :-(
It has been on the back burner for a while.
diff --git a/mailnews/local/src/nsLocalMailFolder.cpp b/mailnews/local/src/nsLocalMailFolder.cpp
--- a/mailnews/local/src/nsLocalMailFolder.cpp
+++ b/mailnews/local/src/nsLocalMailFolder.cpp
@@ -231,18 +231,21 @@ NS_IMETHODIMP nsMsgLocalMailFolder::GetD
nsIMsgDatabase** aDatabase) {
NS_ENSURE_ARG(aDatabase);
if (m_parsingFolder) return NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE;
nsresult rv = NS_OK;
if (!mDatabase) {
rv = OpenDatabase();
if (mDatabase) {
+ // we save mDatabase since UpdateNewMessages() may set it to null.
+ nsCOMPtr<nsIMsgDatabase> database = mDatabase;
mDatabase->AddListener(this);
UpdateNewMessages();
+ mDatabase = database;
}
}
NS_IF_ADDREF(*aDatabase = mDatabase);
if (mDatabase) mDatabase->SetLastUseTime(PR_Now()); <==== ! THIS LINE
return rv;
}
// Makes sure the database is open and exists. If the database is out of date,
|
|
Assignee | |
Comment 8•3 years ago
|
||
Good to hear that.
|
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 9•3 years ago
•
|
||
(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$
|
||
Comment 10•3 years ago
|
||
|
|
||
Comment 11•3 years ago
|
||
On a more general note, I'd really like to clean up the mDatabase handling everywhere. I've got a few questions - if you have any answers I'd love to hear them! Failing that I'll be trying to answer them when next time I get a chance to dig down through the database&folder code.
- what does a null mDatabase mean? (no database at all? an database unloaded, to save memory? something else?)
- why do we do lazy database initialization? (via GetDatabase()).
- Is there any specific reason we shouldn't initialise the database upon folder creation, so folder functions can rely on having a valid mDatabase?
- what simplification can we do with GetDatabase()/GetDatabasewithoutReparse
From a quick look through, it seems like mDatabase is only ever nulled when the folder is being shut down/deleted.
(well, also for folder compaction and renaming and maybe a couple of other special operations).
My gut feeling is that the life cycle of the database object (mDatabase) should be managed by the folder, and almost always be non-null (except for special operations which are managed by the folder anyway). I think this would simplify a lot of code.
Worth tackling in a separate bug, you think?
|
|
Assignee | |
Comment 12•3 years ago
|
||
(In reply to Ben Campbell from comment #10)
Comment on attachment 9210583 [details] [diff] [review]
save/restore |mDatabase| before and after a call to UpdateNewMessages().Review of attachment 9210583 [details] [diff] [review]:
r+ because I can't see it hurting anything, and if it fixes something,
that's wonderful :-)
Is there a specific crash that can be replicated? If so, I'd be happy to try
writing a unit test for it.
I did try a test email without a Date: field, and it seemed to work fine
without the patch (I used dovecot-lda, which I just discovered - seems like
a good way to deliver test mail locally)::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +235,5 @@nsresult rv = NS_OK;
if (!mDatabase) {
rv = OpenDatabase();
if (mDatabase) {
// we save mDatabase since UpdateNewMessages() may set it to null.I've had a little dig down into UpdateNewMessages(), but it wasn't obvious
to me where there's a potential for mDatabase to be nulled.
It Calls SetHasNewMessages(), which in turn calls SetMRUTime(), which could
be a candidate I guess? (a few more layers down it ends up calling
GetDatabase()).
(There's also NotifyBoolPropertyChanged(), but I couldn't find any
OnFolderBoolPropertyChanged() handlers which actually do anything :-)Is there a specific place that nulls mDatabase that you're worried about?
Thank you, Ben, for review and r+.
I am not an expert on mDatabase thingy.
But I was frankly very surprised when James Kent, who was very knowledgeable about TB code six years ago had to say this.:
https://bugzilla.mozilla.org/show_bug.cgi?id=617945#c9
We've had many cases now of where mDatabase mysteriously disappears. A local reference prevents the crash until we discover the root cause.
He had previously found that if we copy a few dozen messages from a folder to another folder in certain order TB crashed
due to mDatabase being null. I had reported a case whereby I could crash TB by moving a few hundred messages to another folder or something like that, and J. Kent came up with a much simpler, shorter example which really shocked me.
And the only way he could fix TB back then was to add |if (mDatabase)|. That was indeed a surprise.
I was so taken aback with this situation and so this episode remained in my mind and
found an interesting comment about mDatabase may be reset to null.
Please see my comment https://bugzilla.mozilla.org/show_bug.cgi?id=617945#c18
This may answer your question in comment 11.
Basically this bugzilla tries to fix the issue for UpdateNewMessages() or at least tries to be theoretically correct although parts affected with null |mDatabase| may have all been protected by |if (mDatabase)|. I say the protection may be already ubiquitous because my checking of |mDatabase| value in comment
https://bugzilla.mozilla.org/show_bug.cgi?id=617945#c22 and the subsequent comments suggest that no drastic change of TRUE/FALSE if-branch exection is observed with this bugzilla's particular patch, and no new errors are introduced with this patch.
The questions remain about the usage of |UpdateSummaryTotals() | in about a dozen places.
https://bugzilla.mozilla.org/show_bug.cgi?id=617945#c18
Well, I am assuming the comment line about "// UpdateNewMessages/UpdateSummaryTotals can null mDatabase" being correct.
|
|
Assignee | |
Comment 13•3 years ago
|
||
As for the handling of message without Date: line, as Ben pointed out, the current release version of TB seems to handle this correctly.
Well, at least, in my limited test also.
|mDatabase| handling might have improved in the last several years, too.
TB code base is very brittle, indeed. The phrase, "Handle with care", comes to my mind.
|
|
Assignee | |
Comment 14•3 years ago
|
||
(In reply to Ben Campbell from comment #11)
...
Worth tackling in a separate bug, you think?
Yes. I had similar questions when I checked the code before. Why are there these layers of initialization?
There has to be a good reason for that many years ago, but it seems it is forgotten and may not hold any more?
(Like virtual folder handling? I don't use unified folder thingy, so I am not sure.)
|
|
Assignee | |
Comment 15•3 years ago
|
||
This may be another interesting observation.
https://bugzilla.mozilla.org/show_bug.cgi?id=1535993#c14
|
|
Assignee | |
Comment 16•3 years ago
|
||
I think we can forget about saving/restoring |mDatabase| around the calls to |UpdateSummaryTotals()| in the original Bug 617945 for now.
See
https://bugzilla.mozilla.org/show_bug.cgi?id=617945#c31
The code might have mutated enough to render the |UpdateSummaryTotals ()| part of the original comment invalid to some extent.
|
|
||
Comment 17•2 years ago
•
|
||
Ben, is this obsolete or subsquently covered in other bugs you are working on?
If it's still valid, It would be good to move this forward for greater stability, but I'd like to see it have a full run on nightly and a full run on beta. How do we feel about taking this immediately after the next merge which is July 26?
|
|
||
Comment 18•2 years ago
|
||
c/obsolute/obsolete/
|
|
||
Updated•8 months ago
|
|
|
||
Comment 19•8 months ago
|
||
(In reply to Wayne Mery (:wsmwk) from comment #17)
Ben, is this obsolete or subsquently covered in other bugs you are working on?
|
|
||
Updated•1 month ago
|
|
|
Assignee | |
Comment 20•19 days ago
|
||
(In reply to Wayne Mery (:wsmwk) from comment #19)
(In reply to Wayne Mery (:wsmwk) from comment #17)
Ben, is this obsolete or subsquently covered in other bugs you are working on?
Interesting question. I am tied up with the deadline of a few documents tomorrow and so let me looks this over
after that and hopefully, I will create a phabricator patch.
I would say, please do not hold your breath.
I am still using hg MQ extension and hg evolve in a mixed manner (I have created a separate copy of the source tree for phabricator patch submission AND another copy for local MQ usage. Not ideal, but it works until I get to know the EVOLVE and other niceties of HG and the amount of local patches diminishes.
But this patch is very small, and it should not take DAYS to create the phabricator patch and run it through try-comm-central.
I am not clearing the needinfo request until it gets done.
Stay tuned.
|
|
Assignee | |
Comment 21•3 days ago
|
||
Description
•