Closed Bug 1422545 Opened 6 years ago Closed 6 years ago

Visiting a certain URL makes Firefox crash

Categories

(Core :: Networking: HTTP, defect, P2)

58 Branch
x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- verified
firefox59 --- verified

People

(Reporter: giul.mus, Assigned: dragana)

Details

(Keywords: crash, Whiteboard: [necko-triaged])

Crash Data

Attachments

(3 files, 2 obsolete files)

Visiting http://pbook.s3-eu-west-1.amazonaws.com/global.zip makes my Firefox crash. Out of three times I visited the page (by entering the URL in the address bar and pressing enter), every time it crashed with a signature of "mozilla::net::HttpBaseChannel::DoApplyContentConversions". Here is the latest report: https://crash-stats.mozilla.com/report/index/e8505ca5-f649-4be5-b207-1ef1e0171202

I tried to reproduce it by downloading the file and serving it with a local server (python3 -m http.server), but in that case, it doesn't crash.

I attached the file for debug purposes, as well as the output of "curl -v <URL>".
Attached file Output of "curl -v"
Severity: normal → critical
Crash Signature: [@ mozilla::net::HttpBaseChannel::DoApplyContentConversions ]
Component: Untriaged → Networking: HTTP
Keywords: crash
Product: Firefox → Core
I will take a look.
Assignee: nobody → dd.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Whiteboard: [necko-triaged]
Attached patch bug_1422545_v1.patch (obsolete) — Splinter Review
Attachment #8934601 - Flags: review?(honzab.moz)
Comment on attachment 8934601 [details] [diff] [review]
bug_1422545_v1.patch

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

description please
Attachment #8934601 - Flags: review?(honzab.moz) → review-
actually I forgot to change the commit message, sorry...
Whether we need to divert to parent or not is decided in OnStartReques call, OnStartReques call of urlLoader.
If unknownDecoder is involved, unknowDecoder will delay calling OnStartRequest until it gets some data, exactly 512bytes. If a content is small OnStartRequest will be triggered by OnStopRequest call to unknownDecoder and only after that we know whether we need to divert to parent or not. 

HttpChannelChild::OnStopRequest function does a cleanup at the end (if diversion has not been started before OnStopRequest is called; In this case we know whether we need to divert to parent only almost at the end of the HttpChannelChild::OnStopRequest function). Therefore when diversion happens the parent channel is already half cleaned up, e.g. mChannel in HttpChannelParent is nullptr, that is why this crash happens.

So if we need to divert to parent HttpChannelChild::OnStopRequest should skip cleanup.
Attached patch bug_1422545_v1.patch (obsolete) — Splinter Review
Attachment #8934601 - Attachment is obsolete: true
Attachment #8935019 - Flags: review?(honzab.moz)
Comment on attachment 8935019 [details] [diff] [review]
bug_1422545_v1.patch

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

thanks for the detailed description!

please mention in commit message that this deals with http channels only (current is too general. IMO).

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1105,5 @@
> +  // listeners chain is called in DoOnStopRequest. At that moment
> +  // unknownDecoder will call OnStartRequest of the real listeners of the
> +  // channel including the OnStopRequest of UrlLoader which decides whether we
> +  // need to divert to parent.
> +  // If we are diverting to parent we should not do a cleanup.

would be good to note who does the cleanup eventually.  I presume it's the divert logic when it's done on the child?

@@ +1106,5 @@
> +  // unknownDecoder will call OnStartRequest of the real listeners of the
> +  // channel including the OnStopRequest of UrlLoader which decides whether we
> +  // need to divert to parent.
> +  // If we are diverting to parent we should not do a cleanup.
> +  if (!mDivertingToParent) {

cosmetic: would if (mDivertingToParent) { LOG(("..diverting..."; return; } do better here?  is diversion logged well enough so that we know why cleanup has not been done at this point?
Attachment #8935019 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #8)
> Comment on attachment 8935019 [details] [diff] [review]
> bug_1422545_v1.patch
> 
> +  if (!mDivertingToParent) {
> 
> cosmetic: would if (mDivertingToParent) { LOG(("..diverting..."; return; }
> do better here?  is diversion logged well enough so that we know why cleanup
> has not been done at this point?


ti is log well, but one more line does not hurt.
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e87b07c9db7c
Do not close connection between a httpChannelChild and its httpChannelParent if we need to divert to parent.r=mayhemer
Attachment #8935019 - Attachment is obsolete: true
Attachment #8937991 - Flags: review+
Comment on attachment 8937991 [details] [diff] [review]
bug_1422545_v1.patch

Approval Request Comment
[Feature/Bug causing the regression]: Long existing bug.
[User impact if declined]: Can cause a crash.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]: There are steps to reproduce in the description of the bug.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It is a very short patch and all code paths are already used.
[String changes made/needed]: none
Attachment #8937991 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/e87b07c9db7c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8937991 [details] [diff] [review]
bug_1422545_v1.patch

very low volume crash, not a new regression, OTOH it seems this is reproducible and well enough understood, so let's get the fix in beta.
Attachment #8937991 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I managed to reproduce the bug on Windows 10 x64 and  Ubuntu 16.04 x64 using an older version of Nightly (2017-12-04). As soon as I entered the url the browser crashed.
I retested everything using the latest Nightly 59 and beta 58.0b13 on Windows 10 x64, Mac OS 10.11 and Ubuntu 16.04 x64 and the bug is not reproducing.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: