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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Robert Kaiser]

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

Comment 1

[Robert Kaiser]

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.

Comment 2

[Robert Kaiser]

Note that all those crashes are in content processes.

Comment 3

[Josh Matthews [:jdm]]

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.

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug-1067346.patch

Comment 5

[Josh Matthews [:jdm]]

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

[Jason Duell]

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

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

+1 with jdm's nit.

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: fix v2

Comment 8

[Dragana Damjanovic [:dragana]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1531c87c48

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b1531c87c48

Comment 11

[Dragana Damjanovic [:dragana]]

this bug is introduced with patch from Bug 1043256, they should be applied at the same time

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is there a question for me here?

Comment 13

[Dragana Damjanovic [:dragana]]

(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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Dragana Damjanovic [:dragana]]

(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

[Jason Duell]

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

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa6dc8cffc42

Comment 19

[Jason Duell]

> 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.

Comment 20

[Dragana Damjanovic [:dragana]]

(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