1307085 - C-C TB: Tighter checking of InitWithFile and friends

Status: NEW

(Thunderbird :: General, defect)

Description

[ISHIKAWA, Chiaki]

While debugging strange bug under Windows on try-comm-central treeherder job (I use linux for local debug), I noticed that a function called InitWithFile has an important role that interfaces with local file system but
it does not print messages when it fails (for debugging) and also some checks in it and related functions were either a mess or lacking.

So I added them for local debugging, but I think at least the bulk of the patches are worth importing to the official tree. 

The patches DID help the strange bug I saw under Windows with C-C TB: a patch of mine changes the timing of closing a file and while linux/solaris/freebsd does not have a qualm of removing/renaming a file while it is open, windows balks at such operation. Since the closing is delayed, TB was trying to rename a file still open. My patch helped me to home in the name of the file and directory where the prolblem occurred. (The print occurs only in debug build #if DEBUG).
The tighter checking, however, should be useful in non-debug build, too.

The currently used patches follow.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Bug 1307085-a - C-C TB: Tigher checking of InitWithFile and friends. A few checks thrown in other files as well.

The first patch. It contains a few extra checks, but you get the idea.

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Bug 1307085-b - check the return value of InitWithFile() in files under 'import' directory.

Second patch that extends the check to import directory.

Comment 3

[ISHIKAWA, Chiaki]

I hasten to add that the extra checks in part-a of the patches were necessary to track down the strange bug I saw under Windows on try-comm-central (remotely, that is).

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Bug 1307085-a - C-C TB: Tigher checking of InitWithFile and friends. A few checks thrown in other files as well.

Fixed : #if DEBUG -> #ifdef DEBUG
and bitrot caused by source tree change.

Comment 5

[ISHIKAWA, Chiaki]

Attachment: check-InitWithFile-import-files-Rev00.patch

Fixed #if DEBUG -> DEBUG
fixed bitrot.

Jorg, this patch set has helped me to find an obscure bug related to the rename/moving/deletion of a file that is still open under windows.

With this patch, I noticed that there *may* be a problem in the test 
mailnews/jsaccount/test/unit/test_jaMsgFolder.js

Without the patch, the test seems to succeed, but there is a directory creation error which was ignored during operation. I think that is bad...
Either the test is broken or TB does not check low-level operation very well.
With the patch, the test seems to fail.

Anyway, now I feel this patch set is important and so requesting review.

TIA

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8889301 [details] [diff] [review]
check-InitWithFile-import-files-Rev00.patch

Can you please explain why changes in the (dead) import of Apple Mail and Windows Live Mail would affect a test in JsAccount?

Maybe the first patch uncovers an error, but most likely not this one.

Comment 7

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+2) from comment #6)
> Comment on attachment 8889301 [details] [diff] [review]
> check-InitWithFile-import-files-Rev00.patch
> 
> Can you please explain why changes in the (dead) import of Apple Mail and
> Windows Live Mail would affect a test in JsAccount?
> 
> Maybe the first patch uncovers an error, but most likely not this one.

Of course, you are totally correct. I put the comment on the second patch for /import subdirectory.

My original comment should have read:

Jorg, this patch set has helped me to find an obscure bug related to the rename/moving/deletion of a file that is still open under windows.

With this patch *SET*, I noticed that there *may* be a problem in the test 
mailnews/jsaccount/test/unit/test_jaMsgFolder.js

Without the patch *SET*, the test seems to succeed, but there is a directory creation error which was ignored during operation. I think that is bad...
Either the test is broken or TB does not check low-level operation very well.
With the patch *SET*, the test seems to fail.

Anyway, now I feel this patch set is important and so requesting review.

I changed the phrase, "this patch" to "this patch *SET*" in a few places.

Yes, definitely the first patch caused the test fail somehow.

TIA


TIA

Comment 8

[ISHIKAWA, Chiaki]

Hi,

My patch set is good. (I will upload a somewhat enhanced patch with detailed pathname dump inside #ifdef DEBUG,#endif).

It is the xpctest mailnews/jsaccount/test/unit/test_jaMsgFolder.js that is buggy or not robust enough for environmental changes.

I found a very interesting behavior.
If I run this test individually continuously, I get the following pattern:

pass
fail
pass
fail
...

That is, it succeeds, and then on the subsequent invocation it fails.
But it succeeds again.
I suspect the test puts the temporary test profile directory in a strange state (one directory is created without
"X" bit under linux and thus the subsequent directory access is denied! I have no idea where it is left in this
strange state yet.

Detail:
mailnews/jsaccount/test/unit/test_jaMsgFolder.js seems to leave
the following directory without access bits RWX, X being the directory entry can be read.
/tmp/firefox/xpcshellprofile/TestFoo

This seems to cause the failure sometimes. 

ls -l /tmp/firefox/xpcshellprofile/TestFoo
ls: cannot access /tmp/firefox/xpcshellprofile/TestFoo/.: Permission denied
ls: cannot access /tmp/firefox/xpcshellprofile/TestFoo/..: Permission denied
total 0
d????????? ? ? ? ?            ? ./
d????????? ? ? ? ?            ? ../
ishikawa@debian-hp-note:/home/ishikawa/TB-SRC/comm-central$ 
ls -l /tmp/firefox/xpcshellprofile/
total 88
drwxr-xr-x 5 ishikawa ishikawa  4096 Jul 26 16:19 ./
drwxr-xr-x 3 ishikawa ishikawa  4096 Jul 26 16:19 ../
drw------- 2 ishikawa ishikawa  4096 Jul 26 16:19 TestFoo/  <--- No X bit.
drwx------ 4 ishikawa ishikawa  4096 Jul 26 16:19 cache2/
-rw------- 1 ishikawa ishikawa 65536 Jul 26 16:19 cert8.db
-rw------- 1 ishikawa ishikawa 16384 Jul 26 16:19 key3.db
-rw-r--r-- 1 ishikawa ishikawa   821 Jul 26 16:19 mozinfo.json
-rw-r--r-- 1 ishikawa ishikawa   160 Jul 26 16:19 panacea.dat
-rw------- 1 ishikawa ishikawa 16384 Jul 26 16:19 secmod.db
drwxr-xr-x 2 ishikawa ishikawa  4096 Jul 26 16:19 startupCache/
ishikawa@debian-hp-note:/home/ishikawa/TB-SRC/comm-central$ 


My patch here checks these file I/O issues much strictly and thus seems to cause
the failure always, and without the strict checking, the test seems to repeat the following "pass; failure" pattern:
pass
failure
pass
failure
...


I will file a separate bug on the test
mailnews/jsaccount/test/unit/test_jaMsgFolder.js

(These are one of the two remaining bugs that were uncovered during the final testing of buffered-output or buffered-download for POP3 patch. The other one seems to be also caused by a local patch that is NOT related to the buffered output patch at all.
So I hope to make the patch for buffered output ready on Sunday, but a family business may put it off a few days.)

Comment 9

[Jorg K (CEST = GMT+2)]

Do you want me to accept a patch that causes test failures?

I haven't looked into test_jaMsgFolder.js, but the test is 50 lines long and does no direct interaction with the file system. So if it fails, there must be a problem elsewhere.

Comment 10

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+2) from comment #9)
> Do you want me to accept a patch that causes test failures?
> 
> I haven't looked into test_jaMsgFolder.js, but the test is 50 lines long and
> does no direct interaction with the file system. So if it fails, there must
> be a problem elsewhere.

Jorg, it is the test test_jaMsgFolder.js that is buggy, I think.
See the bugzilla:
Bug 1384461 - C-C TB: Unreliable test: mailnews/jsaccount/test/unit/test_jaMsgFolder.js (it repeats pass/fail/pass/fail if executed individually)

I think if the test if executed as part of whole xpcshell test, it succeeds, but
as I found it,
if it is INDIVIDUALLY EXECUTED, it repeats succeeds, fails, suceeds, fails, ...
I have no idea what is wrong. (That is under linux).
My patch in this bugzilla simply seems to find a strange failure all the time instead of only every two times.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8889293 [details] [diff] [review]
Bug 1307085-a - C-C TB: Tigher checking of InitWithFile and friends. A few checks thrown in other files as well.

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

OK, I've looked this over. So so added a lot of debug and additional error checking which potentially changed program behaviour. Please address the minor issues I mention below.

::: mailnews/base/search/src/nsMsgBodyHandler.cpp
@@ +133,5 @@
>  
> +// xxx
> +// Even if the low-level routine returns early with I/O error indication,
> +// the current framework does not pay heed to such error value :-(
> +//

Lose this comment.

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +1887,5 @@
>    nsresult rv = m_folder->GetMsgInputStream(aMsgHdr, &reusable,
>                                              getter_AddRefs(m_inputStream));
> +#ifdef DEBUG
> +  if (NS_FAILED(rv)) {
> +    fprintf(stderr,"(debug) nsMsgSearchScopeTerm::GetInputStream():m_folder->GetMsgInputStream failed returning rv=0x%08x\n", (unsigned int) rv);

Please use:
rv=0x%" PRIx32 "\n", static_cast<uint32_t>(rv)));
here and elsewhere.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +1363,3 @@
>    nsCOMPtr <nsIFile> path;
>    rv = GetFilePath(getter_AddRefs(path));
> +  NS_ENSURE_SUCCESS(rv, rv);    // why was the error checking missing before?

Lose comment.

@@ +1389,3 @@
>  
>      // if it's a server, we don't need the .msf appended to the name
> +    if (NS_SUCCEEDED(rv2) && !isServer)

Why change behaviour here?

@@ +1402,5 @@
> +
> +      // However, note that rv from the above line is returned at the
> +      // end of this function body.  But what about the errors of
> +      // InitWithFile() or the failure of dbPath->Create() (the
> +      // latter was NOT checked AT ALL. Sigh ....)

Please no epic emotional comments. If there is work to do, put XXX TODO: Check ...

@@ +1423,5 @@
>        if (createDBIfMissing && NS_SUCCEEDED(dbPath->Exists(&exists)) && !exists) {
>          rv = dbPath->Create(nsIFile::NORMAL_FILE_TYPE, 0644);
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
> +    } 

Trailing space.

@@ +4815,5 @@
> +    if (NS_FAILED(rv)) {
> +#ifdef DEBUG
> +      fprintf(stderr,"(debug) nsMsgDBFolder::GetFilePath: parseURI(true) failed. rv=%08x\n", (unsigned int) rv);
> +#endif
> +      // We ought to retun here to avoid passing empty mPath to InitWithFile below.

Typo. In fact, lose the comment. Can mPath be null?

::: mailnews/base/util/nsMsgUtils.h
@@ +432,5 @@
> + *    nsresult functionA () {
> + *    ....
> + *    rv = ptr->InitWithFile(filePath);
> + *    if (NS_FAILED(rv)) {
> + *    #ifdef DEBUG   

Trailing spaces.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +1172,5 @@
> +        else {
> +#ifdef DEBUG
> +          DUMP_INITWITHNATIVEPATH_PATH("nsMsgDatabase::OpenMDB", dbFile, m_dbName, ret);
> +#endif
> +          return NS_MSG_ERROR_FOLDER_SUMMARY_MISSING;

Why not return 'ret' here? You're returning a specific error that someone might be looking for.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +486,2 @@
>      currentFolderDBNameStr = currentFolderNameStr;
> +    NS_ENSURE_SUCCESS(rv, rv);

Why again?

::: mailnews/mime/src/mimeebod.cpp
@@ +185,5 @@
> +        if (NS_FAILED(rv)) {
> +#ifdef DEBUG
> +          DUMP_INITWITHNATIVEPATH_PATH("MimeExternalBody_make_url", fs, NS_LITERAL_CSTRING("/afs/."), rv);
> +#endif
> +          return 0;             /* xxx: we may want to show a large error dialog? */

// XXX:

@@ +511,5 @@
> +      if (NS_FAILED(rv)) {
> +#ifdef DEBUG
> +        DUMP_INITWITHNATIVEPATH_PATH("MimeExternalBody_displayable_inline_p", fs, NS_LITERAL_CSTRING("/afs/."), rv);
> +#endif
> +        return 0;             /* xxx: we may want to show a large error dialog? */

// XXX:

Comment 12

[Jorg K (CEST = GMT+2)]

I ran |mozilla/mach xpcshell-test mailnews/jsaccount/test/unit/test_jaMsgFolder.js| various times with the patch applied and it passed every time. So I don't see a problem accepting the patch once the issues are addressed.

Comment 13

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+2) from comment #12)
> I ran |mozilla/mach xpcshell-test
> mailnews/jsaccount/test/unit/test_jaMsgFolder.js| various times with the
> patch applied and it passed every time. So I don't see a problem accepting
> the patch once the issues are addressed.

I will look over your comment to clean up the patch.

BTW, did you run the test under Windows?
In my case, it was under Linux. Not sure if there is any difference (except for the cases where move/deletion/rename happens while there are open file descriptors to the files in question.)

TIA

Comment 14

[ISHIKAWA, Chiaki]

I have no idea why Windows test proceeds without flipping (pass/fail/pass/fail).
At least, under linux, I needed to apply a couple of fixes to run
mailnews/jsaccount/test/unit/test_jaMsgFolder.js
the test successfully finally.

The two patches: I need a patch in
Bug 1384461 - C-C TB: Unreliable test: mailnews/jsaccount/test/unit/test_jaMsgFolder.js (it repeats pass/fail/pass/fail if executed individually)
and  another patch in
Bug 1385816 - nsFileStreamBase::DoOpen() should call directory creation with 0755 permission instead of 0644.

With these, my linux PCs can run the test mailnews/jsaccount/test/unit/test_jaMsgFolder.js successfully.

I now start to polish the patch here.
Once done, I will post the patch.
(Then I will post the cleanup patch to Bug 1242030 - Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500 : the bug in bug 13894461 is one of the two remaining bugs I saw with the patches in Bug 1242830. The other one seems related to the exchange of news message from a news server(?), and I am not sure if I can run that test successfully on local PC.)

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to ISHIKAWA, Chiaki from comment #13)
> BTW, did you run the test under Windows?
Yes. Windows has a different security model, so it's well possible that it behaves differently.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8889293 [details] [diff] [review]
Bug 1307085-a - C-C TB: Tigher checking of InitWithFile and friends. A few checks thrown in other files as well.

Clearing the review here since I already commented in comment #11.
Looks like bug 1385816 and bug 1384461 have landed, so nothing is stopping us now ;-)

Comment 17

[ISHIKAWA, Chiaki]

(In reply to Jorg K (GMT+2) from comment #16)
> Comment on attachment 8889293 [details] [diff] [review]
> Bug 1307085-a - C-C TB: Tigher checking of InitWithFile and friends. A few
> checks thrown in other files as well.
> 
> Clearing the review here since I already commented in comment #11.
> Looks like bug 1385816 and bug 1384461 have landed, so nothing is stopping
> us now ;-)

I will polish the patch as you suggested in comment #11.
Right now, I am in the process of obtaining a statistics of how the patch(es) in 
Bug 1242030 - Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500
affect the performance (elapsed time, # of invoked system calls) for downloading "typical mix" of e-mails under various settings.
(local mail host vs remote mailhost, mail storage in local disk vs remote file system.)
This will guide us to decide whether we use caches of file streams for saving a few pair of open/close 
per download of one message, or forget about the cache and simplify the code for easier maintenance.
[I would think to save the precious man-hour of small developer community, the latter is the better, but let's wait for the statistics.]

Well gathering the meaningful data is slow going :-(

Finally I picked up about 200 e-mails from real-world usage of my own [from a day last winter],
and figured out how to anomize the data so that I can share the messages if need be.
[I can't really simulate the effect of long header lines from remote host, and so simply decided to INCLUDE the original header lines of my messages as part the main message body when I send and receive these sample messages during data gathering.]

I need a few more steps to create scripts to collect and summarize interesting data.
(I will repeat this step a few times until I get a satisfactory data set.
If someone needs MORE data, I will let that someone do the job by releasing the script  files (used under linux) and
the sample messages (characters are transliterated so that the original text cannot be guessed. So MIME attachments are now
not an attachment, but rather a part of plain text message.)

I am cleaning up the patches including the patch in this bugzilla as part of the process, and so hopefully once the
holiday(s) are over (circa Aug 22) and I will have have the data, I can upload the cleaned patch here and in Bug 1242030 with some statistics.

TIA

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8889301 [details] [diff] [review]
check-InitWithFile-import-files-Rev00.patch

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

You know how I love looking at this dead import code. I was waiting for the first patch to be refreshed, hence the delay. Anyway, in an effort to clear my review queue down to the essential, I've given this a quick glance. Whilst adding debug doesn't hurt, I'm a unsure whether we should add debug to code that is essentially not executed since these importers are pretty much dead, as I said on various occasions.

::: mailnews/import/applemail/src/nsAppleMailImport.cpp
@@ +469,5 @@
> +        DUMP_INITWITHPATH_PATH_IMPORT("nsAppleMailImportMail::FindMboxDirs", siblingMailboxDir,
> +                                      (char *) NS_ConvertUTF16toUTF8(siblingMailboxDirPath).get(),
> +                                      rv);
> +#endif
> +        continue;               // xxx: we may want to print a big warning dialog.

XXX:

::: mailnews/import/applemail/src/nsAppleMailImport.h
@@ +1,1 @@
> +

Extra blank line.

@@ +16,5 @@
>  // logging facilities
>  static mozilla::LazyLogModule APPLEMAILLOGMODULE("APPLEMAILIMPORTLOG");
>  
> +#if !defined(IMPORT_LOG0)
> +/* It is likely that these are defined in mailnews/import/src/ImportDebug.h already */

Hmm, can we give a definitive answer, "likely" is likely to confuse the reader.

::: mailnews/import/src/ImportDebug.h
@@ +27,5 @@
> +
> +#define DUMP_INITWITHFILE_PATH_IMPORT(func, p1, p2, result)                    \
> +  {                                                                     \
> +    nsAutoCString nativePath;                                           \
> +    IMPORT_LOG4("(debug) %s: %s->InitWithFile(%s) failed: rv = 0x%08x\n", func, #p1, #p2, result); \

The proper logging for "rv" is like this:
MOZ_LOG(DBLog, LogLevel::Info, ("error opening db %" PRIx32, static_cast<uint32_t>(rv)));

::: mailnews/import/winlivemail/WMDebugLog.h
@@ +9,5 @@
>  #include "mozilla/Logging.h"
>  static mozilla::LazyLogModule WMLOGMODULE("IMPORT");  // Logging module
>  
> +#if !defined(IMPORT_LOG0)
> +/* It is likely that these are defined in mailnews/import/src/ImportDebug.h already */

Same comment here.

Comment 19

[ISHIKAWA, Chiaki]

Dear Jorg,

Thank you for your comment:

(In reply to Jorg K (GMT+2) from comment #18)
> Comment on attachment 8889301 [details] [diff] [review]
> check-InitWithFile-import-files-Rev00.patch
> 
> Review of attachment 8889301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You know how I love looking a this dead import code. I was waiting for the
> first patch to be refreshed, hence the delay. 

Sorry, I was wondering which way to go in bug 1242030
since this patch has to accommodate the
changes in the patch in 1242030.

> Anyway, in an effort to clear
> my review queue down to the essential, I've given this a quick glance.
> Whilst adding debug doesn't hurt, I'm a unsure whether we should add debug
> to code that is essentially not executed since these importers are pretty
> much dead, as I said on various occasions.

I am not sure what the status here.
I was checking the code for the completeness's sake.
Do you  mean the OSX users don't need the import code?
If so, I can certainly omit the changes to applemail import code.

 
> ::: mailnews/import/applemail/src/nsAppleMailImport.cpp
> @@ +469,5 @@
> > +        DUMP_INITWITHPATH_PATH_IMPORT("nsAppleMailImportMail::FindMboxDirs", siblingMailboxDir,
> > +                                      (char *) NS_ConvertUTF16toUTF8(siblingMailboxDirPath).get(),
> > +                                      rv);
> > +#endif
> > +        continue;               // xxx: we may want to print a big warning dialog.
> 
> XXX:
> 

will do.

> ::: mailnews/import/applemail/src/nsAppleMailImport.h
> @@ +1,1 @@
> > +
> 
> Extra blank line.


will remove.

 
> @@ +16,5 @@
> >  // logging facilities
> >  static mozilla::LazyLogModule APPLEMAILLOGMODULE("APPLEMAILIMPORTLOG");
> >  
> > +#if !defined(IMPORT_LOG0)
> > +/* It is likely that these are defined in mailnews/import/src/ImportDebug.h already */
> 
> Hmm, can we give a definitive answer, "likely" is likely to confuse the
> reader.

I will remove "It is likely that" part.
 
> ::: mailnews/import/src/ImportDebug.h
> @@ +27,5 @@
> > +
> > +#define DUMP_INITWITHFILE_PATH_IMPORT(func, p1, p2, result)                    \
> > +  {                                                                     \
> > +    nsAutoCString nativePath;                                           \
> > +    IMPORT_LOG4("(debug) %s: %s->InitWithFile(%s) failed: rv = 0x%08x\n", func, #p1, #p2, result); \
> 
> The proper logging for "rv" is like this:
> MOZ_LOG(DBLog, LogLevel::Info, ("error opening db %" PRIx32,
> static_cast<uint32_t>(rv)));


I meant to fix this issue of printing the |rv| value uniformly when 
I refresh the patch in bug 1242030.
There are many instances and so I would like to have them fixed in one go when the
decision in bug 1242030 is made.

> ::: mailnews/import/winlivemail/WMDebugLog.h
> @@ +9,5 @@
> >  #include "mozilla/Logging.h"
> >  static mozilla::LazyLogModule WMLOGMODULE("IMPORT");  // Logging module
> >  
> > +#if !defined(IMPORT_LOG0)
> > +/* It is likely that these are defined in mailnews/import/src/ImportDebug.h already */
> 
> Same comment here.

Ditto above.

Thank you again for the review.

PS:  BTW, have you seen anything like the error I posted to dev-apps-thunderbird mailing list.
I can't run |make mozmill| locally for the last few days which makes it rather difficult for me to check the sanity of my local patches :-(  
I don't see anything like that in comm-central and try-comm-central and wonder what went wrong.
https://groups.google.com/forum/#!topic/mozilla.dev.apps.thunderbird/Y575uOOIE_s

Comment 20

[Wayne Mery (:wsmwk)]

(In reply to ISHIKAWA, Chiaki from comment #19)
> Dear Jorg,
> ...
> > The proper logging for "rv" is like this:
> > MOZ_LOG(DBLog, LogLevel::Info, ("error opening db %" PRIx32,
> > static_cast<uint32_t>(rv)));
> 
> 
> I meant to fix this issue of printing the |rv| value uniformly when 
> I refresh the patch in bug 1242030.
> There are many instances and so I would like to have them fixed in one go
> when the
> decision in bug 1242030 is made.
> 
> > ::: mailnews/import/winlivemail/WMDebugLog.h
> > @@ +9,5 @@
> > >  #include "mozilla/Logging.h"
> > >  static mozilla::LazyLogModule WMLOGMODULE("IMPORT");  // Logging module
> > >  
> > > +#if !defined(IMPORT_LOG0)
> > > +/* It is likely that these are defined in mailnews/import/src/ImportDebug.h already */
> > 
> > Same comment here.
> 
> Ditto above.
> 
> Thank you again for the review.
> 
> PS:  BTW, have you seen anything like the error I posted to
> dev-apps-thunderbird mailing list.
> I can't run |make mozmill| locally for the last few days which makes it
> rather difficult for me to check the sanity of my local patches :-(  
> I don't see anything like that in comm-central and try-comm-central and
> wonder what went wrong.
> https://groups.google.com/forum/#!topic/mozilla.dev.apps.thunderbird/
> Y575uOOIE_s

The last posting there was "Thank you. I left home PC building new binary after rebasing. 
I hope it finished building fine when I'll be back. "

Comment 21

[Wayne Mery (:wsmwk)]

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

...

The proper logging for "rv" is like this:
MOZ_LOG(DBLog, LogLevel::Info, ("error opening db %" PRIx32,
static_cast<uint32_t>(rv)));

I meant to fix this issue of printing the |rv| value uniformly when
I refresh the patch in bug 1242030.
There are many instances and so I would like to have them fixed in one go when the
decision in bug 1242030 is made.

So you want to land this at the same time as bug 1242030 ?

Comment 22

[ISHIKAWA, Chiaki]

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

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

...

The proper logging for "rv" is like this:
MOZ_LOG(DBLog, LogLevel::Info, ("error opening db %" PRIx32,
static_cast<uint32_t>(rv)));

I meant to fix this issue of printing the |rv| value uniformly when
I refresh the patch in bug 1242030.
There are many instances and so I would like to have them fixed in one go when the
decision in bug 1242030 is made.

So you want to land this at the same time as bug 1242030 ?

After 1242030 is more likely it.
This can be applied independently AFTER the patch set in 1242030 lands.
(I am planning to upload the updated patch set of bug 1242030 that has been modified to cope with clang-formated source tree REAL SOON NOW, maybe even tomorrow since it is a windfall holiday here in Japan.)