Closed Bug 1511692 Opened Last year Closed Last year

Extremely spammy warning |WARNING: 'NS_FAILED(rv)', file [snip]/xpcom/io/NonBlockingAsyncInputStream.cpp, line 171|

Categories

(Core :: Networking, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1435899 +++

We see this warning thousands of times in our test suite:
https://taskcluster-artifacts.net/byD651nKS3unzZJq4IzNHQ/0/public/logs/live_backing.log

It comes from here:
https://hg.mozilla.org/mozilla-central/rev/5461b62599d7#l1.14
Flags: needinfo?(amarchesini)
Thank you for filing this.  I am not sure if the FF application generates this warning in the DEBUG BUILD as  TB's DEBUG BUILD of TB.

But I think we can discards "NS_WARN_IF()" call in the if-expression once the situtation is assessed.
It's perfectly legal to return an error (e.g. NS_BASE_STREAM_CLOSED) for Available().  It's usually not fatal, only indication of a legal state.
If it is legit to return an error, then we can safely remove the call to NS_WARN_IF().

Suggested patch attached.
It compiles locally under linux.
Assignee: nobody → ishikawa
Attachment #9029366 - Flags: review?(honzab.moz)
Comment on attachment 9029366 [details] [diff] [review]
remove NS_WARN_IF() call to suppress spammy warning.

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

::: xpcom/io/NonBlockingAsyncInputStream.cpp
@@ +167,5 @@
>  
>  NS_IMETHODIMP
>  NonBlockingAsyncInputStream::Available(uint64_t* aLength) {
>    nsresult rv = mInputStream->Available(aLength);
> +  if (NS_FAILED(rv)) {

I don't think this will be approved since the idea is to hear about errors. I think first we need to find out what the specific error is and then ignore it.
I added some debug and the error returned is 0x80470002 which is NS_BASE_STREAM_CLOSED. Oh, that was already predicted. So I would code something like
nsresult rv = mInputStream->Available(aLength);
if (rv == NS_BASE_STREAM_CLOSED || NS_WARN_IF(NS_FAILED(rv)) {
  return rv;
}
(In reply to Jorg K (GMT+1) from comment #5)
> I added some debug and the error returned is 0x80470002 which is
> NS_BASE_STREAM_CLOSED. Oh, that was already predicted. So I would code
> something like
> nsresult rv = mInputStream->Available(aLength);
> if (rv == NS_BASE_STREAM_CLOSED || NS_WARN_IF(NS_FAILED(rv)) {
>   return rv;
> }

I see. Thank you.
Attachment #9029389 - Flags: review?(honzab.moz) → review+
Attachment #9029366 - Attachment is obsolete: true
Attachment #9029366 - Flags: review?(honzab.moz)
Attachment #9029389 - Attachment description: 1511692-suppress-warning.patch → 1511692-suppress-warning.patch - When landing, change to r=baku
Flags: needinfo?(amarchesini)
Thanks, Andrea!

Dear sheriff, please change reviewer to r=baku when landing.
Assignee: ishikawa → jorgk
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e77b8931ae7a
suppress warning for NS_BASE_STREAM_CLOSED from base streams Available() call. r=baku
Keywords: checkin-needed
Comment on attachment 9029389 [details] [diff] [review]
1511692-suppress-warning.patch - When landing, change to r=baku

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

Yes, that's exactly what I wanted to suggest!  thanks.
Attachment #9029389 - Flags: review+
Whiteboard: [necko-triaged]
https://hg.mozilla.org/mozilla-central/rev/e77b8931ae7a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.