Closed
Bug 1422545
Opened 6 years ago
Closed 6 years ago
Visiting a certain URL makes Firefox crash
Categories
(Core :: Networking: HTTP, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla59
People
(Reporter: giul.mus, Assigned: dragana)
Details
(Keywords: crash, Whiteboard: [necko-triaged])
Crash Data
Attachments
(3 files, 2 obsolete files)
453 bytes,
application/zip
|
Details | |
995 bytes,
text/plain
|
Details | |
2.11 KB,
patch
|
dragana
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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>".
Severity: normal → critical
Crash Signature: [@ mozilla::net::HttpBaseChannel::DoApplyContentConversions ]
Component: Untriaged → Networking: HTTP
Keywords: crash
Product: Firefox → Core
Assignee | ||
Comment 2•6 years ago
|
||
I will take a look.
Assignee: nobody → dd.mozilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Assignee | ||
Updated•6 years ago
|
Whiteboard: [necko-triaged]
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8934601 -
Flags: review?(honzab.moz)
Comment 4•6 years ago
|
||
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-
Assignee | ||
Comment 5•6 years ago
|
||
actually I forgot to change the commit message, sorry...
Assignee | ||
Comment 6•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #8934601 -
Attachment is obsolete: true
Attachment #8935019 -
Flags: review?(honzab.moz)
Comment 8•6 years ago
|
||
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+
Assignee | ||
Comment 9•6 years ago
|
||
(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.
Comment 10•6 years ago
|
||
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
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #8935019 -
Attachment is obsolete: true
Attachment #8937991 -
Flags: review+
Assignee | ||
Comment 12•6 years ago
|
||
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?
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e87b07c9db7c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•6 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
status-firefox-esr52:
--- → wontfix
Comment 14•6 years ago
|
||
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+
Comment 15•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e7e0bfcbc040
Updated•6 years ago
|
Flags: qe-verify+
Comment 16•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•