content process crash in mozilla::net::HttpBaseChannel::DoApplyContentConversions

RESOLVED FIXED in Firefox 34

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kairo, Assigned: dragana)

Tracking

({crash})

Trunk
mozilla35
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34+ fixed, firefox35 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
This bug was filed from the Socorro interface and is 
report bp-f3b08aa5-27fe-4e50-beb9-dfe1c2140915.
=============================================================

This started spiking on Nightly 35 in the 20140911064110 build, so the regression is in 9/10 code.

Top frames:
0 	xul.dll 	mozilla::net::HttpBaseChannel::DoApplyContentConversions(nsIStreamListener*, nsIStreamListener**, nsISupports*) 	netwerk/protocol/http/HttpBaseChannel.cpp
1 	xul.dll 	mozilla::net::HttpChannelChild::OnStartRequest(tag_nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&) 	netwerk/protocol/http/HttpChannelChild.cpp
2 	xul.dll 	mozilla::net::HttpChannelChild::RecvOnStartRequest(tag_nsresult const&, mozilla::net::nsHttpResponseHead const&, bool const&, mozilla::net::nsHttpHeaderArray const&, bool const&, bool const&, unsigned int const&, nsCString const&, nsCString const&, mozilla::net::NetAddr const&, mozilla::net::NetAddr const&, short const&) 	netwerk/protocol/http/HttpChannelChild.cpp
3 	xul.dll 	mozilla::net::PHttpChannelChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PHttpChannelChild.cpp
4 	xul.dll 	mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PContentChild.cpp
5 	xul.dll 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
6 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
7 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp

It looks like all the addresses are 0x0, on Windows it's EXCEPTION_ACCESS_VIOLATION_READ, so I guess this might be a null deref.

See stats and more reports at https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Anet%3A%3AHttpBaseChannel%3A%3ADoApplyContentConversions%28nsIStreamListener%2A%2C%20nsIStreamListener%2A%2A%2C%20nsISupports%2A%29
Reporter

Comment 1

5 years ago
This is the first build after bug 1064885 so I guess any regression range I can get will point to this as the cause and this is an e10s regression.
Reporter

Comment 2

5 years ago
Note that all those crashes are in content processes.
Summary: Nightly crash in mozilla::net::HttpBaseChannel::DoApplyContentConversions → content process crash in mozilla::net::HttpBaseChannel::DoApplyContentConversions
In the mDivertingToParent branch (http://hg.mozilla.org/mozilla-central/annotate/d070787de8f7/netwerk/protocol/http/HttpChannelChild.cpp#l329) we clear mListener, then we pass it as the first argument to DoApplyContentConversions. That's not going to work.
Assignee

Updated

5 years ago
Assignee: nobody → dd.mozilla
Assignee

Comment 4

5 years ago
Posted patch bug-1067346.patch (obsolete) — Splinter Review
Attachment #8489457 - Flags: review?(jduell.mcbugs)
Comment on attachment 8489457 [details] [diff] [review]
bug-1067346.patch

>+  if (*aNewNextListener) {
>+    NS_ADDREF(*aNewNextListener);
>+  }

This can just be NS_IF_ADDREF(*aNewNextListener);

Comment 6

5 years ago
Comment on attachment 8489457 [details] [diff] [review]
bug-1067346.patch

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

+1 with jdm's nit.
Attachment #8489457 - Flags: review?(jduell.mcbugs) → review+
Assignee

Comment 7

5 years ago
Posted patch fix v2Splinter Review
Attachment #8489457 - Attachment is obsolete: true
Attachment #8490016 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b1531c87c48
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee

Comment 11

5 years ago
this bug is introduced with patch from Bug 1043256, they should be applied at the same time
Flags: needinfo?(ryanvm)
Is there a question for me here?
Flags: needinfo?(ryanvm)
Assignee

Comment 13

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> Is there a question for me here?

target milestone is different. and bug 1043256 depends on this one.
Target milestone tracks when the patch landed on m-c. If you want this landed on 34, you need to fill out an Aurora approval request like any other patch. I'm taking care of setting the status flags for you. In the future, it'd be helpful if you did so yourself. Also setting the proper dependencies is useful.
Assignee

Comment 15

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Target milestone tracks when the patch landed on m-c. If you want this
> landed on 34, you need to fill out an Aurora approval request like any other
> patch. I'm taking care of setting the status flags for you. In the future,
> it'd be helpful if you did so yourself. Also setting the proper dependencies
> is useful.

thanks a lot

Comment 16

5 years ago
Comment on attachment 8490016 [details] [diff] [review]
fix v2

Approval Request Comment
[Feature/regressing bug #]: This needs to land in the same places as bug 1043256 or that fix may cause crashes in e10s occasionally (clearly not that often, since 1043256 has already landed in b2g 2.1 and the world hasn't ended. But we should fix).

Sadly I'm not clear on which flags are needed to ask for B2G 2.1 approval--do I need something more than approval-aurora+?

[Describe test coverage new/current, TBPL]: Crash-stats is not showing me any crashes for builds after this landed.
[Risks and why]: very low--just change to return early before we might deref a variable that could possibly be null.
[String/UUID change made/needed]: none
Attachment #8490016 - Flags: approval-mozilla-aurora?
Comment on attachment 8490016 [details] [diff] [review]
fix v2

(In reply to Jason Duell (:jduell) from comment #16)
> Approval Request Comment
> [Feature/regressing bug #]: This needs to land in the same places as bug
> 1043256 or that fix may cause crashes in e10s occasionally (clearly not that
> often, since 1043256 has already landed in b2g 2.1 and the world hasn't
> ended. But we should fix).

Given that e10s is disabled by default in 34, I think this means that Firefox 34 is unaffected. Can you please confirm?

> Sadly I'm not clear on which flags are needed to ask for B2G 2.1
> approval--do I need something more than approval-aurora+?

You've done the right thing. B2G 2.1 currently requires aurora approval.

Aurora+
Attachment #8490016 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(jduell.mcbugs)

Comment 19

5 years ago
> Given that e10s is disabled by default in 34, I think this means that Firefox 34
> is unaffected. Can you please confirm?

AFAIK that's true, but dragana should know for sure.
Flags: needinfo?(jduell.mcbugs) → needinfo?(dd.mozilla)
Assignee

Comment 20

5 years ago
(In reply to Jason Duell (:jduell) from comment #19)
> > Given that e10s is disabled by default in 34, I think this means that Firefox 34
> > is unaffected. Can you please confirm?
> 
> AFAIK that's true, but dragana should know for sure.

it is true, Firefox 34 with e10s disable is unaffected
Assignee

Updated

5 years ago
Flags: needinfo?(dd.mozilla)
You need to log in before you can comment on or make changes to this bug.