977736 - DoAuthRetry consistently gives ASSERTION: Appended stream not at beginning

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Kent James (:rkent)]

Bug 799234 added an assertion in nsMultiplexInputStream when the stream is not at the beginning. However, in nsHttpChannel::DoAuthRetry, the stream is not rewound until after the MultiplexInputStream is setup. This causes grief in my unit tests.

Here is a stack trace of the call to the assertion I am seeing:

 	xul.dll!NS_DebugBreak(unsigned int aSeverity, const char * aStr, const char * aExpr, const char * aFile, int aLine)  Line 414 + 0xc bytes	C++
 	xul.dll!nsMultiplexInputStream::AppendStream(nsIInputStream * aStream)  Line 111 + 0x2b bytes	C++
 	xul.dll!mozilla::net::nsHttpTransaction::Init(unsigned int caps, mozilla::net::nsHttpConnectionInfo * cinfo, mozilla::net::nsHttpRequestHead * requestHead, nsIInputStream * requestBody, bool requestBodyHasHeaders, nsIEventTarget * target, nsIInterfaceRequestor * callbacks, nsITransportEventSink * eventsink, nsIAsyncInputStream * * responseBody)  Line 349 + 0x26 bytes	C++
 	xul.dll!mozilla::net::nsHttpChannel::SetupTransaction()  Line 775 + 0xc1 bytes	C++
>	xul.dll!mozilla::net::nsHttpChannel::DoAuthRetry(mozilla::net::nsAHttpConnection * conn)  Line 5721 + 0x8 bytes	C++
 	xul.dll!mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest * request, nsISupports * ctxt, tag_nsresult status)  Line 5100 + 0x17 bytes	C++
 	xul.dll!nsInputStreamPump::OnStateStop()  Line 704	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream)  Line 438 + 0xb bytes	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 86	C++

I can fix the problem by moving the stream rewind slightly earlier, patch to follow.

Comment 1

[Kent James (:rkent)]

Attachment: Move stream reset before transaction init

I don't know enough about all of the intricate possibilities to really judge this approach, but it makes sense to me (and stops the assertion).

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8383207 [details] [diff] [review]
Move stream reset before transaction init

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

I think you are actually fixing a bug :)  This looks OK to me.  Please push to try with also ASAN linux build, but I think this really OK to go.

Comment 3

[Kent James (:rkent)]

Honza,

I'm a mailnews developer, so I'm not really up on mozilla-central try server setup and issues. Could you do the push for me?

Comment 4

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=e954d3b4e947

Comment 5

[Kent James (:rkent)]

Thanks, Patrick. Looks clean enough to me, so I'll flag this for landing.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/645fe7e9cc70

Thanks for the patch, Kent! One request, please make sure that future patches include your name and email address in the commit information when requesting checkin. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/645fe7e9cc70