1329506 - Crash in mozilla::net::DoUpdateExpirationTime

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-dc000373-3213-4b5f-b3ee-a5c562170108.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::net::DoUpdateExpirationTime(mozilla::net::nsHttpChannel*, nsICacheEntry*, mozilla::net::nsHttpResponseHead*, unsigned int&) 	netwerk/protocol/http/nsHttpChannel.cpp:4241
1 	xul.dll 	mozilla::net::nsHttpChannel::UpdateExpirationTime() 	netwerk/protocol/http/nsHttpChannel.cpp:4260
2 	xul.dll 	mozilla::net::nsHttpChannel::InitCacheEntry() 	netwerk/protocol/http/nsHttpChannel.cpp:4666
3 	xul.dll 	mozilla::net::nsHttpChannel::ContinueProcessNormal(nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp:2165
4 	xul.dll 	mozilla::net::nsHttpChannel::ProcessNormal() 	netwerk/protocol/http/nsHttpChannel.cpp:2132
5 	xul.dll 	mozilla::net::nsHttpChannel::ContinueProcessResponse2(nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp:2105
6 	xul.dll 	mozilla::net::nsHttpChannel::ContinueProcessResponse2(nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp:2109
7 	xul.dll 	mozilla::net::nsHttpChannel::OnRedirectVerifyCallback(nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp:7320
8 	xul.dll 	mozilla::net::nsAsyncVerifyRedirectCallbackEvent::Run() 	netwerk/base/nsAsyncRedirectVerifyHelper.cpp:40
9 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1076
10 	xul.dll 	NS_InvokeByIndex 	xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54
11 	xul.dll 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	js/xpconnect/src/XPCWrappedNative.cpp:1361
12 	xul.dll 	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1128
13 		@0x2e7e6917 	
14 		@0xf2839f7 	
15 		@0x2fd80942 	
16 	xul.dll 	EnterBaseline 	js/src/jit/BaselineJIT.cpp:155
17 	xul.dll 	js::jit::EnterBaselineMethod(JSContext*, js::RunState&) 	js/src/jit/BaselineJIT.cpp:194
18 	xul.dll 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp:389
19 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:471
20 	xul.dll 	InternalCall 	js/src/vm/Interpreter.cpp:498
21 	xul.dll 	nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) 	js/xpconnect/src/XPCWrappedJSClass.cpp:1135
22 	xul.dll 	mozilla::dom::Promise::PerformMicroTaskCheckpoint() 	dom/promise/Promise.cpp:1045
23 	mozglue.dll 	je_free 	memory/mozjemalloc/jemalloc.c:6479
24 	xul.dll 	nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::ShiftData<nsTArrayInfallibleAllocator>(unsigned int, unsigned int, unsigned int, unsigned int, unsigned int) 	obj-firefox/dist/include/nsTArray-inl.h:261
25 	xul.dll 	nsTArray_Impl<ObserverRef, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) 	obj-firefox/dist/include/nsTArray.h:1906
26 	xul.dll 	nsObserverList::FillObserverArray(nsCOMArray<nsIObserver>&) 	xpcom/ds/nsObserverList.cpp:89
27 	xul.dll 	nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) 	xpcom/ds/nsObserverService.cpp:305

this crash signature started rising across all release channels starting with 2016-11-26, so probably some sort of external influence contributed to that. 
it's occurring on shutdown of the process though and in rather low volume over all.

Comment 1

[Honza Bambas (:mayhemer)]

Apparently missing nullcheck for aCacheEntry in DoUpdateExpirationTime.

Comment 2

[Honza Bambas (:mayhemer)]

We are apparently after shutdown.  I makes CacheStorageService::AddStorageEntry fail at [1] which makes CacheEntry::Recreate return null and NS_OK.  Should throw, tho.

[1] https://dxr.mozilla.org/mozilla-central/rev/88030580b14bb253a55bc174c987a9fa43c3fb55/netwerk/cache2/CacheStorageService.cpp#1498

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1 (let CacheEntry::Recreate throw when it fails)

after shutdown, creation of cache entries is prohibited.  but CacheEntry::Recreate  returns null and OK.  this leads callers to an expectation that there is an entry returned (there are failure check paths).  but it is not created and code execution continues w/o further non-null checks.

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e168644a85
Add missing non-null check in mozilla::net::DoUpdateExpirationTime. r=michal

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8e168644a85

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8827475 [details] [diff] [review]
v1 (let CacheEntry::Recreate throw when it fails)

Approval Request Comment
[Feature/Bug causing the regression]: cache2, I think (so, very long ago)
[User impact if declined]: shutdown crash
[Is this code covered by automated tests?]: apparently the crash is not, but otherwise this code is heavily executed
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: very simple change correcting error handing paths (checked that all consumers handle errors returned by this API)
[String changes made/needed]: none

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 8827475 [details] [diff] [review]
v1 (let CacheEntry::Recreate throw when it fails)

crash fix for aurora53 and beta52

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/56b35490bf16