Closed Bug 1367810 Opened 2 years ago Closed 2 years ago

Crash in mozilla::net::nsHttpChannel::CallOnStartRequest

Categories

(Core :: Networking: HTTP, defect, critical)

55 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- disabled
firefox56 --- fixed

People

(Reporter: philipp, Assigned: valentin.gosu)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [necko-active])

Crash Data

Attachments

(1 file)

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.
Blocks: RCWN
Assignee: nobody → michal.novotny
Whiteboard: [necko-active]
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.
Michael, is RCWN nightly only? Or is this riding the trains?
Flags: needinfo?(michal.novotny)
It is disabled. It was enabled on nightly only for few days.
Flags: needinfo?(michal.novotny)
(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?
Flags: needinfo?(valentin.gosu)
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.
Assignee: michal.novotny → valentin.gosu
Flags: needinfo?(valentin.gosu)
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?
(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.
Blocks: 1367951
Attachment #8877983 - Flags: review?(michal.novotny) → review+
Pushed by valentin.gosu@gmail.com:
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
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
https://hg.mozilla.org/mozilla-central/rev/12caabbf2ba8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.