Closed Bug 1310127 Opened 8 years ago Closed 7 years ago

Use MOZ_MUST_USE in netwerk/protocol/http

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: wcpan, Assigned: wcpan)

References

Details

(Whiteboard: [necko-would-take])

Attachments

(18 files, 5 obsolete files)

Bug 1310127 - Part 1: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1310127 - Part 7: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
jst
: review+
Details
Bug 1310127 - Part 8: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
mkaply
: review+
Details
Bug 1310127 - Part 9: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
hsivonen
: review+
Details
Bug 1310127 - Part 2: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1310127 - Part 11: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
bugzilla
: review+
Details
Bug 1310127 - Part 12: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1310127 - Part 13: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1310127 - Part 14: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
tnikkel
: review+
Details
Bug 1310127 - Part 15: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1310127 - Part 16: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1310127 - Part 17: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
smaug
: review+
Details
bug1310127_part5_psm-remove-unhandled-call_v2.patch
8.06 KB, patch
wcpan
: review+
Details | Diff | Splinter Review
Bug 1310127 - Part 3: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1310127 - Part 4: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1310127 - Part 10: Handle netwerk/protocol/http MOZ_MUST_USE functions in PSM.
59 bytes, text/x-review-board-request
wcpan
: review+
Details
Bug 1310127 - Part 5: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1310127 - Part 6: Use MOZ_MUST_USE in netwerk/protocol/http
59 bytes, text/x-review-board-request
dragana
: review+
Details
Assignee: nobody → wpan
Blocks: 1303701
Whiteboard: [necko-would-take]
Flags: needinfo?(valentin.gosu) → needinfo?(mcmanus)
you are apparently asserting that many functions that sometimes return false will not do so. Its not necessarily a bug that they aren't being checked right there - oftentimes this is checked by looking at the expected side effect of the function.
Flags: needinfo?(mcmanus)
Attached patch 0002-PostEvent-may-fail.patch (obsolete) — — Splinter Review 
These PostEvent-based methods may fail if the underlying thread has been terminated first.
Should we just ignore the failure?
Attachment #8819214 - Flags: feedback?(mcmanus)
The parsing sometimes fails.
But I didn't see any bad side effect, maybe we can just print a warning?
Attachment #8819218 - Flags: feedback?(mcmanus)
Attached patch 0004-ResumeRecv-may-fail.patch (obsolete) — — Splinter Review 
If I assert the ResumeRecv() failure, the control flow of Http2Session::WriteSegmentsAgain will break.
I don't know how to catch the error though.
Attachment #8819220 - Flags: feedback?(mcmanus)
I have no idea why this code is here and why it fails.
Maybe it can be safely ignored?
Attachment #8819222 - Flags: feedback?(mcmanus)
Comment on attachment 8819214 [details] [diff] [review]
0002-PostEvent-may-fail.patch

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

your new comments are wrong. Those calls cannot fail - they were all asserting success in debug builds. I think its fine to remove the asserts at this point (we are confident they work) and move to Unused - just don't add the comment.
Attachment #8819214 - Flags: feedback?(mcmanus)
Comment on attachment 8819218 [details] [diff] [review]
0003-ParseCachedOriginalHeaders-may-fail.patch

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

This should just LOG() on failure. thanks.
Attachment #8819218 - Flags: feedback?(mcmanus) → feedback+
Comment on attachment 8819220 [details] [diff] [review]
0004-ResumeRecv-may-fail.patch

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

instead of the comment, this should have a LOG if it fails.. but no further action would be necessary. Thanks.
Attachment #8819220 - Flags: feedback?(mcmanus)
Comment on attachment 8819222 [details] [diff] [review]
0005-read-dummy-byte-may-fail.patch

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

yes you can ignore a fail here
Attachment #8819222 - Flags: feedback?(mcmanus)
Comment on attachment 8819214 [details] [diff] [review]
0002-PostEvent-may-fail.patch

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

oh dear - your patch is not against the trunk. Is it against your previous patches? Please don't do that by default. I rely on the context in the patch to know what's going on.

In any event - I looked at the trunk. These can indeed fail, so your patch comments are correct
Comment on attachment 8821129 [details]
Bug 1310127 - Part 15: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100504/#review101024

I don't know what NullHttpChannel is and whether it might somehow leak into the docshell's use.
If DocShell may have pointers to it, then we'd fire assertions all the time.
Can't really review without knowledge about NullHttpChannel. Some necko peer will need to take a look at this.
Attachment #8821129 - Flags: review?(bugs)
Comment on attachment 8821130 [details]
Bug 1310127 - Part 16: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100506/#review101028

Same applies here. Since we have NullHttpChannel implementation in tree, and it returns failure values in many cases, it is hard to say what is the correct behavior. We don't want to break any current behavior with these patches.
Attachment #8821130 - Flags: review?(bugs)
Comment on attachment 8821131 [details]
Bug 1310127 - Part 17: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100508/#review101030

And here.
Attachment #8821131 - Flags: review?(bugs)
Valentin, could you explain what NullHttpChannel is and whether it may be used outside Necko?
Since if it maybe used somehow, then these patches will start to assert a lot.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8821126 [details]
Bug 1310127 - Part 12: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100498/#review101034

::: browser/components/feeds/nsFeedSniffer.cpp:272
(Diff revision 1)
>      // set the feed header as a response header, since we have good metadata
>      // telling us that the feed is supposed to be RSS or Atom
> +    mozilla::DebugOnly<nsresult> rv =
> -    channel->SetResponseHeader(NS_LITERAL_CSTRING("X-Moz-Is-Feed"),
> +      channel->SetResponseHeader(NS_LITERAL_CSTRING("X-Moz-Is-Feed"),
> -                               NS_LITERAL_CSTRING("1"), false);
> +                                 NS_LITERAL_CSTRING("1"), false);
> +                                 MOZ_ASSERT(NS_SUCCEEDED(rv));

wrong indentation
Attachment #8821126 - Flags: review?(mak77) → review+
Comment on attachment 8821121 [details]
Bug 1310127 - Part 7: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100488/#review101096
Attachment #8821121 - Flags: review?(jst) → review+
Comment on attachment 8821128 [details]
Bug 1310127 - Part 14: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100502/#review101118
Attachment #8821128 - Flags: review?(tnikkel) → review+
Attachment #8819213 - Attachment is obsolete: true
Attachment #8819214 - Attachment is obsolete: true
Attachment #8819218 - Attachment is obsolete: true
Attachment #8819220 - Attachment is obsolete: true
Attachment #8819222 - Attachment is obsolete: true
Comment on attachment 8821127 [details]
Bug 1310127 - Part 13: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100500/#review101180

::: layout/style/FontFaceSet.cpp:622
(Diff revision 1)
>  
>    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
>    if (httpChannel) {
> -    httpChannel->SetReferrerWithPolicy(aFontFaceSrc->mReferrer,
> +    rv = httpChannel->SetReferrerWithPolicy(aFontFaceSrc->mReferrer,
> -                                       mDocument->GetReferrerPolicy());
> +                                            mDocument->GetReferrerPolicy());
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

We know these can't fail (at least, according to the documentation of these methods) since the channel is in the right state to accept these setters.

But given we have other nsresult checks in this function, where we return using NS_ENSURE_SUCCESS(rv, rv) if they fail, we should probably return rv if these fail, too.

I don't know that it's super important to MOZ_ASSERT them rather than just warn, so I'd be happy with just replacing the MOZ_ASSERT lines with NS_ENSURE_SUCCESS(rv, rv).

r=me with that, and in Loader.cpp.
Attachment #8821127 - Flags: review?(cam) → review+
Comment on attachment 8821124 [details]
Bug 1310127 - Part 2: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100494/#review101308

Thanks.

The changes here look fine, but it turns we can actually get rid of this code entirely.
I'll attach a patch to do exactly that, but it's up to you whether to take the patch or land the changes here instead.
Attachment #8821124 - Flags: review?(cykesiopka.bmo) → review+
Alternate Part 5 PSM changes that involve pure code removal.
Attachment #8821559 - Flags: review?(wpan)
Comment on attachment 8821120 [details]
Bug 1310127 - Part 1: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100486/#review101340

I stopped going through this review part way.

You have a mixture of cases where you 

a] add the must_use markings
b] keep the current behavior marking things unused or maybe logging fails
c] add asserts that things succeed where they didn't previously check that
d] return errors that were ignored before to callers

In general but c and d can introduce bugs where the code was tolerant to the failure before and will now fail at a user visible layer. That's not necessarily what we want. When calling a XPCOM interface I almost never want [c] (but when calling non-xpcom interfaces its common)

Can you rework this megapatch into 4 patches along those lines - that will be easier to understand - and consider whether everything is in the right category?

::: netwerk/base/Predictor.cpp:1311
(Diff revision 1)
>      PREDICTOR_LOG(("    Could not get HTTP Channel from new channel!"));
>      return NS_ERROR_UNEXPECTED;
>    }
>  
> -  httpChannel->SetReferrer(referrer);
> +  rv = httpChannel->SetReferrer(referrer);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

reutrn the error instead

::: netwerk/base/nsIncrementalDownload.cpp:308
(Diff revision 1)
>        return rv;
>  
> -    if (!mPartialValidator.IsEmpty())
> -      http->SetRequestHeader(NS_LITERAL_CSTRING("If-Range"),
> +    if (!mPartialValidator.IsEmpty()) {
> +      rv = http->SetRequestHeader(NS_LITERAL_CSTRING("If-Range"),
> -                             mPartialValidator, false);
> +                                  mPartialValidator, false);
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

this one can be silently ignored (or logged)

::: netwerk/base/nsIncrementalDownload.cpp:314
(Diff revision 1)
> +    }
>  
>      if (mCacheBust) {
> -      http->SetRequestHeader(NS_LITERAL_CSTRING("Cache-Control"),
> +      rv = http->SetRequestHeader(NS_LITERAL_CSTRING("Cache-Control"),
> -                             NS_LITERAL_CSTRING("no-cache"), false);
> +                                  NS_LITERAL_CSTRING("no-cache"), false);
> -      http->SetRequestHeader(NS_LITERAL_CSTRING("Pragma"),
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

I think these can be silently ignored or logged

::: netwerk/base/nsIncrementalDownload.cpp:675
(Diff revision 1)
>      if (StringBeginsWith(mPartialValidator, NS_LITERAL_CSTRING("W/")))
>        mPartialValidator.Truncate(); // don't use weak validators
> -    if (mPartialValidator.IsEmpty())
> -      http->GetResponseHeader(NS_LITERAL_CSTRING("Last-Modified"), mPartialValidator);
> +    if (mPartialValidator.IsEmpty()) {
> +      rv = http->GetResponseHeader(NS_LITERAL_CSTRING("Last-Modified"), mPartialValidator);
> +      if (NS_FAILED(rv)) {
> +        NS_WARNING("empty validator");

LOG(()) rather than NS_WARNING

::: netwerk/base/nsIncrementalDownload.cpp:881
(Diff revision 1)
>      return rv;
>  
>    // If we didn't have a Range header, then we must be doing a full download.
>    nsAutoCString rangeVal;
> -  http->GetRequestHeader(rangeHdr, rangeVal);
> +  rv = http->GetRequestHeader(rangeHdr, rangeVal);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

you can ignore the failure, as rangeval will still be empty

::: netwerk/base/nsIncrementalDownload.cpp:891
(Diff revision 1)
>  
>    // A redirection changes the validator
>    mPartialValidator.Truncate();
>  
>    if (mCacheBust) {
> -    newHttpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Cache-Control"),
> +    rv = newHttpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Cache-Control"),

either ignore rv or log the fail. no need to return

::: netwerk/base/nsIncrementalDownload.cpp:894
(Diff revision 1)
>  
>    if (mCacheBust) {
> -    newHttpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Cache-Control"),
> +    rv = newHttpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Cache-Control"),
> -                                     NS_LITERAL_CSTRING("no-cache"), false);
> +                                          NS_LITERAL_CSTRING("no-cache"), false);
> -    newHttpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Pragma"),
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +    rv = newHttpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Pragma"),

either ignore rv or log the fail. no need to return

::: netwerk/base/nsNetUtil.cpp:641
(Diff revision 1)
>  
>    NS_ENSURE_SUCCESS(rv, rv);
>    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
>    if (httpChannel) {
> -    httpChannel->SetReferrer(aReferrer);
> +      rv = httpChannel->SetReferrer(aReferrer);
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

either ignore rv or log the fail. no need to return or assert

::: netwerk/base/nsPACMan.cpp:47
(Diff revision 1)
>    bool result = true;  // default to assuming success
>  
>    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(request);
> -  if (httpChannel)
> -    httpChannel->GetRequestSucceeded(&result);
> +  if (httpChannel) {
> +    DebugOnly<nsresult> rv = httpChannel->GetRequestSucceeded(&result);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

no need to assert success here. its designed failsafe. you can reassign result to true on failure if you like
Attachment #8821120 - Flags: review?(mcmanus) → review-
(In reply to Olli Pettay [:smaug] from comment #29)
> Valentin, could you explain what NullHttpChannel is and whether it may be
> used outside Necko?
> Since if it maybe used somehow, then these patches will start to assert a
> lot.

I added NullHttpChannel to be used in bug 1113676, but I never got around to landing that.
It's supposed to be a dummy implementation of the nsIHttpChannel, carrying the timing information, and only that.
As far as I can tell, it's only currently being used in NullHttpTransaction which does speculative connections.
Flags: needinfo?(valentin.gosu)
So it cannot "leak" outside NullHttpTransaction to be used elsewhere in Gecko?
Flags: needinfo?(valentin.gosu)
or are the plans to use it outside NullHttpTransaction?
It is passed to nsIHttpActivityObserver.observeActivity(channel,...)
but we don't call any failing methods on the channel from what I can tell.
Comment on attachment 8821129 [details]
Bug 1310127 - Part 15: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100504/#review101608
Attachment #8821129 - Flags: review+
Comment on attachment 8821130 [details]
Bug 1310127 - Part 16: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100506/#review101610

::: uriloader/base/nsURILoader.cpp:540
(Diff revision 1)
>    // error page.  If we got one, we don't want to handle it with a helper app,
>    // really.
>    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(request));
>    if (httpChannel) {
>      bool requestSucceeded;
> -    httpChannel->GetRequestSucceeded(&requestSucceeded);
> +    rv = httpChannel->GetRequestSucceeded(&requestSucceeded);

Why this potential change to the behavior?
Either explain why NS_FAILED here is ok or just use Unused and not rv =.

Though, hmm, looks like the behavior has been random so far, since requestSucceeded isn't necessarily ever initialized. So, ok, NS_FAILED should be fine, but please still re-read all the GetRequestSucceeded implementations to ensure that.
Attachment #8821130 - Flags: review+
Comment on attachment 8821131 [details]
Bug 1310127 - Part 17: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100508/#review101612
Attachment #8821131 - Flags: review+
Attachment #8821122 - Flags: review?(gps) → review?(mozilla)
Comment on attachment 8821122 [details]
Bug 1310127 - Part 8: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100490/#review101688

As sane as this looks, I'm not a peer of this code, so I'm redirecting review.
Comment on attachment 8821125 [details]
Bug 1310127 - Part 11: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100496/#review101726
Attachment #8821125 - Flags: review?(aklotz) → review+
Comment on attachment 8821130 [details]
Bug 1310127 - Part 16: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100506/#review101610

> Why this potential change to the behavior?
> Either explain why NS_FAILED here is ok or just use Unused and not rv =.
> 
> Though, hmm, looks like the behavior has been random so far, since requestSucceeded isn't necessarily ever initialized. So, ok, NS_FAILED should be fine, but please still re-read all the GetRequestSucceeded implementations to ensure that.

There are three implementations:

nsViewSourceChannel::GetRequestSucceeded
and
HttpBaseChannel::GetRequestSucceeded
will not change the input for any error.

NullHttpChannel::GetRequestSucceeded
just leaves the input unchanged.

If requestSucceeded has been initialized to false, then we can just ignore the return value of GetRequestSucceeded.
Attachment #8821559 - Flags: review?(wpan) → review+
Comment on attachment 8822331 [details]
Bug 1310127 - Part 10: Handle netwerk/protocol/http MOZ_MUST_USE functions in PSM.

https://reviewboard.mozilla.org/r/101270/#review101778
Attachment #8822331 - Flags: review?(wpan) → review+
Comment on attachment 8821122 [details]
Bug 1310127 - Part 8: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100490/#review102266
Attachment #8821122 - Flags: review?(mozilla) → review+
Comment on attachment 8821124 [details]
Bug 1310127 - Part 2: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100494/#review102278
Attachment #8821124 - Flags: review?(mcmanus) → review+
Comment on attachment 8821120 [details]
Bug 1310127 - Part 1: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100486/#review102280
Attachment #8821120 - Flags: review?(mcmanus) → review+
Comment on attachment 8822329 [details]
Bug 1310127 - Part 3: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/101266/#review103548

please change the calls to resumerecv, forcerecv, resumesend, and forcesend to be unused rather than assert.. they cant be void as there are a couple places where the error code breaks a loop, but in general they can be ignored if they fail.

also it seems like there is no way this would have passed debug level try.. has it?

some of the places I said log-it should probably just be unused instead.. use your judgment - is the string in the binary worth it?

in general I prefer to use ASSERT around inputs, rather than returns.. especially across interfaces. So I've changed a lot of these

::: netwerk/protocol/http/Http2Session.cpp:395
(Diff revision 1)
>      if (trans && !trans->GetPushedStream()) {
>        LOG3(("Http2Session::AddStream %p atrans=%p trans=%p session unusable - resched.\n",
>              this, aHttpTransaction, trans));
>        aHttpTransaction->SetConnection(nullptr);
> -      gHttpHandler->InitiateTransaction(trans, trans->Priority());
> +      DebugOnly<nsresult> rv = gHttpHandler->InitiateTransaction(trans, trans->Priority());
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead.. this can happen during shutdown

::: netwerk/protocol/http/Http2Session.cpp:1744
(Diff revision 1)
>  
>    // Fake the request side of the pushed HTTP transaction. Sets up hash
>    // key and origin
>    uint32_t notUsed;
> -  pushedStream->ReadSegments(nullptr, 1, &notUsed);
> +  rv = pushedStream->ReadSegments(nullptr, 1, &notUsed);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

change this to unused

::: netwerk/protocol/http/Http2Session.cpp:3471
(Diff revision 1)
>      // this on the tunnel connection with the specific ci. If that can't
>      // happen the cmgr checks with us via MaybeReTunnel() to see if it should
>      // make a new tunnel or just wait longer.
>      LOG3(("Http2Session::DispatchOnTunnel %p trans=%p queue in connection manager",
>            this, trans));
> -    gHttpHandler->InitiateTransaction(trans, trans->Priority());
> +    DebugOnly<nsresult> rv = gHttpHandler->InitiateTransaction(trans, trans->Priority());

just log it.. shutdown can make ths happen

::: netwerk/protocol/http/Http2Session.cpp:3491
(Diff revision 1)
>    }
>  
>    if (mClosed || mShouldGoAway) {
>      LOG(("Http2Session::MaybeReTunnel %p %p session closed - requeue\n", this, trans));
>      trans->SetTunnelProvider(nullptr);
> -    gHttpHandler->InitiateTransaction(trans, trans->Priority());
> +    DebugOnly<nsresult> rv = gHttpHandler->InitiateTransaction(trans, trans->Priority());

log it instead

::: netwerk/protocol/http/Http2Stream.cpp:418
(Diff revision 1)
>    mRequestHeadersDone = 1;
>  
>    nsAutoCString authorityHeader;
>    nsAutoCString hashkey;
> -  head->GetHeader(nsHttp::Host, authorityHeader);
> +  DebugOnly<nsresult> rv = head->GetHeader(nsHttp::Host, authorityHeader);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

this one really should have been checked. thanks for the catch.

can you change it to
if (NS_FAILED(rv)) {
 MOZ_ASSERT(false);
 return rv;
 }

::: netwerk/protocol/http/Http2Stream.cpp:519
(Diff revision 1)
>    // of HTTP/2 headers by writing to mTxInlineFrame{sz}
>  
>    nsCString compressedData;
>    nsAutoCString authorityHeader;
> -  head->GetHeader(nsHttp::Host, authorityHeader);
> +  DebugOnly<nsresult> rv = head->GetHeader(nsHttp::Host, authorityHeader);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

same thing with this one.. both assert and return failure on getheader() failure.

::: netwerk/protocol/http/Http2Stream.cpp:550
(Diff revision 1)
> -                                            path,
> +                                                 path,
> -                                            authorityHeader,
> +                                                 authorityHeader,
> -                                            scheme,
> +                                                 scheme,
> -                                            head->IsConnect(),
> +                                                 head->IsConnect(),
> -                                            compressedData);
> +                                                 compressedData);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

NS_ENSURE_SUCCESS(rv, rv) for this one

::: netwerk/protocol/http/HttpBaseChannel.cpp:2419
(Diff revision 1)
>  {
>    if (!mResponseHead)
>      return NS_ERROR_NOT_AVAILABLE;
>    uint32_t lastMod;
> -  mResponseHead->GetLastModifiedValue(&lastMod);
> +  DebugOnly<nsresult> rv = mResponseHead->GetLastModifiedValue(&lastMod);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

this is a bug.. it should be NS_ENSURE_SUCCESS(rv,rv)

::: netwerk/protocol/http/HttpChannelChild.cpp:1910
(Diff revision 1)
>      if (RemoteChannelExists()) {
>        SendResume();
>      }
>      if (mCallOnResume) {
> -      AsyncCall(mCallOnResume);
> +      rv = AsyncCall(mCallOnResume);
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

should be NS_ENSURE_SUCESS(rv,rv) instaed

::: netwerk/protocol/http/HttpChannelChild.cpp:2008
(Diff revision 1)
>    if (mCanceled) {
>      // We may have been canceled already, either by on-modify-request
>      // listeners or by load group observers; in that case, don't create IPDL
>      // connection. See nsHttpChannel::AsyncOpen().
> -    AsyncAbort(mStatus);
> +    rv = AsyncAbort(mStatus);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

just make unused

::: netwerk/protocol/http/HttpChannelChild.cpp:2847
(Diff revision 1)
>  
>    // Continue with the original cross-process request
>    nsresult rv = ContinueAsyncOpen();
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> -    AsyncAbort(rv);
> +    rv = AsyncAbort(rv);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

just make unused

::: netwerk/protocol/http/HttpChannelChild.cpp:2881
(Diff revision 1)
>      mShouldInterceptSubsequentRedirect = true;
>      // Continue with the original cross-process request
>      nsresult rv = ContinueAsyncOpen();
>      if (NS_WARN_IF(NS_FAILED(rv))) {
> -      AsyncAbort(rv);
> +      rv = AsyncAbort(rv);
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

just make unused

::: netwerk/protocol/http/HttpChannelParent.cpp:1648
(Diff revision 1)
>    // mDivertListener.
>    nsCOMPtr<nsIStreamListener> converterListener;
> +  DebugOnly<nsresult> rv =
> -  mChannel->DoApplyContentConversions(mDivertListener,
> +    mChannel->DoApplyContentConversions(mDivertListener,
> -                                      getter_AddRefs(converterListener));
> +                                        getter_AddRefs(converterListener));
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

okay to mark this one unused.. converterListener out param is the real key

::: netwerk/protocol/http/NullHttpChannel.cpp:32
(Diff revision 1)
>    ssm->GetChannelURIPrincipal(chan, getter_AddRefs(mResourcePrincipal));
>  
> +  DebugOnly<nsresult> rv =
> -  chan->GetResponseHeader(NS_LITERAL_CSTRING("Timing-Allow-Origin"),
> +    chan->GetResponseHeader(NS_LITERAL_CSTRING("Timing-Allow-Origin"),
> -                          mTimingAllowOriginHeader);
> +                            mTimingAllowOriginHeader);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

unused

::: netwerk/protocol/http/NullHttpTransaction.cpp:76
(Diff revision 1)
>        mActivityType,
>        mActivitySubtype,
>        mTimestamp,
>        mExtraSizeData,
>        mExtraStringData);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

NS_ENSURE_SUCCESS(rv,rv)

::: netwerk/protocol/http/TunnelUtils.cpp:397
(Diff revision 1)
>      // fatal handshake failure
>      LOG(("TLSFilterTransaction %p Fatal Handshake Failure: %d\n", this, PR_GetError()));
>      return NS_ERROR_FAILURE;
>    }
>  
> -  OnReadSegment("", 0, &notUsed);
> +  DebugOnly<nsresult> rv = OnReadSegment("", 0, &notUsed);

unused

::: netwerk/protocol/http/TunnelUtils.cpp:1036
(Diff revision 1)
>    if (mDrivingTransaction) {
>      // requeue it I guess. This should be gone.
> +    DebugOnly<nsresult> rv =
> -    gHttpHandler->InitiateTransaction(mDrivingTransaction,
> +      gHttpHandler->InitiateTransaction(mDrivingTransaction,
> -                                      mDrivingTransaction->Priority());
> +                                        mDrivingTransaction->Priority());
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

unusued.. shutdown races

::: netwerk/protocol/http/TunnelUtils.cpp:1097
(Diff revision 1)
>    mDrivingTransaction->MakeSticky();
>  
>    // jump the priority and start the dispatcher
> -  gHttpHandler->InitiateTransaction(
> +  rv = gHttpHandler->InitiateTransaction(
>      mDrivingTransaction, nsISupportsPriority::PRIORITY_HIGHEST - 60);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

unused shutdown issues

::: netwerk/protocol/http/TunnelUtils.cpp:1191
(Diff revision 1)
>      mTunneledConn->DontReuse();
>      return NS_OK;
>    }
>  
>    *countRead = 0;
> -  Flush(count, countRead);
> +  rv = Flush(count, countRead);

unused

::: netwerk/protocol/http/nsCORSListenerProxy.cpp:1409
(Diff revision 1)
>  
>    nsAutoCString method;
>    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aRequestChannel));
>    NS_ENSURE_TRUE(httpChannel, NS_ERROR_UNEXPECTED);
> -  httpChannel->GetRequestMethod(method);
> +  nsresult rv = httpChannel->GetRequestMethod(method);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

either ignore or log this one. it could fail, but that will be detected by the value of method later

::: netwerk/protocol/http/nsHttpAuthCache.cpp:593
(Diff revision 1)
>          mList.InsertElementAt(0, entry);
>      }
>      else {
>          // update the entry...
> -        entry->Set(path, realm, creds, challenge, ident, metadata);
> +        DebugOnly<nsresult> rv = entry->Set(path, realm, creds, challenge, ident, metadata);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

NS_ENSURE_SUCCESS(rv,rv)

::: netwerk/protocol/http/nsHttpChannel.cpp:492
(Diff revision 1)
>              nsRunnableMethod<nsHttpChannel> *event = nullptr;
> +            nsresult rv;
>              if (!mCachedContentIsPartial) {
> -                AsyncCall(&nsHttpChannel::AsyncOnExamineCachedResponse, &event);
> +                rv = AsyncCall(&nsHttpChannel::AsyncOnExamineCachedResponse,
> +                               &event);
> +                MOZ_ASSERT(NS_SUCCEEDED(rv));

I'm honestly not sure about this one. I would change it to a log to be safe.

::: netwerk/protocol/http/nsHttpChannel.cpp:580
(Diff revision 1)
>      if (!callbacks)
>          return;
>  
> -    gHttpHandler->SpeculativeConnect(
> +    DebugOnly<nsresult> rv = gHttpHandler->SpeculativeConnect(
>          mConnectionInfo, callbacks, mCaps & NS_HTTP_DISALLOW_SPDY);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

unused

::: netwerk/protocol/http/nsHttpChannel.cpp:1488
(Diff revision 1)
>      LOG(("Cancelling failed proxy CONNECT [this=%p httpStatus=%u]\n",
>           this, httpStatus));
>      Cancel(rv);
> -    CallOnStartRequest();
> +    {
> +      DebugOnly<nsresult> rv = CallOnStartRequest();
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

no.. just log this one - that would be a big mistake.

::: netwerk/protocol/http/nsHttpChannel.cpp:1622
(Diff revision 1)
>                      consoleErrorCategory = NS_LITERAL_STRING("Invalid HPKP Headers");
>                      break;
>                  default:
>                      return NS_ERROR_FAILURE;
>              }
> -            AddSecurityMessage(consoleErrorTag, consoleErrorCategory);
> +            rv = AddSecurityMessage(consoleErrorTag, consoleErrorCategory);

just ignore it

::: netwerk/protocol/http/nsHttpChannel.cpp:1838
(Diff revision 1)
>          if (state & nsIWebProgressListener::STATE_USES_WEAK_CRYPTO) {
>              nsString consoleErrorTag = NS_LITERAL_STRING("WeakCipherSuiteWarning");
>              nsString consoleErrorCategory = NS_LITERAL_STRING("SSL");
> +            DebugOnly<nsresult> rv =
> -            AddSecurityMessage(consoleErrorTag, consoleErrorCategory);
> +                AddSecurityMessage(consoleErrorTag, consoleErrorCategory);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/http/nsHttpChannel.cpp:1862
(Diff revision 1)
>                  nsString consoleErrorTag = NS_LITERAL_STRING("SHA1Sig");
>                  nsString consoleErrorMessage
>                          = NS_LITERAL_STRING("SHA-1 Signature");
> +                DebugOnly<nsresult> rv =
> -                AddSecurityMessage(consoleErrorTag, consoleErrorMessage);
> +                    AddSecurityMessage(consoleErrorTag, consoleErrorMessage);
> +                MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/http/nsHttpChannel.cpp:2088
(Diff revision 1)
>  
>      // handle unused username and password in url (see bug 232567)
>      if (httpStatus != 401 && httpStatus != 407) {
> -        if (!mAuthRetryPending)
> -            mAuthProvider->CheckForSuperfluousAuth();
> +        if (!mAuthRetryPending) {
> +            rv = mAuthProvider->CheckForSuperfluousAuth();
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

just log it

::: netwerk/protocol/http/nsHttpChannel.cpp:2096
(Diff revision 1)
>              return CallOnStartRequest();
>  
>          // reset the authentication's current continuation state because our
>          // last authentication attempt has been completed successfully
> -        mAuthProvider->Disconnect(NS_ERROR_ABORT);
> +        rv = mAuthProvider->Disconnect(NS_ERROR_ABORT);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

just log it

::: netwerk/protocol/http/nsHttpChannel.cpp:2254
(Diff revision 1)
>              LOG(("ProcessAuthentication failed [rv=%x]\n", rv));
>              if (mTransaction->ProxyConnectFailed())
>                  return ProcessFailedProxyConnect(httpStatus);
> -            if (!mAuthRetryPending)
> -                mAuthProvider->CheckForSuperfluousAuth();
> +            if (!mAuthRetryPending) {
> +                rv = mAuthProvider->CheckForSuperfluousAuth();
> +                MOZ_ASSERT(NS_SUCCEEDED(rv));

just log it

::: netwerk/protocol/http/nsHttpChannel.cpp:2338
(Diff revision 1)
>          }
>          CloseCacheEntry(false);
>  
>          if (mApplicationCacheForWrite) {
>              // Store response in the offline cache
> -            InitOfflineCacheEntry();
> +            rv = InitOfflineCacheEntry();

ask :mayhemer what to do here

::: netwerk/protocol/http/nsHttpChannel.cpp:2520
(Diff revision 1)
>      }
>  
>      nsresult rv = StartRedirectChannelToHttps();
> -    if (NS_FAILED(rv))
> -        ContinueAsyncRedirectChannelToURI(rv);
> +    if (NS_FAILED(rv)) {
> +        rv = ContinueAsyncRedirectChannelToURI(rv);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:2554
(Diff revision 1)
>  
>      nsresult rv = StartRedirectChannelToURI(mAPIRedirectToURI,
>                                              nsIChannelEventSink::REDIRECT_PERMANENT);
> -    if (NS_FAILED(rv))
> -        ContinueAsyncRedirectChannelToURI(rv);
> +    if (NS_FAILED(rv)) {
> +        rv = ContinueAsyncRedirectChannelToURI(rv);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:2951
(Diff revision 1)
>              nsAutoString message
>                  (NS_LITERAL_STRING("Failed Assoc-Req. Received "));
>              nsAutoCString assoc_req;
> +            DebugOnly<nsresult> rv =
> -            mResponseHead->GetHeader(nsHttp::Assoc_Req, assoc_req);
> +                mResponseHead->GetHeader(nsHttp::Assoc_Req, assoc_req);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/http/nsHttpChannel.cpp:2986
(Diff revision 1)
>              nsAutoString message
>                  (NS_LITERAL_STRING("Failed Assoc-Req. Received "));
>              nsAutoCString assoc_req;
> +            DebugOnly<nsresult> rv =
> -            mResponseHead->GetHeader(nsHttp::Assoc_Req, assoc_req);
> +                mResponseHead->GetHeader(nsHttp::Assoc_Req, assoc_req);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/http/nsHttpChannel.cpp:4170
(Diff revision 1)
>              } else {
>                  nsAutoCString val;
>                  // Add If-Modified-Since header if a Last-Modified was given
>                  // and we are allowed to do this (see bugs 510359 and 269303)
>                  if (canAddImsHeader) {
> -                    mCachedResponseHead->GetHeader(nsHttp::Last_Modified, val);
> +                    rv = mCachedResponseHead->GetHeader(nsHttp::Last_Modified, val);

ignore the get().. it throws often

::: netwerk/protocol/http/nsHttpChannel.cpp:4242
(Diff revision 1)
>  
>      rv = OnCacheEntryAvailableInternal(entry, aNew, aAppCache, status);
>      if (NS_FAILED(rv)) {
>          CloseCacheEntry(false);
> -        AsyncAbort(rv);
> +        DebugOnly<nsresult> rv2 = AsyncAbort(rv);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv2));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:6036
(Diff revision 1)
>      }
>  
>      // check to see if authorization headers should be included
>      // mCustomAuthHeader is set in AsyncOpen if we find Authorization header
> -    mAuthProvider->AddAuthorizationHeaders(mCustomAuthHeader);
> +    rv = mAuthProvider->AddAuthorizationHeaders(mCustomAuthHeader);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:6094
(Diff revision 1)
>              mCaps |= NS_HTTP_ALLOW_PIPELINING;
>      }
>  
>      // if this somehow fails we can go on without it
> -    gHttpHandler->AddConnectionHeader(&mRequestHead, mCaps);
> +    rv = gHttpHandler->AddConnectionHeader(&mRequestHead, mCaps);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:6254
(Diff revision 1)
>          return NS_OK;
>      mPriority = newValue;
> -    if (mTransaction)
> +    if (mTransaction) {
> +        DebugOnly<nsresult> rv =
> -        gHttpHandler->RescheduleTransaction(mTransaction, mPriority);
> +            gHttpHandler->RescheduleTransaction(mTransaction, mPriority);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:6300
(Diff revision 1)
>  nsHttpChannel::ContinueBeginConnect()
>  {
>      nsresult rv = ContinueBeginConnectWithResult();
>      if (NS_FAILED(rv)) {
>          CloseCacheEntry(false);
> -        AsyncAbort(rv);
> +        rv = AsyncAbort(rv);

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:6363
(Diff revision 1)
>      }
>  
>      if (NS_FAILED(rv)) {
>          CloseCacheEntry(false);
> -        AsyncAbort(rv);
> +        DebugOnly<nsresult> rv2 = AsyncAbort(rv);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv2));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:6760
(Diff revision 1)
>                  NS_NOTREACHED("unexpected request");
>          }
>          // Do not to leave the transaction in a suspended state in error cases.
> -        if (NS_FAILED(status) && mTransaction)
> -            gHttpHandler->CancelTransaction(mTransaction, status);
> +        if (NS_FAILED(status) && mTransaction) {
> +            DebugOnly<nsresult> rv = gHttpHandler->CancelTransaction(mTransaction, status);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:6935
(Diff revision 1)
>          // New implementation just returns value of the !mCacheEntryIsReadOnly flag passed in.
>          // Old implementation checks on nsICache::ACCESS_WRITE flag.
>          mCacheEntry->HasWriteAccess(!mCacheEntryIsReadOnly, &writeAccess);
>          if (writeAccess) {
> -            FinalizeCacheEntry();
> +            DebugOnly<nsresult> rv = FinalizeCacheEntry();
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead

::: netwerk/protocol/http/nsHttpChannel.cpp:8124
(Diff revision 1)
>      mIsCorsPreflightDone = 1;
>      mPreflightChannel = nullptr;
>  
>      CloseCacheEntry(false);
> -    AsyncAbort(aError);
> +    DebugOnly<nsresult> rv = AsyncAbort(aError);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

log it instead

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp:288
(Diff revision 1)
>      // an attempt to spoof a different site (see bug 232567).
>      if (!ConfirmAuth(NS_LITERAL_STRING("SuperfluousAuth"), true)) {
>          // calling cancel here sets our mStatus and aborts the HTTP
>          // transaction, which prevents OnDataAvailable events.
> -        mAuthChannel->Cancel(NS_ERROR_ABORT);
> +        DebugOnly<nsresult> rv = mAuthChannel->Cancel(NS_ERROR_ABORT);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

unused

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp:804
(Diff revision 1)
>          // If the flag is set and identity is invalid, it means we received the first
>          // challange for a new negotiation round after negotiating a connection based
>          // auth failed (invalid password).
>          // The mConnectionBased flag is set later for the newly received challenge,
>          // so here it reflects the previous 401/7 response schema.
> -        mAuthChannel->CloseStickyConnection();
> +        rv = mAuthChannel->CloseStickyConnection();

please get mayhemer to review this file (perhaps a new patch)

::: netwerk/protocol/http/nsHttpChunkedDecoder.cpp:121
(Diff revision 1)
>                  // allocate a header array for the trailers on demand
>                  if (!mTrailers) {
>                      mTrailers = new nsHttpHeaderArray();
>                  }
> -                mTrailers->ParseHeaderLine(nsDependentCSubstring(buf, count));
> +                DebugOnly<nsresult> rv = mTrailers->ParseHeaderLine(nsDependentCSubstring(buf, count));
> +                MOZ_ASSERT(NS_SUCCEEDED(rv));

unused

::: netwerk/protocol/http/nsHttpConnection.cpp:276
(Diff revision 1)
>  
>      if (!mTLSFilter) {
>          mTransaction = mSpdySession;
>      } else {
> -        mTLSFilter->SetProxiedTransaction(mSpdySession);
> +        rv = mTLSFilter->SetProxiedTransaction(mSpdySession);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log instead

::: netwerk/protocol/http/nsHttpConnection.cpp:468
(Diff revision 1)
>      if (trans != mTLSFilter) {
>          return;
>      }
>      LOG(("nsHttpConnection::OnTunnelNudged %p Calling OnSocketWritable\n", this));
> -    OnSocketWritable();
> +    DebugOnly<nsresult> rv = OnSocketWritable();
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

unused

::: netwerk/protocol/http/nsHttpConnection.cpp:558
(Diff revision 1)
>               this, rv));
>      }
>  
>      if (mTLSFilter) {
> -        mTLSFilter->SetProxiedTransaction(trans);
> +        rv = mTLSFilter->SetProxiedTransaction(trans);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

NS_ENSURE_SUCCESS(rv,rv)

::: netwerk/protocol/http/nsHttpConnection.cpp:2221
(Diff revision 1)
>          // our normal timers or protocol stack are the place to deal with
>          // any exception logic.
>  
>          if (!CanReuse()) {
>              LOG(("Server initiated close of idle conn %p\n", this));
> -            gHttpHandler->ConnMgr()->CloseIdleConnection(this);
> +            DebugOnly<nsresult> rv = gHttpHandler->ConnMgr()->CloseIdleConnection(this);

unused

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:228
(Diff revision 1)
>  nsHttpConnectionMgr::PostEvent(nsConnEventHandler handler,
>                                 int32_t iparam, ARefBase *vparam)
>  {
> -    EnsureSocketThreadTarget();
> +    nsresult rv;
> +
> +    rv = EnsureSocketThreadTarget();

unused.. shutdown races

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:313
(Diff revision 1)
>  
>      if (0 == strcmp(topic, NS_TIMER_CALLBACK_TOPIC)) {
>          nsCOMPtr<nsITimer> timer = do_QueryInterface(subject);
> +        DebugOnly<nsresult> rv;
>          if (timer == mTimer) {
> -            PruneDeadConnections();
> +            rv = PruneDeadConnections();

unused

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:319
(Diff revision 1)
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));
>          }
>          else if (timer == mTimeoutTick) {
>              TimeoutTick();
>          } else if (timer == mTrafficTimer) {
> -            PruneNoTraffic();
> +            rv = PruneNoTraffic();

unused

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:464
(Diff revision 1)
>  }
>  
>  nsresult
>  nsHttpConnectionMgr::GetSocketThreadTarget(nsIEventTarget **target)
>  {
> -    EnsureSocketThreadTarget();
> +    DebugOnly<nsresult> rv = EnsureSocketThreadTarget();

unused

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:1511
(Diff revision 1)
>          if (conn) {
>              if ((caps & NS_HTTP_ALLOW_KEEPALIVE) || !conn->IsExperienced()) {
>                  LOG(("   dispatch to spdy: [conn=%p]\n", conn.get()));
>                  trans->RemoveDispatchedAsBlocking();  /* just in case */
> -                DispatchTransaction(ent, trans, conn);
> +                DebugOnly<nsresult> rv = DispatchTransaction(ent, trans, conn);
> +                MOZ_ASSERT(NS_SUCCEEDED(rv));

ns_ensure_success(rv,rv)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:1609
(Diff revision 1)
>          if (conn) {
>              // This will update the class of the connection to be the class of
>              // the transaction dispatched on it.
>              AddActiveConn(conn, ent);
> -            DispatchTransaction(ent, trans, conn);
> +            DebugOnly<nsresult> rv = DispatchTransaction(ent, trans, conn);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

ns_ensure_success(rv,rv)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:1850
(Diff revision 1)
>  
>      /* the first transaction can go in unconditionally - 1 transaction
>         on a nsHttpPipeline object is not a real HTTP pipeline */
>  
>      RefPtr<nsHttpPipeline> pipeline = new nsHttpPipeline();
> -    pipeline->AddTransaction(firstTrans);
> +    DebugOnly<nsresult> rv = pipeline->AddTransaction(firstTrans);

ns_ensure_success(rv,rv)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:2691
(Diff revision 1)
>  
> -    if (NS_SUCCEEDED(rv))
> -        data->mUpgradeListener->OnTransportAvailable(socketTransport,
> +    if (NS_SUCCEEDED(rv)) {
> +      rv = data->mUpgradeListener->OnTransportAvailable(socketTransport,
> -                                                     socketIn,
> +                                                        socketIn,
> -                                                     socketOut);
> +                                                        socketOut);
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpHandler.cpp:2196
(Diff revision 1)
>  
>          mHandlerActive = false;
>  
>          // clear cache of all authentication credentials.
> -        mAuthCache.ClearAll();
> -        mPrivateAuthCache.ClearAll();
> +        rv = mAuthCache.ClearAll();
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpHandler.cpp:2198
(Diff revision 1)
>  
>          // clear cache of all authentication credentials.
> -        mAuthCache.ClearAll();
> -        mPrivateAuthCache.ClearAll();
> +        rv = mAuthCache.ClearAll();
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));
> +        rv = mPrivateAuthCache.ClearAll();
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpHandler.cpp:2222
(Diff revision 1)
>          // initialize connection manager
> -        InitConnectionMgr();
> +        rv = InitConnectionMgr();
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));
>      } else if (!strcmp(topic, "net:clear-active-logins")) {
> -        mAuthCache.ClearAll();
> -        mPrivateAuthCache.ClearAll();
> +        rv = mAuthCache.ClearAll();
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpHandler.cpp:2224
(Diff revision 1)
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));
>      } else if (!strcmp(topic, "net:clear-active-logins")) {
> -        mAuthCache.ClearAll();
> -        mPrivateAuthCache.ClearAll();
> +        rv = mAuthCache.ClearAll();
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));
> +        rv = mPrivateAuthCache.ClearAll();
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpHandler.cpp:2250
(Diff revision 1)
>          if (uri && mConnMgr) {
>              mConnMgr->ReportFailedToProcess(uri);
>          }
>      } else if (!strcmp(topic, "last-pb-context-exited")) {
> -        mPrivateAuthCache.ClearAll();
> +        rv = mPrivateAuthCache.ClearAll();
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpPipeline.cpp:104
(Diff revision 1)
>      uint32_t qlen = PipelineDepth();
> +    DebugOnly<nsresult> rv;
>  
>      if (qlen != 1) {
> -        trans->SetPipelinePosition(qlen);
> +        rv = trans->SetPipelinePosition(qlen);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/http/nsHttpPipeline.cpp:110
(Diff revision 1)
>      }
>      else {
>          // do it for this case in case an idempotent cancellation
>          // is being repeated and an old value needs to be cleared
> -        trans->SetPipelinePosition(0);
> +        rv = trans->SetPipelinePosition(0);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/http/nsHttpPipeline.cpp:878
(Diff revision 1)
>              // response queue to reflect that if necessary. We are now sending
>              // out a request while we haven't received all responses.
>              nsAHttpTransaction *response = Response(0);
> -            if (response && !response->PipelinePosition())
> -                response->SetPipelinePosition(1);
> +            if (response && !response->PipelinePosition()) {
> +                rv = response->SetPipelinePosition(1);
> +                MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/http/nsHttpResponseHead.cpp:300
(Diff revision 1)
>  
>          p = PL_strstr(block, "\r\n");
>          if (!p)
>              return NS_ERROR_UNEXPECTED;
>  
> -        ParseHeaderLine_locked(nsDependentCSubstring(block, p - block), false);
> +        DebugOnly<nsresult> rv = ParseHeaderLine_locked(nsDependentCSubstring(block, p - block), false);

ignore it

::: netwerk/protocol/http/nsHttpTransaction.cpp:333
(Diff revision 1)
>              mChannel,
>              NS_HTTP_ACTIVITY_TYPE_HTTP_TRANSACTION,
>              NS_HTTP_ACTIVITY_SUBTYPE_REQUEST_HEADER,
>              PR_Now(), 0,
>              mReqHeaderBuf);
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpTransaction.cpp:584
(Diff revision 1)
> -            mActivityDistributor->ObserveActivity(
> +            DebugOnly<nsresult> rv = mActivityDistributor->ObserveActivity(
>                  mChannel,
>                  NS_HTTP_ACTIVITY_TYPE_HTTP_TRANSACTION,
>                  NS_HTTP_ACTIVITY_SUBTYPE_REQUEST_BODY_SENT,
>                  PR_Now(), 0, EmptyCString());
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpTransaction.cpp:596
(Diff revision 1)
>                  NS_HTTP_ACTIVITY_TYPE_SOCKET_TRANSPORT,
>                  static_cast<uint32_t>(status),
>                  PR_Now(),
>                  progress,
>                  EmptyCString());
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpTransaction.cpp:908
(Diff revision 1)
>                  NS_HTTP_ACTIVITY_TYPE_HTTP_TRANSACTION,
>                  NS_HTTP_ACTIVITY_SUBTYPE_RESPONSE_COMPLETE,
>                  PR_Now(),
>                  static_cast<uint64_t>(mContentRead),
>                  EmptyCString());
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpTransaction.cpp:917
(Diff revision 1)
> -        mActivityDistributor->ObserveActivity(
> +        DebugOnly<nsresult> rv = mActivityDistributor->ObserveActivity(
>              mChannel,
>              NS_HTTP_ACTIVITY_TYPE_HTTP_TRANSACTION,
>              NS_HTTP_ACTIVITY_SUBTYPE_TRANSACTION_CLOSE,
>              PR_Now(), 0, EmptyCString());
> +        MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpTransaction.cpp:1062
(Diff revision 1)
>          // sure we parse the remaining header line, and then hopefully, the
>          // response will be usable (see bug 88792).
>          if (!mHaveAllHeaders) {
>              char data = '\n';
>              uint32_t unused;
> -            ParseHead(&data, 1, &unused);
> +            DebugOnly<nsresult> rv = ParseHead(&data, 1, &unused);

ignore it

::: netwerk/protocol/http/nsHttpTransaction.cpp:1273
(Diff revision 1)
>      // disable pipelining for the next attempt in case pipelining caused the
>      // reset.  this is being overly cautious since we don't know if pipelining
>      // was the problem here.
>      mCaps &= ~NS_HTTP_ALLOW_PIPELINING;
> -    SetPipelinePosition(0);
> +    DebugOnly<nsresult> rv = SetPipelinePosition(0);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/http/nsHttpTransaction.cpp:1462
(Diff revision 1)
> -            mActivityDistributor->ObserveActivity(
> +            rv = mActivityDistributor->ObserveActivity(
>                  mChannel,
>                  NS_HTTP_ACTIVITY_TYPE_HTTP_TRANSACTION,
>                  NS_HTTP_ACTIVITY_SUBTYPE_RESPONSE_START,
>                  PR_Now(), 0, EmptyCString());
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpTransaction.cpp:1590
(Diff revision 1)
>          bool reset = false;
> -        if (!mRestartInProgressVerifier.IsSetup())
> -            mConnection->OnHeadersAvailable(this, mRequestHead, mResponseHead, &reset);
> +        if (!mRestartInProgressVerifier.IsSetup()) {
> +            DebugOnly<nsresult> rv =
> +                mConnection->OnHeadersAvailable(this, mRequestHead,
> +                                                mResponseHead, &reset);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

NS_ENSURE_SUCCESS(rv,rv)

::: netwerk/protocol/http/nsHttpTransaction.cpp:1799
(Diff revision 1)
>                  NS_HTTP_ACTIVITY_TYPE_HTTP_TRANSACTION,
>                  NS_HTTP_ACTIVITY_SUBTYPE_RESPONSE_COMPLETE,
>                  PR_Now(),
>                  static_cast<uint64_t>(mContentRead),
>                  EmptyCString());
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpTransaction.cpp:1856
(Diff revision 1)
>                  mChannel,
>                  NS_HTTP_ACTIVITY_TYPE_HTTP_TRANSACTION,
>                  NS_HTTP_ACTIVITY_SUBTYPE_RESPONSE_HEADER,
>                  PR_Now(), 0,
>                  completeResponseHeaders);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

log it

::: netwerk/protocol/http/nsHttpTransaction.cpp:1885
(Diff revision 1)
>          // we may have read more than our share, in which case we must give
>          // the excess bytes back to the connection
>          if (mResponseIsComplete && countRemaining) {
>              MOZ_ASSERT(mConnection);
> -            mConnection->PushBack(buf + *countRead, countRemaining);
> +            rv = mConnection->PushBack(buf + *countRead, countRemaining);
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

NS_ENSURE_SUCCESS(rv.rv)

::: netwerk/protocol/http/nsHttpTransaction.cpp:2219
(Diff revision 1)
>  
>      mSubmittedRatePacing = true;
>      mSynchronousRatePaceRequest = true;
> +    DebugOnly<nsresult> rv =
> -    gHttpHandler->SubmitPacedRequest(this, getter_AddRefs(mTokenBucketCancel));
> +        gHttpHandler->SubmitPacedRequest(this, getter_AddRefs(mTokenBucketCancel));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

ignore it

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp:935
(Diff revision 1)
>      NS_NAMED_LITERAL_CSTRING(contentTypeStr, "Content-Type");
>      nsAutoCString contentType;
>      nsresult rv =
>          mHttpChannel->GetResponseHeader(contentTypeStr, contentType);
> -    if (NS_SUCCEEDED(rv))
> -        aVisitor->VisitHeader(contentTypeStr, contentType);
> +    if (NS_SUCCEEDED(rv)) {
> +        rv = aVisitor->VisitHeader(contentTypeStr, contentType);

just return aVisitor->VisitHeader()

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp:951
(Diff revision 1)
>      nsresult rv = GetResponseHeader(aHeader, value);
>      if (NS_FAILED(rv)) {
>          return rv;
>      }
> -    aVisitor->VisitHeader(aHeader, value);
> +    rv = aVisitor->VisitHeader(aHeader, value);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

return rv but check with :dragana to be sure
Attachment #8822329 - Flags: review?(mcmanus)
Comment on attachment 8822330 [details]
Bug 1310127 - Part 4: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/101268/#review103580

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp:114
(Diff revision 1)
>      mAuthChannel = channel;
>  
>      nsresult rv = mAuthChannel->GetURI(getter_AddRefs(mURI));
>      if (NS_FAILED(rv)) return rv;
>  
> -    mAuthChannel->GetIsSSL(&mUsingSSL);
> +    rv = mAuthChannel->GetIsSSL(&mUsingSSL);

thats a good fix
Attachment #8822330 - Flags: review?(mcmanus) → review+
(In reply to Patrick McManus [:mcmanus] from comment #66)
> Comment on attachment 8822329 [details]
> Bug 1310127 - Part 3: Use MOZ_MUST_USE in netwerk/protocol/http
> 
> https://reviewboard.mozilla.org/r/101266/#review103548
> 
> please change the calls to resumerecv, forcerecv, resumesend, and forcesend
> to be unused rather than assert.. they cant be void as there are a couple
> places where the error code breaks a loop, but in general they can be
> ignored if they fail.

Done. Also changed some AsyncAbort to be unused.

> also it seems like there is no way this would have passed debug level try..
> has it?

There are some orange results for each try push, but they do not seem to related to these patches.
I'll keep tracking it on the most recent m-c.

> some of the places I said log-it should probably just be unused instead..
> use your judgment - is the string in the binary worth it?
> 
> in general I prefer to use ASSERT around inputs, rather than returns..
> especially across interfaces. So I've changed a lot of these
>
Comment on attachment 8821123 [details]
Bug 1310127 - Part 9: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/100492/#review105092
Attachment #8821123 - Flags: review?(hsivonen) → review+
Comment on attachment 8826131 [details]
Bug 1310127 - Part 5: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/104140/#review105154

::: netwerk/protocol/http/nsHttpChannel.cpp:2345
(Diff revision 1)
>          CloseCacheEntry(false);
>  
>          if (mApplicationCacheForWrite) {
>              // Store response in the offline cache
> -            InitOfflineCacheEntry();
> +            rv = InitOfflineCacheEntry();
> +            MOZ_ASSERT(NS_SUCCEEDED(rv));

rather use Unused, otherwise I want to see a green try for this change

(offline application cache is anyway unsupported)

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp:1479
(Diff revision 1)
>      // information to the server (or proxy).  If it doesn't accept our
>      // authentication it'll respond with failure and resend the challenge list
>      mRemainingChallenges.Truncate();
>  
> -    mAuthChannel->OnAuthAvailable();
> +    rv = mAuthChannel->OnAuthAvailable();
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

Unused please
Attachment #8826131 - Flags: review?(honzab.moz) → review+
Comment on attachment 8826132 [details]
Bug 1310127 - Part 6: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/104142/#review106238
Attachment #8826132 - Flags: review?(dd.mozilla) → review+
Flags: needinfo?(valentin.gosu)
Comment on attachment 8822329 [details]
Bug 1310127 - Part 3: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/101266/#review109270

what hapened to the http2stream.cpp changes from the last version? Two of those that I mentioned in my comments actually identified bugs. Please bring them back. other than that I think we're good.

::: netwerk/protocol/http/Http2Stream.cpp:418
(Diff revision 1)
>    mRequestHeadersDone = 1;
>  
>    nsAutoCString authorityHeader;
>    nsAutoCString hashkey;
> -  head->GetHeader(nsHttp::Host, authorityHeader);
> +  DebugOnly<nsresult> rv = head->GetHeader(nsHttp::Host, authorityHeader);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

so it should be checked and the error returned

::: netwerk/protocol/http/Http2Stream.cpp:550
(Diff revision 1)
> -                                            path,
> +                                                 path,
> -                                            authorityHeader,
> +                                                 authorityHeader,
> -                                            scheme,
> +                                                 scheme,
> -                                            head->IsConnect(),
> +                                                 head->IsConnect(),
> -                                            compressedData);
> +                                                 compressedData);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

this one should be checked and the error returned
Attachment #8822329 - Flags: review?(mcmanus) → review-
(In reply to Honza Bambas (:mayhemer) from comment #87)
> Comment on attachment 8826131 [details]
> Bug 1310127 - Part 5: Use MOZ_MUST_USE in netwerk/protocol/http
> 
> https://reviewboard.mozilla.org/r/104140/#review105154
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp:2345
> (Diff revision 1)
> >          CloseCacheEntry(false);
> >  
> >          if (mApplicationCacheForWrite) {
> >              // Store response in the offline cache
> > -            InitOfflineCacheEntry();
> > +            rv = InitOfflineCacheEntry();
> > +            MOZ_ASSERT(NS_SUCCEEDED(rv));
> 
> rather use Unused, otherwise I want to see a green try for this change
> 
> (offline application cache is anyway unsupported)

Didn't see any failed test because of this, but since it is not actually being used, I changed it to mozilla::Unused.

(In reply to Patrick McManus [:mcmanus] from comment #89)
> Comment on attachment 8822329 [details]
> Bug 1310127 - Part 3: Use MOZ_MUST_USE in netwerk/protocol/http
> 
> https://reviewboard.mozilla.org/r/101266/#review109270
> 
> what hapened to the http2stream.cpp changes from the last version? Two of
> those that I mentioned in my comments actually identified bugs. Please bring
> them back. other than that I think we're good.
> 

I'm sorry about the confusion.
Those changes have been moved to part 4, because they changed control flows.
Comment on attachment 8822329 [details]
Bug 1310127 - Part 3: Use MOZ_MUST_USE in netwerk/protocol/http

https://reviewboard.mozilla.org/r/101266/#review115618
Attachment #8822329 - Flags: review?(mcmanus) → review+
Removed pipeline related code because bug 1340655.
Pushed by wpan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c78d25d4024d
Part 1: Use MOZ_MUST_USE in netwerk/protocol/http r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/2b04c29e7c67
Part 2: Use MOZ_MUST_USE in netwerk/protocol/http r=Cykesiopka,mcmanus
https://hg.mozilla.org/integration/autoland/rev/3367be623273
Part 3: Use MOZ_MUST_USE in netwerk/protocol/http r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/6305df95b90a
Part 4: Use MOZ_MUST_USE in netwerk/protocol/http r=mcmanus
https://hg.mozilla.org/integration/autoland/rev/03ddf10ea39e
Part 5: Use MOZ_MUST_USE in netwerk/protocol/http r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/77322ec67208
Part 6: Use MOZ_MUST_USE in netwerk/protocol/http r=dragana
https://hg.mozilla.org/integration/autoland/rev/e7ee73a65e05
Part 7: Use MOZ_MUST_USE in netwerk/protocol/http r=jst
https://hg.mozilla.org/integration/autoland/rev/d574affa110c
Part 8: Use MOZ_MUST_USE in netwerk/protocol/http r=mkaply
https://hg.mozilla.org/integration/autoland/rev/bd6e0e3fa619
Part 9: Use MOZ_MUST_USE in netwerk/protocol/http r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/685cc7384101
Part 10: Handle netwerk/protocol/http MOZ_MUST_USE functions in PSM. r=wcpan
https://hg.mozilla.org/integration/autoland/rev/614c0b551ab7
Part 11: Use MOZ_MUST_USE in netwerk/protocol/http r=aklotz
https://hg.mozilla.org/integration/autoland/rev/084db32e36cd
Part 12: Use MOZ_MUST_USE in netwerk/protocol/http r=mak
https://hg.mozilla.org/integration/autoland/rev/8982e9798695
Part 13: Use MOZ_MUST_USE in netwerk/protocol/http r=heycam
https://hg.mozilla.org/integration/autoland/rev/efe670144c9e
Part 14: Use MOZ_MUST_USE in netwerk/protocol/http r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/1726ee98ad36
Part 15: Use MOZ_MUST_USE in netwerk/protocol/http r=smaug
https://hg.mozilla.org/integration/autoland/rev/06a867ffecdd
Part 16: Use MOZ_MUST_USE in netwerk/protocol/http r=smaug
https://hg.mozilla.org/integration/autoland/rev/828e8f37ba26
Part 17: Use MOZ_MUST_USE in netwerk/protocol/http r=smaug
Depends on: 1383748
Depends on: 1399467
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: