This bug was filed from the Socorro interface and is report bp-fc80605a-8cf6-455d-81ac-057dc2170410. ============================================================= Top #15 of Aurora 20170409004009 on Windows, 11 crashes from 2 installations. The same signature can also be found on Nightly.

This should be a regression of bug 1325915. mReentrantMonitor in nsHttpResponseHead is used after free, which means we are using a bogus pointer of nsHttpResponseHead. Maybe I should use HttpBaseChannel::GetContentLength instead of directly access |mResponseHead| in https://hg.mozilla.org/releases/mozilla-aurora/annotate/20c110248317/netwerk/protocol/http/HttpChannelChild.cpp#l710 and assign -1 to |progressMax| if failed. @mayhemer how do you think?

I think [@ mozilla::net::nsHttpResponseHead::ContentLength ] is the Linux version of this crash.

[@ <name omitted> | mozilla::net::HttpChannelChild::OnTransportAndData ] looks like the OSX signature.

Thanks @mccr8! This crash is definitely affecting Firefox 54. @mayhemer are you able to provide some feedback soon or maybe @dragana can jump in to help the review?

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #1) > This should be a regression of bug 1325915. > > mReentrantMonitor in nsHttpResponseHead is used after free, which means we > are using a bogus pointer of nsHttpResponseHead. > > Maybe I should use HttpBaseChannel::GetContentLength instead of directly > access |mResponseHead| in > https://hg.mozilla.org/releases/mozilla-aurora/annotate/20c110248317/netwerk/ > protocol/http/HttpChannelChild.cpp#l710 and assign -1 to |progressMax| if > failed. > > @mayhemer how do you think? Can you explain what is happening? mResponseHead is set in OnStartRequest. OnStartRequest must have been called before OnTransportAndData. mResponseHead is never nulled, as I can see from a quick look?

(In reply to Dragana Damjanovic [:dragana] from comment #6) > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment > #1) > > This should be a regression of bug 1325915. > > > > mReentrantMonitor in nsHttpResponseHead is used after free, which means we > > are using a bogus pointer of nsHttpResponseHead. > > > > Maybe I should use HttpBaseChannel::GetContentLength instead of directly > > access |mResponseHead| in > > https://hg.mozilla.org/releases/mozilla-aurora/annotate/20c110248317/netwerk/ > > protocol/http/HttpChannelChild.cpp#l710 and assign -1 to |progressMax| if > > failed. > > > > @mayhemer how do you think? > > Can you explain what is happening? > mResponseHead is set in OnStartRequest. OnStartRequest must have been called > before OnTransportAndData. mResponseHead is never nulled, as I can see from > a quick look? mResponseHead is not always set because it depends on other flags. https://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/netwerk/protocol/http/HttpChannelChild.cpp#474 Actually nearly all the places use mResponseHead in have a null check for it. (take HttpBaseChannel::GetContentLength as an example: https://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/netwerk/protocol/http/HttpBaseChannel.cpp#703) In addition HttpBaseChannel::GetContentLength also shows the content length for alt data is not store in mResponseHead, so it would be natural to use that function to align all different cases.

Comment on attachment 8857348 [details] Bug 1355680 - set progressMax to -1 when HTTP response head is not available. https://reviewboard.mozilla.org/r/129332/#review132934 Thanks for the explanation!

[Tracking Requested - why for this release]: crash regression

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #1) > This should be a regression of bug 1325915. > > mReentrantMonitor in nsHttpResponseHead is used after free It more seems to me like a null deref (NOT use after free), which is consistent with the patch and its explanation.

Comment on attachment 8857348 [details] Bug 1355680 - set progressMax to -1 when HTTP response head is not available. https://reviewboard.mozilla.org/r/129332/#review132956 Thanks! ::: netwerk/protocol/http/HttpChannelChild.cpp:759 (Diff revision 1) > > // Hold queue lock throughout all three calls, else we might process a later > // necko msg in between them. > AutoEventEnqueuer ensureSerialDispatch(mEventQ); > > - const int64_t progressMax = mResponseHead->ContentLength(); > + int64_t progressMax = 0; do you really need to init to 0? ::: netwerk/protocol/http/HttpChannelChild.cpp:760 (Diff revision 1) > // Hold queue lock throughout all three calls, else we might process a later > // necko msg in between them. > AutoEventEnqueuer ensureSerialDispatch(mEventQ); > > - const int64_t progressMax = mResponseHead->ContentLength(); > + int64_t progressMax = 0; > + if(NS_FAILED(GetContentLength(&progressMax))) { nit: if (

Tracking 54/55+ for this Necko crash, which is a regression.

(In reply to Honza Bambas (:mayhemer) from comment #10) > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment > #1) > > This should be a regression of bug 1325915. > > > > mReentrantMonitor in nsHttpResponseHead is used after free > > It more seems to me like a null deref (NOT use after free), which is > consistent with the patch and its explanation. Yes should be null deref because the crash dump shows a very small number is used as an address.

Pushed by schien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5908d5fd0aa3 set progressMax to -1 when HTTP response head is not available. r=dragana,mayhemer

Comment on attachment 8857348 [details] Bug 1355680 - set progressMax to -1 when HTTP response head is not available. Approval Request Comment [Feature/Bug causing the regression]: bug 1325915 [User impact if declined]: might crash [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: no [Why is the change risky/not risky?]: add null check [String changes made/needed]: no

Comment on attachment 8857348 [details] Bug 1355680 - set progressMax to -1 when HTTP response head is not available. Fix a potential crash. Aurora54+.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #17) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: yes > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on Shih-Chiang Chien's assessment on manual testing needs and the fact that this fix has automated coverage.