Bugzilla

Comment 4

•

11 years ago

The problem in a slightly different form.
Please see
https://bugzilla.mozilla.org/show_bug.cgi?id=634544#c11

And not only it does not create summary, but actually filtering fails
somehow : I am testing comm-central build20.0a1)

Also, this bug may also be related to 
Bug 497804 - If "filter move" is invoked while "compact folder" is running, both "compact folder" and "filter move" silently fails, and "blank thread pane/UI hang" occurs on "move target folder"

I am not sure if one of the bugzilla entries blocks the other, but
they seem to be part of a larger of interaction/synchronization of
message DB and filtering.

Status: RESOLVED → REOPENED

Ever confirmed: true

Resolution: EXPIRED → ---

Comment 5

•

11 years ago

Bug 493429 - Error: uncaught exception: [Exception... "Component returned failure code: 0x80550005 [nsIMsgFolder.msgDatabase]" nsresult: "0x80550005 (<unknown>)" location: "JS frame :: chrome://messenger/content/mailWidgets.xml :: parseFolder :: line 1951" data: no 

also discusses the out of date (or maybe completely missing) .msf file.

I do understand the outdated .msf causes many problems, but handling them
in an uniform manner is very difficult.
But to avoid user confusion, caused by SILENT FAILURE
of SOME OPERATIONS that THE USER WOULD HAVE EXPECTED THEM TO HAVE HAPPENED
short of kickstarting async rebuild, at least
we should RAISE A VISIBLE UI ERROR DIALOG to warn that "Certain operations
failed due to out-of-sync or missing .msf file, and please repair it
using repair button LATER MANUALLY" where it is possible.

Particular instance may add,
"Please run filter again after REPAIR", for example,
if the error occurs during POP3 download and filtering fails due to
out-of-sync db (currently SILENTLY).
This is the error case of
https://bugzilla.mozilla.org/show_bug.cgi?id=634544#c11

Oh well, maybe we should create a meta bugzilla entry for "out of sync message 
db" file,
and hang all these related bugzilla entries under it.

Phil Ringnalda (:philor)

Updated

•

11 years ago

Assignee: sspitzer → nobody

Comment 6

•

11 years ago

This is the analysis of why the error is not propagated for any visual
or audible feedback when the filtering fails silently due to a missing
.msf file. (Yes, the original mentions slightly different behavior, but
I think we are looking at a core issue from different angle.
Maybe completely missing .msf and out-of-date, yet existing .msf file
may lead to slightly changed appearance of the bug.)

The situation looks grim.

The symptom of not checking error code is rampant in the paths
related to the processing to filtering failure :-(


A low level routine prints warning (at least in debug build of TB) when it
fails to move an incoming e-mail that has been put into Inbox (I think. Not sure, it may be in a transient state) by a filter to a folder,
that has missing/outdated
.msf database file. The following line is printed during the run of
debug build of TB (20.0a1)

WARNING: failed to open mail db parsing folder: 'destMailDB && NS_SUCCEEDED(rv)', file /COMM-CENTRAL/comm-central/mailnews/local/src/nsParseMailbox.cpp, line 2540

(The line number may be a few lines off to the pristine comm-central
due to local changes for various patches.)

This is printed in a function called MoveIncorporatedMessage(), 

https://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2525

After this printing and a slightly obscure error processing
it returns NS_MSG_ERROR_WRITING_MAIL_FOLDER
at the end of function return.
(I think if the error is noticed in NS_WARN_IF_ERROR() the code should
jump right to the end of function error processing at
https://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2542

Anyway, MoveIncorporatedMessage() returns error code when the .msf is
missing, say.

What happens thereafter, and how the error is propagated?

Looking for the usage of MoveIncorporatedMessage
in comm-central I see the references as below.

I am not familiar with IMAP usage, and so I skip it for the moment.

---
Defined as a function prototype in:

    mailnews/imap/src/nsImapMailFolder.cpp (View Hg log or Hg annotations)
        line 3139 -- nsresult err = MoveIncorporatedMessage(newMsgHdr, mDatabase, trashUri, nullptr, msgWindow);
        line 3561 -- nsresult err = MoveIncorporatedMessage(msgHdr, mDatabase, actionTargetFolderUri, filter, msgWindow); 
    mailnews/imap/src/nsImapMailFolder.h (View Hg log or Hg annotations)
        line 324 -- nsresult MoveIncorporatedMessage(nsIMsgDBHdr *mailHdr, 
    mailnews/local/src/nsParseMailbox.h (View Hg log or Hg annotations)
        line 241 -- virtual nsresult MoveIncorporatedMessage(nsIMsgDBHdr *mailHdr, 

Referenced (in 2 files total) in:

    mailnews/imap/src/nsImapMailFolder.cpp (View Hg log or Hg annotations)
        line 4142 -- nsresult nsImapMailFolder::MoveIncorporatedMessage(nsIMsgDBHdr *mailHdr, 
    mailnews/local/src/nsParseMailbox.cpp (View Hg log or Hg annotations)
        line 1876 -- MoveIncorporatedMessage(m_newMsgHdr, m_mailDB, trash,
        line 2091 -- err = MoveIncorporatedMessage(msgHdr, m_mailDB, destIFolder,
        line 2459 -- nsresult nsParseNewMailState::MoveIncorporatedMessage(nsIMsgDBHdr *mailHdr, 

For non-IMAP files there are two usages. On line 1876 and 2091 of
nsParseMailbox.cpp.

Line 1876 is problematic.
MoveIncorporatedMessage() is returning error loud and clear, but
it is ignored.

This happens inside a function, declared this manner.
int32_t nsParseNewMailState::PublishMsgHeader(nsIMsgWindow *msgWindow)

and it seems to return 0 always at the end.
And its callers do not seem to check the return value anyway.
From MXR:

PublishMsgHeader
Defined as a function prototype in:

    mailnews/local/src/nsParseMailbox.h (View Hg log or Hg annotations)
        line 180 -- virtual int32_t PublishMsgHeader(nsIMsgWindow *msgWindow);
        line 224 -- virtual int32_t PublishMsgHeader(nsIMsgWindow *msgWindow); 

Referenced (in 3 files total) in:

    mailnews/local/src/nsMovemailService.cpp (View Hg log or Hg annotations)
        line 451 -- newMailParser->PublishMsgHeader(nullptr);
        line 479 -- newMailParser->PublishMsgHeader(nullptr); 
    mailnews/local/src/nsParseMailbox.cpp (View Hg log or Hg annotations)
        line 347 -- PublishMsgHeader(nullptr);
        line 389 -- int32_t nsMsgMailboxParser::PublishMsgHeader(nsIMsgWindow *msgWindow)
        line 437 -- PublishMsgHeader(msgWindow);
        line 1797 -- PublishMsgHeader(nullptr);
        line 1813 -- int32_t nsParseNewMailState::PublishMsgHeader(nsIMsgWindow *msgWindow) 
    mailnews/local/src/nsPop3Sink.cpp (View Hg log or Hg annotations)
        line 873 -- m_newMailParser->PublishMsgHeader(aMsgWindow); 


BTW, I think if MoveIncorporatedMessage fails in PlubishMsgHeader, the
following call to RemoveHeaderMdbRow() should not be performed, or should it?

                MoveIncorporatedMessage(m_newMsgHdr, m_mailDB, trash,
                                                      nullptr, msgWindow);
                m_mailDB->RemoveHeaderMdbRow(m_newMsgHdr);

https://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#1876
	This is a separate bug and easy to handle, I should file a
	bugzilla entry and submit a patch for this.

	[I have not hit this path in my test luckily.]

Then later this function calls ApplyFilters() which seems to invoke some filtering rules explicitly.

ApplyFilters() seems to be an important function on its own.  

There are many calls of this function outside this file.  Yet,
ApplyFilters() is a void function, so we cannot propagate the error
condition to the caller side. Any error processing related to errors
that happened below it needs to be handled there or by the lower-level
routines themselves.

So it may be a good idea to do something about the error in the
lower-level routine [such as showing error dialog]. A possibility I
may want to pursue.

Or maybe we can add extra argument to return error status explicitly
to ApplyFilters() and handle the error at the call of ApplyFilters()
here.
But, I think it is best, from the viewpoint of long-term fix with least
man-power cost, to define error processing at this level of call stack
[simply showing dialog to warn the user is enough IMHO], and only when
some worthy upper-level caller requests in the future that the error handling be
done on its side instead of here, we can do the required
modification. But I doubt if such caller would appear at all. 

Expecting all the callers of this function (ApplyFilters()) to do
something proper when an error occurs due to missing .msf is too much
to ask today. After all, the problems seem to have been ignored by
code maintainers for so long (how many years?) because it is difficult
to do. Kickstarting async rebuild is very expensive and should be
avoided, too. This is why I think asking the user to repair it
manually and warning the user here would be enough.  Important thing is
we've got to show something to the user to WARN some operations FAILED after
all. Just letting the operations fail SILENTLY and doing nothing is
unacceptable for an interactive program.)

What about the usage of MoveIncorporatedMessage()  in line 2091?
https://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#2091

This happens inside a function declared as follows, and it returns an error code.

NS_IMETHODIMP nsParseNewMailState::ApplyFilterHit(nsIMsgFilter *filter, nsIMsgWindow *msgWindow, bool *applyMore)
	      [...]
return rv;

The situation is not better here again.

The error code |err| is checked simply to log the success of moving an
article by the filter rule.  

Wait a second. When the failure occurs, nothing is DONE, NOT EVEN THE
RECORDING of the FAILURE ITSELF in the log !?  (Would not this event
be worthy of logging after all!?)

   Maybe I should file a bug on this.

And the error is ignored otherwise completely. I see no mention of the
error code propagated above the call chain.  

At least, we have chance of passing this error up to the caller of
ApplyFilterHit() because it returns an error valueh.

Where will it lead?

From mxr.mozilla.org 
Defined as a function in:

    mailnews/imap/src/nsImapMailFolder.cpp (View Hg log or Hg annotations)
        line 3442, as member of class nsImapMailFolder -- NS_IMETHODIMP nsImapMailFolder::ApplyFilterHit(nsIMsgFilter *filter, nsIMsgWindow *msgWindow, bool *applyMore) 
    mailnews/news/src/nsNNTPNewsgroupList.cpp (View Hg log or Hg annotations)
        line 617, as member of class nsNNTPNewsgroupList -- NS_IMETHODIMP nsNNTPNewsgroupList::ApplyFilterHit(nsIMsgFilter *aFilter, nsIMsgWindow *aMsgWindow, bool *aApplyMore) 

Defined as a function prototype in:

    mailnews/base/search/public/nsIMsgFilterHitNotify.idl (View Hg log or Hg annotations)
        line 25 -- boolean applyFilterHit(in nsIMsgFilter filter, in nsIMsgWindow msgWindow); 
    mailnews/base/search/src/nsMsgFilterList.cpp (View Hg log or Hg annotations)
        line 315 -- rv = listener->ApplyFilterHit(filter, msgWindow, &applyMore); 

Referenced in:

    mailnews/local/src/nsParseMailbox.cpp (View Hg log or Hg annotations)
        line 1988 -- NS_IMETHODIMP nsParseNewMailState::ApplyFilterHit(nsIMsgFilter *filter, nsIMsgWindow *msgWindow, bool *applyMore) 


Again, I am skipping IMAP related usage for now.  It looks ApplyFIlterHit() is
called via XPCOM interface, and there is a call within c++ source,
nsMsgFilterList.cpp.

Come to think of it, I can't try to catch all the callers that sit on
the other end of XPCOM such as .js files, and
so I think it would be best to warn of the error with GUI-like
warning dialog on the definition side.

Defining the tentative error processing, namely, simply warning the
user about error happened due to missing .msf file and so repair it later [to re-create .msf file], at this level of function call stacks, may not sit well with my eventual hope ofmshowing the error dialog ONLY ONCE during a POP3 session.  for an error-case reported in the bug and its comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=634544#c11 

(Somehow 20.0a1 no longer shows the error warning at all. This is the problem
I am hoping to fix now.)
[But a couple of years ago, many e-mails to be filtered to a particular folder end up showing such an error dialog repeatedly.]

But at least, once the error is explicitly warned, it is much better
than the current situation.

A user can be confused today when the user expects certain e-mails in
a particular folder and can not find them because the e-mail articles
remain in the Inbox because filtering has failed silently.

But once the error dialog is properly shown and that the processing
path is identified, I should be able to find a way to control the
showing such warning "ONLY ONCE" during the whole POP3 session
somehow. (I can even record the time of a warning issued to
a folder in the lower-level routine, and I can disable the second showing
if it is, say, less than 1-minute apart, for example, without changing the
overall structure of the code.)

One thing at at a time. 

The failure to check return code of functions is something that
permeates mozilla code base today in so many files (at least in comm-central)
and is a curse.  Most of the bugs would have been noticed
properly if simple checks had been done, and corrected (maybe the
latter is wishful thinking on my side).

Maybe adding document ala JavaDoc style to source files and
marking where the return value is not checked can be a good
volunteer work for TB user community like eradicating compile time warning.

TIA

Updated

•

11 years ago

Depends on: 854172

Updated

•

11 years ago

Blocks: 495760

Comment 7

•

11 years ago

When outdated_msf condition exists, or .msf file is null, explicit folder open(by folder click, explicit mail copy request) invokes internal rebuild index automatically. This is current design/implementation of "recovery from outdated msf condition".

I think message filter should explicitly open copy/move target folder before copy/move.

Comment 8

•

11 years ago

Bug 493429, which was closed as WONTFIX, is also exception report of outdated msf condition from same code upon "mouseover of mail folder".
Why Tb doesn't invoke rebuild index even though Tb fortunately could detect outdated msf condition?

Updated

•

11 years ago

Blocks: 931303

Updated

•

11 years ago

Blocks: 905576

Updated

•

10 years ago

Comment 9

•

10 years ago

This blocks dataloss bugs, so increasing severity

Severity: normal → critical

Comment 10

•

9 years ago

(In reply to WADA from comment #8)
> Bug 493429, which was closed as WONTFIX, is also exception report of
> outdated msf condition from same code upon "mouseover of mail folder".
> Why Tb doesn't invoke rebuild index even though Tb fortunately could detect
> outdated msf condition?

Good questions. 

And I suspect the dependency list for this ancient bug does not list all the possible impacts, which perhaps include:
* loss of notifications
* loss of folder bolding / highlighting
* dataloss symptoms
* filtering failure

How do we transform ISHIKAWA's collected information into a fix?

Flags: needinfo?(rkent)

Flags: needinfo?(acelists)

Whiteboard: [datalossy]

Kent James (:rkent)

Comment 11

•

9 years ago

Rebuilding of summary file is an async operation, as is moving a message. Normal incoming filtering does not have any organized way to handle async processes, which is the source of many bugs of this nature.

In the long run, we really need to have an organized way to handle async operations in filters, which includes error management and perhaps restarting of operations that are interrupted.

In the short run, specific cases (such as move, which is by far the most common filter operation) could have an ad-hoc async handler added that could manage the move, including possible async operations such as rebuilding the summary index.

Flags: needinfo?(rkent)

:aceman

Comment 12

•

9 years ago

I think kicking off rebuilding of msf after finishing download AND locking the UI for some time (as in comment 1) could be acceptable. Better than having broken folders and outdated msf. As long as we communicate the lock to the user with a status or a dialog.

Flags: needinfo?(acelists)

Updated

•

6 years ago

Summary: filter move mail doesn't build summary (index msf) file if it doesn't exist → filter move mail doesn't rebuild summary (index msf) file if it doesn't exist