Closed Bug 1329506 Opened 3 years ago Closed 3 years ago

Crash in mozilla::net::DoUpdateExpirationTime

Categories

(Core :: Networking, defect, critical)

50 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: philipp, Assigned: mayhemer)

References

Details

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

Crash Data

Attachments

(1 file)

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.
Apparently missing nullcheck for aCacheEntry in DoUpdateExpirationTime.
Assignee: nobody → honzab.moz
Whiteboard: [necko-active]
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
Status: NEW → ASSIGNED
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
Attachment #8827475 - Flags: review?(michal.novotny)
Attachment #8827475 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e168644a85
Add missing non-null check in mozilla::net::DoUpdateExpirationTime. r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8e168644a85
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(honzab.moz)
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
Flags: needinfo?(honzab.moz)
Attachment #8827475 - Flags: approval-mozilla-beta?
Attachment #8827475 - Flags: approval-mozilla-aurora?
Comment on attachment 8827475 [details] [diff] [review]
v1 (let CacheEntry::Recreate throw when it fails)

crash fix for aurora53 and beta52
Attachment #8827475 - Flags: approval-mozilla-beta?
Attachment #8827475 - Flags: approval-mozilla-beta+
Attachment #8827475 - Flags: approval-mozilla-aurora?
Attachment #8827475 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.