1367810 - Crash in mozilla::net::nsHttpChannel::CallOnStartRequest

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-79c6346c-96f6-49bb-9387-b14910170525.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::net::nsHttpChannel::CallOnStartRequest() 	netwerk/protocol/http/nsHttpChannel.cpp:1389
1 	xul.dll 	mozilla::net::nsHttpChannel::ContinueOnStartRequest2(nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp:7054
2 	xul.dll 	mozilla::net::nsHttpChannel::ContinueOnStartRequest1(nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp:7031
3 	xul.dll 	mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) 	netwerk/protocol/http/nsHttpChannel.cpp:7003
4 	xul.dll 	nsInputStreamPump::OnStateStart() 	netwerk/base/nsInputStreamPump.cpp:539
5 	xul.dll 	nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) 	netwerk/base/nsInputStreamPump.cpp:441
6 	xul.dll 	nsInputStreamReadyEvent::Run() 	xpcom/io/nsStreamUtils.cpp:96
7 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1309
8 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:96
9 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
10 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
11 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
12 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:271
13 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:283
14 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4568
15 	xul.dll 	XREMain::XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4748
16 	xul.dll 	XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4843
17 	firefox.exe 	NS_internal_main(int, char**, char**) 	browser/app/nsBrowserApp.cpp:309
18 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
19 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
20 	kernel32.dll 	BaseThreadInitThunk 	
21 	ntdll.dll 	RtlUserThreadStart

so far 2 of these crashes are showing up in today's 20170525030225 nightly build - both with MOZ_RELEASE_ASSERT(!(mRequireCORSPreflight && mInterceptCache != INTERCEPTED) || mIsCorsPreflightDone) (CORS preflight must have been finished by the time we call OnStartRequest) which was added bug 1199049 originally.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

I haven't managed to reproduce this - I need to find a small CORS test case - but I think the problem is that the OnStart is returned from the cache, while the network request is the one to set mRequireCORSPreflight = true - so we hit the assertion.

Comment 2

[Mike Taylor [:miketaylr]]

Michael, is RCWN nightly only? Or is this riding the trains?

Comment 3

[Michal Novotny [:michal]]

It is disabled. It was enabled on nightly only for few days.

Comment 4

[Mike Taylor [:miketaylr]]

Thanks.

Comment 5

[Michal Novotny [:michal]]

(In reply to Valentin Gosu [:valentin] from comment #1)
> I haven't managed to reproduce this - I need to find a small CORS test case
> - but I think the problem is that the OnStart is returned from the cache,
> while the network request is the one to set mRequireCORSPreflight = true -
> so we hit the assertion.

Looks like something that will be fixed by patch in bug 1367742, right?

Comment 6

[Valentin Gosu [:valentin] (he/him)]

I don't think it will.
I think this is actually a correctness problem - if mRequireCORSPreflight, then the channel should actually wait for the preflight to be completed, and not attempt to return the response from the cache.
I have a patch ready to disable racing for channels that require preflight.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1367810 - Don't race cache with network when CORS preflight is required for a channel

Review commit: https://reviewboard.mozilla.org/r/149394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149394/

Comment 8

[Michal Novotny [:michal]]

Comment on attachment 8877983 [details]
Bug 1367810 - Don't race cache with network when CORS preflight is required for a channel

https://reviewboard.mozilla.org/r/149394/#review153930

::: netwerk/protocol/http/nsHttpChannel.cpp:9116
(Diff revision 1)
>          return NS_OK;
>      }
>  
> +    // If a CORS Preflight is required, and it hasn't completed,
> +    // or it has completed but failed, we must not race.
> +    if (mRequireCORSPreflight && (!mIsCorsPreflightDone || NS_FAILED(mStatus))) {

Shouldn't we return early always (i.e. not only when CORS preflight is required) when mStatus is a failure?

Comment 9

[Valentin Gosu [:valentin] (he/him)]

(In reply to Michal Novotny (:michal) from comment #8)
> Shouldn't we return early always (i.e. not only when CORS preflight is
> required) when mStatus is a failure?

We probably should, but I hadn't checked all the code paths, we would get to have a failed mStatus.
I'll update the patch to do so.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8877983 [details]
Bug 1367810 - Don't race cache with network when CORS preflight is required for a channel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149394/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12caabbf2ba8
Don't race cache with network when CORS preflight is required for a channel r=michal

Comment 12

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12caabbf2ba860021b1879c5cc9e7ce521aa373e
Bug 1367810 - Don't race cache with network when CORS preflight is required for a channel r=michal

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/12caabbf2ba8