1400360 - NS_ENSURE warning when popstate.dat does not exist

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[:aceman]

Create a new POP3 account, set it to get messages at startup, but keep TB offline.
On a debug build you get this message in the terminal:
WARNING: NS_ENSURE_SUCCESS(rv, result) failed with result 0x80520012: file mailnews/local/src/nsPop3Protocol.cpp, line 174

It happens because the popstate.dat file doesn't exist yet so can't be opened for reading. We just open a stream from the file without checking it exists.

The code works OK, we exit the function on this error, which is the right action.
But exiting a function via NS_ENSURE_SUCCESS() and getting a console message is not OK under normal conditions.

I think we want warnings in the console output by NS_ENSURE_* macros when there is a programming error (e.g. nullptr passed as out argument to a function). So we do not need a warning under predictable and non-fatal situation - that the file does not exist and that is a not a problem.

Comment 1

[:aceman]

Attachment: 1400360.patch

This should leave the function cleanly and silently.

Comment 2

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #0)
> I think we want warnings in the console output by NS_ENSURE_* macros when
> there is a programming error ...
Sadly that's wishful thinking, the console is littered with errors and I always ask myself how the system can work at all with that many errors :-( - Thanks for making it a little better, but only in a rare case: new profile and offline.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8908845 [details] [diff] [review]
1400360.patch

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

Looks OK, but can't it be simpler?

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +178,2 @@
>    nsCOMPtr<nsIInputStream> fileStream;
> +  rv = NS_NewLocalFileInputStream(getter_AddRefs(fileStream), popState);

Why don't you check for 0x80520012 == NS_ERROR_FILE_NOT_FOUND? Do we need extra code to test file existence?

Comment 4

[:aceman]

(In reply to Jorg K (GMT+2) from comment #2)
> Sadly that's wishful thinking, the console is littered with errors and I
> always ask myself how the system can work at all with that many errors :-( -

Actually I checked that and very little of the messages are from c-c code. So I think we could achieve this 'wish'.

(In reply to Jorg K (GMT+2) from comment #3)
> Why don't you check for 0x80520012 == NS_ERROR_FILE_NOT_FOUND? Do we need
> extra code to test file existence?

I don't want to rely on the specific codes. Those can change any time. Yes, if we cleanup the NS_ENSURE_ARG_* macros, an error can change e.g. from NS_ERROR_FAILURE to NS_ERROR_NULL_POINTER. NS_ERROR_FILE_NOT_FOUND changing to something else would be rare case.
But it may be that NS_NewLocalFileInputStream changes implementation and just returns NS_ERROR_FAILURE in any case. There is no guarantee NS_ERROR_FILE_NOT_FOUND from lower levels will get propagated to the caller for us to check for.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8908845 [details] [diff] [review]
1400360.patch

I'm not really convinced, but anyway. I agree that one vague return code can change to another, but I'd eat my hat if NS_ERROR_FILE_NOT_FOUND changed to something else. I'd prefer not to add more checking code and just add:
if (rv == NS_ERROR_FILE_NOT_FOUND)
  return result;

Worst cast, the error code changes and you'll see the new error on the console. Not the end of the world.

But anyway, I'm not going to impose myself here. Just think of the milliseconds wasted to check file existence of a file which will almost always exist.

If I haven't convinced you, take the r+, otherwise send another patch ;-)

I have another argument ;-) You return if checking for file existence fails. Is that really what you want? If you can't even check, then something is seriously fishy. So it should at least be:

nsresult rv = popState->Exists(&exists);
NS_ENSURE_SUCCESS(rv, result);
if (!exists)
  return result;

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: errors I see.txt

(In reply to :aceman from comment #4)
> Actually I checked that and very little of the messages are from c-c code.
> So I think we could achieve this 'wish'.
You need glasses? ;-(

Comment 7

[:aceman]

So, just 4 unique hits? How does that disprove "very little" ? :)
In my TB debug run I have 2, of which 1 I fix here.

Comment 8

[:aceman]

(In reply to Jorg K (GMT+2) from comment #5)
> I'm not really convinced, but anyway. I agree that one vague return code can
> change to another, but I'd eat my hat if NS_ERROR_FILE_NOT_FOUND changed to
> something else. I'd prefer not to add more checking code and just add:
> if (rv == NS_ERROR_FILE_NOT_FOUND)
>   return result;

But how would the whole block look like in this version? To get the console output in other cases but this one.
 
> Worst cast, the error code changes and you'll see the new error on the
> console. Not the end of the world.

OK.
 
> But anyway, I'm not going to impose myself here. Just think of the
> milliseconds wasted to check file existence of a file which will almost
> always exist.

Yes, I don't like this slowdown either.
 
> I have another argument ;-) You return if checking for file existence fails.
> Is that really what you want? If you can't even check, then something is
> seriously fishy. So it should at least be:
> 
> nsresult rv = popState->Exists(&exists);
> NS_ENSURE_SUCCESS(rv, result);
> if (!exists)
>   return result;

True :)

Comment 9

[:aceman]

  nsCOMPtr<nsIInputStream> fileStream;
  nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(fileStream), popState);
  if (NS_FAILED(rv)) {
    // It is OK if the file doesn't exist. No state is stored yet.
    if (rv == NS_ERROR_FILE_NOT_FOUND)
      return result;
    NS_ENSURE_SUCCESS(rv, result);
  }

?

Comment 10

[Jorg K (CEST = GMT+2)]

No.

  nsCOMPtr<nsIInputStream> fileStream;
  nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(fileStream), popState);
  // It is OK if the file doesn't exist. No state is stored yet.
  if (rv == NS_ERROR_FILE_NOT_FOUND)
    return result;
  NS_ENSURE_SUCCESS(rv, result);

Comment 11

[:aceman]

But then (rv == NS_ERROR_FILE_NOT_FOUND) is an unnecessary perf hit and the condition will almost always be false (and then you run NS_ENSURE again). My version is faster as in common case NS_FAILED(rv) is skipped quickly. But yes, my version is uglier and that is why we discuss how to do this without getting too ugly.

Comment 12

[Jorg K (CEST = GMT+2)]

So you're saying that I test rv twice and you test it once in the good cases. Well. I think the comparison is negligible, not a performance hit ;-) I think NS_FAILED() (http://searchfox.org/mozilla-central/source/xpcom/base/nsError.h#34) is more expensive than a comparison.

Also, if (NS_FAILED(rv)) ... NS_ENSURE_SUCCESS(), looks funny, since we already know it failed.

Comment 13

[:aceman]

(In reply to Jorg K (GMT+2) from comment #12)
> So you're saying that I test rv twice and you test it once in the good
> cases. Well. I think the comparison is negligible, not a performance hit ;-)
> I think NS_FAILED()
> (http://searchfox.org/mozilla-central/source/xpcom/base/nsError.h#34) is
> more expensive than a comparison.

Yes, but you have a comparison + a NS_FAILED (inside NS_ENSURE_SUCCESS).
I only have NS_FAILED. For the common case for both.

> Also, if (NS_FAILED(rv)) ... NS_ENSURE_SUCCESS(), looks funny, since we
> already know it failed.

True, that is why I ask for suggestions :) But I see no other means of getting the output NS_ENSURE_SUCCESS() does.
Maybe just calling  NS_ENSURE_SUCCESS_BODY() directly, but that is done nowhere.

Comment 14

[:aceman]

OK, you win:) There are multiple occurrences of your pattern in c-c and m-c (sample at https://dxr.mozilla.org/comm-central/rev/5ded36cb383d3ccafd9b6c231c5120dcdae196a2/mozilla/netwerk/base/nsFileStreams.cpp#498). Patch incoming.

Comment 15

[:aceman]

Attachment: 1400360.patch v2

I changed the author to you and I give you r+ :)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0247cf33085b
Do not warn if popstate.dat does not exist yet in nsPop3Protocol.cpp. r=aceman