Closed Bug 1067346 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Networking: HTTP, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + fixed
firefox35 --- fixed

People

(Reporter: kairo, Assigned: dragana)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
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.
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: nobody → dd.mozilla
Attached 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 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+
Attached patch fix v2Splinter Review
Attachment #8489457 - Attachment is obsolete: true
Attachment #8490016 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b1531c87c48
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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)
(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.
(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 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)
> 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)
(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
Flags: needinfo?(dd.mozilla)
You need to log in before you can comment on or make changes to this bug.