1233962 - crash in mozilla::net::InterceptedChannelBase::DoNotifyController

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[[:philipp]]

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-653d439a-2022-4fcf-a46c-f238b2151219.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::net::InterceptedChannelBase::DoNotifyController() 	netwerk/protocol/http/InterceptedChannel.cpp
1 	xul.dll 	mozilla::net::InterceptedChannelChrome::NotifyController() 	netwerk/protocol/http/InterceptedChannel.cpp
2 	xul.dll 	mozilla::net::nsHttpChannel::OpenCacheEntry(bool) 	netwerk/protocol/http/nsHttpChannel.cpp
3 	xul.dll 	mozilla::net::nsHttpChannel::Connect() 	netwerk/protocol/http/nsHttpChannel.cpp
4 	xul.dll 	mozilla::net::nsHttpChannel::ContinueBeginConnectWithResult() 	netwerk/protocol/http/nsHttpChannel.cpp
5 	nss3.dll 	PR_Assert 	nsprpub/pr/src/io/prlog.c
6 	xul.dll 	mozilla::net::nsHttpChannel::BeginConnect() 	netwerk/protocol/http/nsHttpChannel.cpp
7 	xul.dll 	mozilla::net::nsHttpChannel::OnProxyAvailable(nsICancelable*, nsIChannel*, nsIProxyInfo*, nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp
8 	xul.dll 	nsAsyncResolveRequest::DoCallback() 	netwerk/base/nsProtocolProxyService.cpp
9 	xul.dll 	nsAsyncResolveRequest::OnQueryComplete(nsresult, nsCString const&, nsCString const&) 	netwerk/base/nsProtocolProxyService.cpp
10 	xul.dll 	ExecuteCallback::Run() 	netwerk/base/nsPACMan.cpp
11 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
12 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
13 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
14 	xul.dll 	nsThreadManager::UnregisterCurrentThread(nsThread*) 	xpcom/threads/nsThreadManager.cpp
15 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
16 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp
17 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp
18 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
19 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp

this signature is spiking in firefox 44 devedition (#4 with 2.88% of crashes) and beta. 
maybe it's related to the changes in bug 1206894, so i'm also cc'ing ehsan here on this bug.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

We should probably gracefully fail interception if the mController provided to the intercepted channel is nullptr.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I have an idea for a patch.  NI to remind myself to look at this tomorrow.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Note, all crash stacks go through OnProxyAvailable in nsProtocolProxyService.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't create an InterceptedChannel if there is no controller. r=jdm

Still working on getting a local socks proxy running, but I'm fairly confident this is crashing due to a nullptr mController.  Its really the only possible nullptr in DoNotifyController().

Another possibility would be if DoNotifyController() was called twice for the same channel, but I don't think thats possibly.  Its only called immediately after construction of the InterceptedChannel.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=645bfe98267e

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Unfortunately we have no way to write a mochitest or web-platform-test to use a SOCKS proxy AFAICT.  We have xpcshell tests that implement a SOCKS proxy, but I'm not sure I can get service workers to run in xpcshell tests.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as it's a crash.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I've tried testing locally with manual proxy settings, a served proxy.pac, etc, but so far no luck reproducing.

I'm going to test various proxy addons as well.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Looking at the crash reports, there are no common extensions in use.

I'm not sure I will be able to reproduce this.  I think we should proceed with the patch as our best path forward.

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 8700641 [details] [diff] [review]
Don't create an InterceptedChannel if there is no controller. r=jdm

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3027,3 @@
>      // If this channel should be intercepted, we do not open a cache entry for this channel
>      // until the interception process is complete and the consumer decides what to do with it.
> +    if (mInterceptCache == MAYBE_INTERCEPT && controller) {

I am concerned that the state here is not actually well understood - in particular, the code in nsHttpChannel never expects to make a network request while mInterceptCache == MAYBE_INTERCEPT. I think we should store the controller in AsyncOpen and just use that, which will allow us to continue executing the regular code paths we already use.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Save nsINetworkInterceptController for later use in http channels. r=jdm

I'm not sure if I should release the mController at some point or if its safe to hold until the channel is destroyed.  Lets see what try says.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f3f570131b

Comment 11

[Josh Matthews [:jdm]]

Mmm, I forgot about the nulling out part. I predict leaks will come of this; we should probably null it out in OnStartRequest.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Yea, try confirms this leaks the world:

  https://treeherder.mozilla.org/logviewer.html#?job_id=14794971&repo=try

I'll try nulling in OnStartRequest later tonight.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Save nsINetworkInterceptController for later use in http channels. r=jdm

Updated to clear mController in HttpBaseChannel::ReleaseListeners().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=029926a3f3c1

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Call ResetInterception() if the controller is nullptr. r=jdm

Unfortunately the last patch does not work in e10s mode.  There it seems the code actually depends on the controller changing from one HttpChannelParent to another HttpChannelParent during redirection.  By saving the first controller we ended up perma-failing some tests because we called back on the wrong actor.

I think it might just be safer for now to call ResetInterception() for a nullptr controller.  In theory this path should be sound since we already call that if the controller returns an error.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9280c79986d8

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 8700900 [details] [diff] [review]
Call ResetInterception() if the controller is nullptr. r=jdm

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

Makes sense.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Call ResetInterception() if the controller is nullptr. r=jdm

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Fix service worker xpcshell test to return a dispatcher from ChannelIntercepted(). r=jdm

The old code did not ResetInterception() if the dispatcher was nullptr.  I'm pretty sure that was a bug, but the necko xpcshell tests depended on it.

This patch makes the xpcshell tests return a dummy dispatcher from ChannelIntercepted().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a193354c753d

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76d24989a1b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/722472112f94

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8701048 [details] [diff] [review]
P1 Call ResetInterception() if the controller is nullptr. r=jdm

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: Crash under certain automatic proxy configurations when a site is using service workers.
[Describe test coverage new/current, TreeHerder]: Unfortunately we have not been able to produce the triggering configuration locally or in a test.  This fix is based on code inspection of where we can end up with a nullptr.  All existing service worker tests continue to pass with these changes, however.
[Risks and why]: This change only affects service workers.
[String/UUID change made/needed]: None.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8701048 [details] [diff] [review]
P1 Call ResetInterception() if the controller is nullptr. r=jdm

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: Crash under certain automatic proxy configurations when a site is using service workers.
[Describe test coverage new/current, TreeHerder]: Unfortunately we have not been able to produce the triggering configuration locally or in a test.  This fix is based on code inspection of where we can end up with a nullptr.  All existing service worker tests continue to pass with these changes, however.
[Risks and why]: This change only affects service workers.
[String/UUID change made/needed]: None.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76d24989a1b0
https://hg.mozilla.org/mozilla-central/rev/722472112f94

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Ritu, can we get this uplifted to beta immediately?  Unfortunately we have no instances of the crash on aurora or nightly due to user configuration, so bake time does not really help us.  I'd like to get this in for next Monday's beta build, but I will be on PTO starting tomorrow.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8701048 [details] [diff] [review]
P1 Call ResetInterception() if the controller is nullptr. r=jdm

Crash fix, taking it. Aurora45+, Beta44+

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/48c66e74eda5
https://hg.mozilla.org/releases/mozilla-aurora/rev/f75514287a07

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8887f01eeba
https://hg.mozilla.org/releases/mozilla-beta/rev/6c6b6dc18c76

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e8887f01eeba
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6c6b6dc18c76

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

After a week there are no reports of this crash in 44.0b4, so the fix does appear to solve the problem.