617945 - crash [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)]

Reporter

Description

•

14 years ago

crash [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] bp-3774dee8-e6f0-4343-a314-67e322101129 EXCEPTION_ACCESS_VIOLATION_READ 0x0 0 thunderbird.exe nsImapMailFolder::DeleteMessages mailnews/imap/src/nsImapMailFolder.cpp:2380 1 thunderbird.exe nsMsgDBView::DeleteMessages mailnews/base/src/nsMsgDBView.cpp:3002 2 thunderbird.exe nsMsgDBView::ApplyCommandToIndices mailnews/base/src/nsMsgDBView.cpp:2726 3 thunderbird.exe nsMsgDBView::DoCommand mailnews/base/src/nsMsgDBView.cpp:2393 4 xpcom_core.dll NS_InvokeByIndex_P xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102 5 thunderbird.exe XPCWrappedNative::CallMethod js/src/xpconnect/src/xpcwrappednative.cpp:2722 slightly didferent stack - bp-7cf2173d-e299-4675-a8c7-128bd2101201

Wayne Mery (:wsmwk)

Reporter

Updated

•

14 years ago

OS: Windows Vista → All

Nobody; OK to take it and work on it

Updated

•

13 years ago

Crash Signature: [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)]

Wayne Mery (:wsmwk)

Reporter

Comment 1

•

13 years ago

bp-74ed0632-02d6-4797-9acb-efedd2111122 v8 this may phave morhed to nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) eg bp-a75f5c71-2db0-4d8e-bc8a-8e9182120329 v11

Crash Signature: [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] → [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) ]

Wayne Mery (:wsmwk)

Reporter

Comment 2

•

12 years ago

two different stacks bp-e9488400-7709-4746-a61e-dd9e72121225 0 xul.dll nsImapMailFolder::DeleteMessages mailnews/imap/src/nsImapMailFolder.cpp:2324 1 xul.dll nsMsgDBView::DeleteMessages mailnews/base/src/nsMsgDBView.cpp:3186 2 xul.dll nsMsgDBView::ApplyCommandToIndices mailnews/base/src/nsMsgDBView.cpp:2919 3 xul.dll nsMsgDBView::DoCommand mailnews/base/src/nsMsgDBView.cpp:2590 4 xul.dll NS_InvokeByIndex_P xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70 5 xul.dll XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:2406 6 xul.dll XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1478 7 mozjs.dll js::InvokeKernel js/src/jsinterp.cpp:352 bp-f764ceb6-a861-48e1-a858-17d412121204 0 xul.dll nsImapMailFolder::DeleteMessages mailnews/imap/src/nsImapMailFolder.cpp:2324 1 xul.dll nsMsgDatabase::ApplyRetentionSettings mailnews/db/msgdb/src/nsMsgDatabase.cpp:5224 2 xul.dll nsMsgDBFolder::ApplyRetentionSettings mailnews/base/util/nsMsgDBFolder.cpp:4786 3 xul.dll nsMsgDBFolder::ApplyRetentionSettings mailnews/base/util/nsMsgDBFolder.cpp:4762 4 xul.dll nsImapMailFolder::ApplyRetentionSettings mailnews/imap/src/nsImapMailFolder.cpp:1281 5 xul.dll nsImapMailFolder::Compact mailnews/imap/src/nsImapMailFolder.cpp:1296 6 xul.dll hashCleanupOnExit mailnews/base/src/nsMsgAccountManager.cpp:1005 7 xul.dll nsBaseHashtable<nsCStringHashKey,__int64,__int64>::s_EnumStub objdir-tb/mozilla/dist/include/nsBaseHashtable.h:419 8 xul.dll PL_DHashTableEnumerate objdir-tb/mozilla/xpcom/build/pldhash.cpp:715 9 xul.dll nsBaseHashtable<nsCStringHashKey,nsAutoPtr<mozilla::scache::CacheEntry>,mozilla: objdir-tb/mozilla/dist/include/nsBaseHashtable.h:223 10 xul.dll nsMsgAccountManager::CleanupOnExit mailnews/base/src/nsMsgAccountManager.cpp:1624

Makoto Kato [:m_kato]

Comment 3

•

12 years ago

mDatabase is nullptr, so mDatabase is gone after checking it.

Magnus Melin [:mkmelin]

Comment 4

•

11 years ago

bp-87165c55-e94d-47ea-bae5-71e7b2131220 - http://hg.mozilla.org/releases/comm-esr24/annotate/a5e50e44b4c4/mailnews/imap/src/nsImapMailFolder.cpp#l2278 but the question is why mDatabase becomes null after it's checked

ISHIKAWA, Chiaki

Comment 5

•

11 years ago

I can only offer wild guess. The routines invoked after |if (mDatabase)| may not change this directly, but are there other threads that may be touching this |mDatabase| variable? nsMsgDB*.cpp files seem to refer to this variable, and I wonder if there are any asynchronous access to this variable. In the second stack in comment 2, I see |nsImapMailFolder::Compact|, maybe |Compact| for IMAP may act slightly differently from |Compact| for POP3, but |Compact| for POP3 does seem to act asynchronously with the rest of POP3-style message operation. I have to wait for the lock (held by Compact) to be freed before I can touch a folder, etc. So maybe a protection on |mDatabase| variable is missing somewhere against such thread-race? For example, shouldn't the code starting from http://mxr.mozilla.org/comm-esr24/source/mailnews/imap/src/nsImapMailFolder.cpp#2264 and end with the usage of |mDatabase->...| several lines below be within some type of mutex/semaphore/lock? (And places that mucks with mDatabase shall properly honour such mutex/semaphore/lock?) TIA

Wayne Mery (:wsmwk)

Reporter

Comment 7

•

9 years ago

(In reply to Magnus Melin from comment #4) > bp-87165c55-e94d-47ea-bae5-71e7b2131220 - > http://hg.mozilla.org/releases/comm-esr24/annotate/a5e50e44b4c4/mailnews/ > imap/src/nsImapMailFolder.cpp#l2278 but the question is why mDatabase > becomes null after it's checked Yeah, compact could surely be one possible cause. What do you think we can do out of comment 5?

Flags: needinfo?(rkent)

Ludovic Hirlimann [:Usul]

Comment 8

•

9 years ago

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

BMO Automation

Updated

•

9 years ago

Crash Signature: [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) ] → [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, int, int, nsIMsgCopyServiceListener*, int)] [@ nsImapMailFolder::DeleteMessages(nsIArray*, nsIMsgWindow*, bool, bool, nsIMsgCopyServiceListener*, bool) ] [@ nsImapMailFolder::DeleteMessages]

Kent James (:rkent)

Assignee

Comment 9

•

9 years ago

Attached patch Use a local reference for the db (deleted) — Details — Splinter Review

We've had many cases now of where mDatabase mysteriously disappears. A local reference prevents the crash until we discover the root cause.

Assignee: nobody → rkent

Status: NEW → ASSIGNED

Flags: needinfo?(rkent)

Attachment #8729707 - Flags: review?(mkmelin+mozilla)

Magnus Melin [:mkmelin]

Comment 10

•

9 years ago

Comment on attachment 8729707 [details] [diff] [review] Use a local reference for the db Review of attachment 8729707 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! r=mkmelin

Attachment #8729707 - Flags: review?(mkmelin+mozilla) → review+

Kent James (:rkent)

Assignee

Comment 11

•

9 years ago

Comment on attachment 8729707 [details] [diff] [review] Use a local reference for the db [Triage Comment] [Triage Comment] http://hg.mozilla.org/releases/comm-aurora/rev/b56c25ae4748 http://hg.mozilla.org/releases/comm-beta/rev/c45514a56b28

Attachment #8729707 - Flags: approval-comm-beta+

Attachment #8729707 - Flags: approval-comm-aurora+

Kent James (:rkent)

Assignee

Updated

•

9 years ago

status-thunderbird45: --- → affected

status-thunderbird46: --- → fixed

status-thunderbird47: --- → fixed

status-thunderbird48: --- → affected

Whiteboard: [checkin-needed comm-central]

Kent James (:rkent)

Assignee

Comment 12

•

9 years ago

Comment on attachment 8729707 [details] [diff] [review] Use a local reference for the db [Approval Request Comment] Regression caused by (bug #): User impact if declined: Testing completed (on c-c, etc.): Risk to taking this patch (and alternatives if risky):

Attachment #8729707 - Flags: approval-comm-esr45?

Kent James (:rkent)

Assignee

Comment 13

•

9 years ago

Comment on attachment 8729707 [details] [diff] [review] Use a local reference for the db http://hg.mozilla.org/releases/comm-esr45/rev/47b22519b5a2

Attachment #8729707 - Flags: approval-comm-esr45? → approval-comm-esr45+

Kent James (:rkent)

Assignee

Updated

•

9 years ago

status-thunderbird45: affected → ---

status-thunderbird_esr45: --- → fixed

Keywords: checkin-needed

Whiteboard: [checkin-needed comm-central]

ISHIKAWA, Chiaki

Comment 14

•

9 years ago

(In reply to Magnus Melin from comment #4) > bp-87165c55-e94d-47ea-bae5-71e7b2131220 - > http://hg.mozilla.org/releases/comm-esr24/annotate/a5e50e44b4c4/mailnews/ > imap/src/nsImapMailFolder.cpp#l2278 but the question is why mDatabase > becomes null after it's checked I found an interesting comment! http://mxr.mozilla.org/comm-esr24/source/mailnews/imap/src/nsImapMailFolder.cpp#608 608 // UpdateNewMessages/UpdateSummaryTotals can null mDatabase, so we save a local copy 609 nsCOMPtr<nsIMsgDatabase> database(mDatabase); 610 UpdateNewMessages(); 611 if(mAddListener) 612 database->AddListener(this); 613 UpdateSummaryTotals(true); 614 mDatabase = database; This means that we may want to save mDatabase and reuse it when the NULL reference happened!?

ISHIKAWA, Chiaki

Comment 15

•

9 years ago

http://hg.mozilla.org/releases/comm-esr24/diff/e0aafab0f5da/mailnews/imap/src/nsImapMailFolder.cpp#l649 This is more clear about the issue. We need to change the code ala the patch above, at least!

Jorg K (CEST = GMT+2)

Comment 16

•

9 years ago

https://hg.mozilla.org/comm-central/rev/eb9570adfe51

Status: ASSIGNED → RESOLVED

Closed: 9 years ago

status-thunderbird48: affected → fixed

Keywords: checkin-needed

Resolution: --- → FIXED

Jorg K (CEST = GMT+2)

Updated

•

9 years ago

Target Milestone: --- → Thunderbird 48.0

Wayne Mery (:wsmwk)

Reporter

Comment 17

•

4 years ago

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

http://hg.mozilla.org/releases/comm-esr24/diff/e0aafab0f5da/mailnews/imap/
src/nsImapMailFolder.cpp#l649

This is more clear about the issue.
We need to change the code ala the patch above, at least!

Did we complete this, and also address comment 14, to your satisfaction?

Flags: needinfo?(ishikawa)

ISHIKAWA, Chiaki

Comment 18

•

4 years ago

I re-read the thread:
My comment in comment 15 was to find the answer to the following questions:

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

bp-87165c55-e94d-47ea-bae5-71e7b2131220 -
http://hg.mozilla.org/releases/comm-esr24/annotate/a5e50e44b4c4/mailnews/
imap/src/nsImapMailFolder.cpp#l2278 but the question is why mDatabase
becomes null after it's checked

(In reply to Kent James (:rkent) from comment #9)

We've had many cases now of where mDatabase mysteriously disappears. A local
reference prevents the crash until we discover the root cause.

http://hg.mozilla.org/releases/comm-esr24/diff/e0aafab0f5da/mailnews/imap/src/nsImapMailFolder.cpp#l649
had a patch with a comment.
as follows.

// UpdateNewMessages/UpdateSummaryTotals can null mDatabase, so we save a local copy

So any use of mDatabase (after mDatabase was verified to be non-null) needs to be saved and restored if the subsequent processing somehow
calls UpdateNewMessages() or UpdateSummaryTotals() before mDatabase is accessed again.

So I checked the usage of UpdateNewMessages() and UpdateSummaryTotals().

First UpdateNewMessages calls.
https://searchfox.org/comm-central/search?q=UpdateNewMessages&path=

There are three cases. One of them seems to be bad.

▼
Textual Occurrences
	mailnews/base/src/nsMsgDBFolder.cpp
508	void nsMsgDBFolder::UpdateNewMessages() {
	mailnews/base/src/nsMsgDBFolder.h
147	void UpdateNewMessages();
	mailnews/imap/src/nsImapMailFolder.cpp
590	// UpdateNewMessages/UpdateSummaryTotals can null mDatabase, so we save a
593	UpdateNewMessages();
	mailnews/jsaccount/src/JaMsgFolder.cpp
70	// UpdateNewMessages();
	mailnews/local/src/nsLocalMailFolder.cpp
240	UpdateNewMessages();

(1) First one.
https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#593
This is the one I mentioned in comment 14 (the line number has changed, though) and
mDatabase is saved/restored. OK.

590	// UpdateNewMessages/UpdateSummaryTotals can null mDatabase, so we save a
593	UpdateNewMessages();

(2) The second one.
https://searchfox.org/comm-central/source/mailnews/jsaccount/src/JaMsgFolder.cpp#70
This is interesting.
It does not call UpdateNewMessages() now, but mentions that UpdateSummaryTotals () can nullify mDatabase and it saves/restores it.
OK.


    if (mDatabase) {
      //
      // When I inadvertently deleted the out-of-date database, I hit this code
      // with the db's m_dbFolderInfo as null from the delete, yet the local
      // mDatabase reference kept the database alive. So I hit an assert when I
      // tried to open the database. Be careful if you try to fix the
      // out-of-date issues!
      //
      // UpdateNewMessages();
      if (mAddListener) mDatabase->AddListener(this);
      // UpdateSummaryTotals can null mDatabase during initialization, so we
      // save a local copy
      nsCOMPtr<nsIMsgDatabase> database(mDatabase);
      UpdateSummaryTotals(true);
      mDatabase = database;
    }
  }

(3) Ths IS buggy, I am afraid.

  if (!mDatabase) {
    rv = OpenDatabase();
    if (mDatabase) {
      mDatabase->AddListener(this);
      UpdateNewMessages();
    }
  }
  NS_IF_ADDREF(*aDatabase = mDatabase);
  if (mDatabase) mDatabase->SetLastUseTime(PR_Now());
  return rv;

See, after UpdateNewMessages(), mDabase can be set to null.
This means the subsequent processing of SetLastUseTime() won't happen. Ouch.

What about UpdateSummaryTotals() use?

https://searchfox.org/comm-central/search?q=+UpdateSummaryTotals&path=

OH MY GOODNESS.

There are about a dozen usage of UpdateSumamryTotalsI(). Many of them are BAD (!)

I marked the occurrences where save/restore does not happen by "?" and occurrences where proper save/restore happens by "x".

In a case or two, they may be legitimate, but others are dubious, VERY dubious.

	mailnews/base/public/nsIMsgFolder.idl
285	void updateSummaryTotals(in boolean force);
	mailnews/base/src/nsMsgDBFolder.cpp
? 395	 UpdateSummaryTotals(true);  ---> maybe a required processing does not happen since mDatabase is null after this call(!)
? 932	 UpdateSummaryTotals(true);
? 994	 UpdateSummaryTotals(true);
? 3741	 UpdateSummaryTotals(false);
? 4513	 UpdateSummaryTotals(true);
	mailnews/imap/src/nsImapMailFolder.cpp
? 567	 UpdateSummaryTotals(false);
x 595	 UpdateSummaryTotals(true);  <--- This is the case already discussed. Saves/Restores mDatabase. Good.
	mailnews/imap/src/nsImapMailFolder.h
255	NS_IMETHOD UpdateSummaryTotals(bool force) override;
	mailnews/jsaccount/src/JaMsgFolder.cpp
x 72	// UpdateSummaryTotals can null mDatabase during initialization, so we
x 75	 UpdateSummaryTotals(true);   <--- This is the case already discussed. SAves/REstores mDatabase. Good
	mailnews/local/src/nsLocalMailFolder.cpp
? 218	 UpdateSummaryTotals(false);
? 344	 UpdateSummaryTotals(true);
? 848	 UpdateSummaryTotals(true); <--- This seems to be part of a new database opening, and thus mDatabase is supposed to be recreated anyay. Somebody has to check this. It is explicitly set to null and then resets with
        mDatabase = nullptr;
        db = nullptr;
        rv = msgDBService->OpenFolderDB(this, false, getter_AddRefs(mDatabase));

? 2892	 UpdateSummaryTotals(true);
	mailnews/news/src/nsNewsFolder.cpp
? 238	rv = UpdateSummaryTotals(true);  <-- this one seems to be bad in that it recreates a new database opening a folder, but it blows it away by
              this UpdateSummaryTotals(). Ouch.

VERY BAD IMHO.

Somebody's got to take a look at this!
One bad case for UpdateNewMessages(),
and many for UpdateSummaryTotalsUsage().

Flags: needinfo?(ishikawa) → needinfo?(vseerror)

Wayne Mery (:wsmwk)

Reporter

Comment 19

•

4 years ago

Thanks for taking another look at it.

Flags: needinfo?(vseerror) → needinfo?(mkmelin+mozilla)

Magnus Melin [:mkmelin]

Comment 20

•

4 years ago

You might be right it's (theoretically) buggy, but I'm not sure it has a big impact in real usage.

Flags: needinfo?(mkmelin+mozilla)

ISHIKAWA, Chiaki

Comment 21

•

4 years ago

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

You might be right it's (theoretically) buggy, but I'm not sure it has a big impact in real usage.

I have no idea myself. I will put in a dump statement to see how often this part of code gets executed during
mochitest and xpcshell tests and also try to save/restore mDatabase if necessary.
At least then we will know why we need to protect for supposedly non-null mDatabase abnormal condition of being null in many parts of code base.

ISHIKAWA, Chiaki

Updated

•

4 years ago

Flags: needinfo?(ishikawa)

ISHIKAWA, Chiaki

Comment 22

•

4 years ago

I have figured there are about 70+ places where mDatabase nullness is checked in TB code.

$ find mail mailnews common chat calendar -type f -print | xargs grep "if[ ]*([ \!]*mDatabase[ ]*)" | grep -v .orig | wc
     71     294    4981

Like these:

ailnews/base/src/nsMsgDBFolder.cpp:  if (mDatabase) {
mailnews/base/src/nsMsgDBFolder.cpp:  if (mDatabase) {
mailnews/base/src/nsMsgDBFolder.cpp:  if (mDatabase) mDatabase->RemoveListener(this);
mailnews/base/src/nsMsgDBFolder.cpp:  if (mDatabase) mDatabase->AddListener(this);
mailnews/base/src/nsMsgDBFolder.cpp:  if (mDatabase) m_newMsgs.Clear();
mailnews/base/src/nsMsgDBFolder.cpp:  if (mDatabase) {
mailnews/base/src/nsMsgDBFolder.cpp:  if (!mDatabase) return NS_ERROR_FAILURE;
mailnews/base/src/nsMsgDBFolder.cpp:  if (mDatabase) {
mailnews/base/src/nsMsgDBFolder.cpp:    else if (mDatabase)
mailnews/base/src/nsMsgDBFolder.cpp:  if (!mDatabase) return NS_ERROR_FAILURE;
mailnews/base/src/nsMsgDBFolder.cpp:  if (mDatabase) {

I think I am going to change the appearance mDatabase into mDatabasecheck(mDatabase, FILE, LINE) and prints the null, non-null state and summarize the output.

THEN, I will fix the bad places mentioned in comment 18 and see if the
output summary would change (I think it would).
It would be interesting to learn how many new errors will be introduced by possibly now being able to run processing formerly proteced by if(mDatabase) now being executed by slightly more correct underlying code.

ISHIKAWA, Chiaki

Comment 23

•

4 years ago

This is the TRUE/FALSE result of mDatabase at the following lines (there are !mDatabase, but I am only reporting whether mDatabase is null or not.)

TRUE for xpctest

    838 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
    137 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
    832 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:261
    204 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
    150 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
   4047 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:858
   2005 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:866
   1092 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:2617
    846 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2668
    846 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2750
    846 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:2846
     20 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:3467
      4 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:3605
    844 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:3945
    846 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:4016
    407 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:4240
    200 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:4558
    574 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:5655
     57 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:5674
  20172 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:578
     74 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:6748
      6 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:8256
     19 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:8507
      9 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/imap/src/nsImapMailFolder.cpp:8522
      1 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/jsaccount/src/JaMsgFolder.cpp:65
   1437 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:1225
      2 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:1670
  38220 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:236
    485 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:238
  38705 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:244
    129 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:263
      6 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:283
      5 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:2869
      6 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:316
     58 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:324
     37 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:361
     37 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:385
  11765 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:870
   2898 TRUE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:875
      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

---

FALSE xpcshell test
   6019 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:232
    385 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp:261
   2500 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
   1515 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:1225
    516 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:236
     31 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:238
     31 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:244
     58 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:263
      1 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:361
      1 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:385
   6472 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:870
   3574 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:875
    133 FALSE, /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp:219

With the correction I mentioned in comment 18, the FALSE cases should go down, and we may see strange errors that are triggered by the now more correct behavior of underlying data structure.

Flags: needinfo?(ishikawa)

ISHIKAWA, Chiaki

Updated

•

4 years ago

Flags: needinfo?(ishikawa)

ISHIKAWA, Chiaki

Comment 24

•

4 years ago

These are the numbers from mochitest.

grep check_m /media/sf_download/bct*.log  | grep FALSE | wc
   6146   61460 1332008
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ grep check_m /media/sf_download/bct*.log  | grep TRUE | wc
 108324 1083240 23468087
ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$

So mDatabase being null is about 1/16th of the non-null cases, but they ARE there.
From:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=8b5c84053a12524b0e2b0013f71823d9e70ebd0a

I will try to reduce the null cases by installing individual patches for the cases in comment 18 one by one.

ISHIKAWA, Chiaki

Comment 25

•

4 years ago

Detailed breakdown for mochitest.


From mochitest

ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ grep check_m /media/sf_download/bct*.log  | grep TRUE | cut --delimiter=" " --fields=13- | sort | uniq -c
    113 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
    113 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
   1688 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:866
   1288 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:1225
  43644 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:236
   1250 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:238
  44894 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:244
    514 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:263
      1 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:283
      1 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:316
    160 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:324
    390 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:361
    391 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:385
  10000 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:870
   1025 TRUE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:875
     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$

ishikawa@ip030:/NREF-COMM-CENTRAL/mozilla$ grep check_m /media/sf_download/bct*.log  | grep FALSE | cut --delimiter=" " --fields=13- | sort | uniq -c
    824 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:232
     87 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:261
    397 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
    140 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
   1537 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:244
    160 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:263
      1 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:361
   1682 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:870
    657 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/local/src/nsLocalMailFolder.cpp:875
     25 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/news/src/nsNewsFolder.cpp:219

ISHIKAWA, Chiaki

Comment 26

•

4 years ago

Interesting.
SetMsgDatabase(nullptr) is used to nullify mDatabase with commit, etc. if necessary.
It is never called with non-null pointer. Maybe we can rename the function to ClearDatabase or something.

https://searchfox.org/comm-central/search?q=SetMsgDatabase&path=

Flags: needinfo?(ishikawa)

ISHIKAWA, Chiaki

Comment 27

•

4 years ago

GetMsgDatabase seems to be the one that assigns valid database pointer to mDatabase.

https://searchfox.org/comm-central/search?q=GetMsgDatabase

Ugh. I hate to see the missing error checks of some lines.

ISHIKAWA, Chiaki

Comment 28

•

4 years ago

The patches I created to monitor TRUE/FALSE conditions doe not cover the cases where |mDatabase| is used in ? expression.
(I don't know if such use cases exist or not.)
So it is not complete, but gives us a fairly good view of how TB fares.
I already have a feeling that TB may not be flushing internal message database when the following happens.

    397 FALSE, /builds/worker/checkouts/gecko/comm/mailnews/base/src/nsMsgDBFolder.cpp:3866

https://searchfox.org/comm-central/source/mailnews/base/src/nsMsgDBFolder.cpp#3866

Let me see if the patches I will produce in the next few days turn the condition to TRUE in more cases. If it does, then it means we have lost proper flushing of internal database to the external store before.

Stay tuned.

Flags: needinfo?(ishikawa)

ISHIKAWA, Chiaki

Updated

•

4 years ago

Blocks: 1699968

ISHIKAWA, Chiaki

Comment 29

•

4 years ago

I have created a bugzilla to take care of the first patch to save/resotre |mDatabse| before and after a call to UpdateNewMessages();
bug 1699968.

I am going to file other patches for the save/retstore before and after calls to updateSummaryTotals().

Wayne Mery (:wsmwk)

Reporter

Updated

•

4 years ago

No longer blocks: 1699968

See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1699968

ISHIKAWA, Chiaki

Updated

•

4 years ago

See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1535993

ISHIKAWA, Chiaki

Comment 30

•

3 years ago

(In reply to ISHIKAWA, Chiaki from comment #18)
....

What about UpdateSummaryTotals() use?

https://searchfox.org/comm-central/search?q=+UpdateSummaryTotals&path=

OH MY GOODNESS.

There are about a dozen usage of UpdateSumamryTotalsI(). Many of them are BAD (!)

I marked the occurrences where save/restore does not happen by "?" and occurrences where proper save/restore happens by "x".

In a case or two, they may be legitimate, but others are dubious, VERY dubious.

	mailnews/base/public/nsIMsgFolder.idl
285	void updateSummaryTotals(in boolean force);
	mailnews/base/src/nsMsgDBFolder.cpp
? 395	 UpdateSummaryTotals(true);  ---> maybe a required processing does not happen since mDatabase is null after this call(!)
? 932	 UpdateSummaryTotals(true);
? 994	 UpdateSummaryTotals(true);
? 3741	 UpdateSummaryTotals(false);
? 4513	 UpdateSummaryTotals(true);
	mailnews/imap/src/nsImapMailFolder.cpp
? 567	 UpdateSummaryTotals(false);
x 595	 UpdateSummaryTotals(true);  <--- This is the case already discussed. Saves/Restores mDatabase. Good.
	mailnews/imap/src/nsImapMailFolder.h
255	NS_IMETHOD UpdateSummaryTotals(bool force) override;
	mailnews/jsaccount/src/JaMsgFolder.cpp
x 72	// UpdateSummaryTotals can null mDatabase during initialization, so we
x 75	 UpdateSummaryTotals(true);   <--- This is the case already discussed. SAves/REstores mDatabase. Good
	mailnews/local/src/nsLocalMailFolder.cpp
? 218	 UpdateSummaryTotals(false);
? 344	 UpdateSummaryTotals(true);
? 848	 UpdateSummaryTotals(true); <--- This seems to be part of a new database opening, and thus mDatabase is supposed to be recreated anyay. Somebody has to check this. It is explicitly set to null and then resets with
        mDatabase = nullptr;
        db = nullptr;
        rv = msgDBService->OpenFolderDB(this, false, getter_AddRefs(mDatabase));

? 2892	 UpdateSummaryTotals(true);
	mailnews/news/src/nsNewsFolder.cpp
? 238	rv = UpdateSummaryTotals(true);  <-- this one seems to be bad in that it recreates a new database opening a folder, but it blows it away by
              this UpdateSummaryTotals(). Ouch.

VERY BAD IMHO.

Somebody's got to take a look at this!
One bad case for UpdateNewMessages(),
and many for UpdateSummaryTotalsUsage().

I checked whether save/restore is required around |UpdateSummaryTotals()| calls.
The answer is tentative NO.
More on this in the next comment.

Flags: needinfo?(ishikawa)

ISHIKAWA, Chiaki

Comment 31

•

3 years ago

Attached file Trying to save/restore mDatabase before/after |UpdateSummaryTotals()| (deleted) — Details

Hi,

The original comment about

// save mDatabase since UpdateSummaryTotals() may nullify mDatabase
// during initialization so we save a local copy

are maybe only applicable during initialization indeed, and
saving and restoring |mDatabase| before and after |UpdateSummaryTotals()|
wholesale for mochitest and xpcshell test
IS harmful.

This is becase I found out that there are cases where mDatabase was null
upon entry to UpDateSummaryTotals(), but mDatabase becomes non-null after the call..
So saving / restoring mDatabase without discrimation IS INCORRECT.

OTOH, there are cases where |mDatabase| was null upon entry to UpdateSummaryTotals() and
remains null after the call. These are problematic cases.

I checked this with the patch attached.

I am quoting the null -> non-null transition case below for mochitest and xpcshell tests in the following.
I will post the null -> null transition cases for both tests in the next attachment

So the QUESTION WE PROBABLY SHOULD BE ASKING is WHY mDatabase remains null after the call to
upDateSummary Totals().

Beware that the test coverage of code by mochitest and xpcshell tests are NOT that great.
I am afraid that the code coverage is like 40% (?).
(This is my educated guess based on the bugs we find although TB is deemed to have passed mochitest and xpcshell tests before shipment.)
So there MAY be (or MAY HAVE BEEN) cases when mDatabase became null from non-null value after the call to UpdateSummaryTotals().

However, given that many affected places are protected by |if (mDatabase)|,
maybe we may not have to bother with saving / restoring |mDatabase| around the call to UpdateSummaryTotals() for now,
OR we can simply change the restoring code in the patch with

      if(!mDatabase) mDatabase = ldatabase;

null -> non-null caes.

mochitest

108:38.70 GECKO(73098) {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp,4691) mDatabase = 0x5595f2d797d0, ldatabase = (nil)
108:43.99 GECKO(73098) {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/base/src/nsMsgDBFolder.cpp,4691) mDatabase = 0x5595f2d797d0, ldatabase = (nil)

xpcshell test

 0:22.06 pid:96917 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp,2996) mDatabase = 0x5612e52f2f50, ldatabase = (nil)
 0:12.08 pid:98236 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x564d4c3e51f0, ldatabase = (nil)
 0:00.90 pid:107646 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x56497e4c3180, ldatabase = (nil)
 0:01.46 pid:107676 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x5566dbe448c0, ldatabase = (nil)
 0:01.92 pid:107706 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x561c412fd4a0, ldatabase = (nil)
 0:02.48 pid:107737 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55b9d3ac14e0, ldatabase = (nil)
 0:03.00 pid:107768 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55d5725c98e0, ldatabase = (nil)
 0:03.57 pid:107798 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55a650e62220, ldatabase = (nil)
 0:04.14 pid:107830 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55a6a3669a90, ldatabase = (nil)
 0:04.69 pid:107860 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x559f7cfc9db0, ldatabase = (nil)
 0:05.25 pid:107890 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55ce72893aa0, ldatabase = (nil)
 0:05.84 pid:107920 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x56456aa764e0, ldatabase = (nil)
 0:06.37 pid:107945 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55bff821fe70, ldatabase = (nil)
 0:07.01 pid:107976 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x5601c9907f00, ldatabase = (nil)
 0:07.59 pid:108006 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x564938d75c70, ldatabase = (nil)
 0:08.11 pid:108036 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55ccc5ffddc0, ldatabase = (nil)
 0:08.63 pid:108061 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55bb0b564280, ldatabase = (nil)
 0:09.28 pid:108091 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55ec21ac90c0, ldatabase = (nil)
 0:10.71 pid:108143 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x558f0f520f00, ldatabase = (nil)
 0:11.56 pid:108195 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55eb3e7c2a40, ldatabase = (nil)
 0:12.12 pid:108228 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55831e98cc80, ldatabase = (nil)
 0:12.69 pid:108253 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x5632ddf91a60, ldatabase = (nil)
 0:13.85 pid:108313 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x5559a64814a0, ldatabase = (nil)
 0:14.51 pid:108344 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55911d3f1a70, ldatabase = (nil)
 0:15.16 pid:108375 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x561d40ee57a0, ldatabase = (nil)
 0:15.81 pid:108405 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55c95c7184e0, ldatabase = (nil)
 0:16.48 pid:108436 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x563c6b8c9c10, ldatabase = (nil)
 0:17.16 pid:108466 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x5575d3ac85a0, ldatabase = (nil)
 0:17.81 pid:108496 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x563e6b56b210, ldatabase = (nil)
 0:18.48 pid:108526 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55aa48036210, ldatabase = (nil)
 0:19.27 pid:108551 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x562f5c2c2660, ldatabase = (nil)
 0:19.89 pid:108581 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x560e789f6ec0, ldatabase = (nil)
 0:20.50 pid:108611 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55593bc86850, ldatabase = (nil)
 0:20.98 pid:108641 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x5632afae47a0, ldatabase = (nil)
 0:21.50 pid:108666 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x5621792eb260, ldatabase = (nil)
 0:22.30 pid:108695 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55c1aa226e00, ldatabase = (nil)
 0:23.72 pid:108746 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55a832377b60, ldatabase = (nil)
 0:24.83 pid:108800 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x55b5ef120290, ldatabase = (nil)
 0:25.42 pid:108831 {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNewsFolder.cpp,218) mDatabase = 0x56463dfdcb60, ldatabase = (nil)

One reason why I felt maybe the test coverage is not that great is we see null -> non-null transition cases in nsNewsFolder.cpp. So there may be a special situation in news handling, but I felt we may not be covering similar cases for ordinary mail messages (?). I checked the code in nsNewsFolder.cpp and it seems that news articles are saved into cache(?) and read from there. This may not happen for mail messages, so maybe there is INDEED a special handling for news articles.

These are exceprted lines are picked up by a command line from respective local log.
grep "mDatabase =" log1341-xpcshell.txt .txt | grep ldatabase | gawk -f ~/Dropbox/TB-DIR/compare-mDatabase-ldatabase.awk

where compare-mDatabase-ldatabase.awk is

#  3:18.30 GECKO(1862871) {debug} (/NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp,222) mDatabase = (nil), ldatabase = (nil)

$5 ~ /mDatabase/ { mDatabase = substr($7, 1, length($7) - 1); }
$6 ~ /mDatabase/ { mDatabase = substr($8, 1, length($8) - 1); }

$8 ~ /ldatabase/ { ldatabase = $10;
                   if (substr(ldatabase, length(ldatabase) ) == "\r" ) {
                      ldatabase =substr(ldatabase, 1, length(ldatabase) - 1);
                      }
                }
$9 ~ /ldatabase/ { ldatabase = $11;
                   if (substr(ldatabase, length(ldatabase) ) == "\r" ) {
                      ldatabase =substr(ldatabase, 1, length(ldatabase) - 1);
                      }
                 }

{
        if (mDatabase != ldatabase) {
           print    $0;  
        }
}

ISHIKAWA, Chiaki

Comment 32

•

3 years ago

Attached file Cases when |mDatabase| remains null after the call to |UpdateSummaryTotals()|. (deleted) — Details

Excerpted lines from local log where the |mDatabase| remains null after the call to
|UpdateSummaryTotals()|.

Use a local reference for the db 9 years ago Kent James (:rkent) (deleted), patch	mkmelin : review+ rkent : approval-comm-aurora+ rkent : approval-comm-beta+ rkent : approval-comm-esr45+	Details \| Diff \| Splinter Review
Trying to save/restore mDatabase before/after \|UpdateSummaryTotals()\| 3 years ago ISHIKAWA, Chiaki (deleted), text/plain		Details
Cases when \|mDatabase\| remains null after the call to \|UpdateSummaryTotals()\|. 3 years ago ISHIKAWA, Chiaki (deleted), text/plain		Details