1342425 - Crash in mozilla::net::AppendRequestsToArray

Status: ASSIGNED

(Core :: Networking, defect, P3)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-318fe3b4-1e51-4e1b-81b3-697f22170224.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::net::AppendRequestsToArray 	netwerk/base/nsLoadGroup.cpp:207
1 	xul.dll 	mozilla::net::nsLoadGroup::Cancel(nsresult) 	netwerk/base/nsLoadGroup.cpp:230
2 	xul.dll 	nsDocLoader::Stop() 	uriloader/base/nsDocLoader.cpp:243
3 	xul.dll 	nsDocShell::Stop(unsigned int) 	docshell/base/nsDocShell.cpp:5512
4 	xul.dll 	nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) 	docshell/base/nsDocShell.cpp:10517
5 	xul.dll 	nsDocShell::LoadHistoryEntry(nsISHEntry*, unsigned int) 	docshell/base/nsDocShell.cpp:12457
6 	xul.dll 	nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) 	docshell/base/nsDocShell.cpp:1437
7 		@0x46c463 	
8 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:458
9 	xul.dll 	js::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) 	js/src/vm/TypeInference.cpp:3275
10 	xul.dll 	js::NativeObject::getChildProperty(js::ExclusiveContext*, JS::Handle<js::NativeObject*>, JS::Handle<js::Shape*>, JS::MutableHandle<js::StackShape>) 	js/src/vm/Shape.cpp:446
11 	xul.dll 	XPCWrappedNative::GetNewOrUsed(xpcObjectHelper&, XPCWrappedNativeScope*, XPCNativeInterface*, XPCWrappedNative**) 	js/xpconnect/src/XPCWrappedNative.cpp:460
12 	xul.dll 	XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, bool, nsresult*) 	js/xpconnect/src/XPCConvert.cpp:874

this crash signature was first appearing in firefox 49 - it's mostly affecting 32bit versions of firefox on windows. the share of crash reports from ru & zh-cn also looks higher than usual...

Comment 1

[Michal Novotny [:michal]]

I'm not sure how this can happen. Requests are added to mRequests only in nsLoadGroup::AddRequest(), if the request were null there it would crash on null pointer in MergeDefaultLoadFlags() or MergeLoadFlags().

Comment 2

[Julien Cristau [:jcristau] (back April 22)]

Too late for firefox 52, mass-wontfix.

Comment 3

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 5

[Hani Yacoub, Desktop QA]

Closing this as resolved:worksforme since there were no crashes in the last 6 months for this signature.

Comment 6

[Hani Yacoub, Desktop QA]

I'll Reopen this bug again. There have been crashes with this signature.

Comment 7

[Wayne Mery (:wsmwk)]

Only two version 90 crashes in the past month. For example bp-6bed2226-137d-405d-8444-6e5020210806

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Since the crash volume is low (less than 5 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

No direct indication this was ever a regression per se; and certainly isn't now.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1342425: make assertion for null request a DIAGNOSTIC_ASSERT r=#necko-reviewers

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bca019ee7e0c
make assertion for null request a DIAGNOSTIC_ASSERT r=necko-reviewers,valentin

Comment 12

[Sandor Molnar[:smolnar]]

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

Comment 13

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

This crash report has request == NULL
https://crash-stats.mozilla.org/report/index/f5511770-df62-4efc-aecc-4f57d0230127#tab-details

	Name	Value	Type
▶	aArray	0x000000000088ca80 size = ???, capacity = ???	nsTArray<nsIRequest *> *
▶	aTable	0x0000000000d6fec0 {mOps=??? mEntryStore={mEntryStore=??? } mGeneration=??? ...}	PLDHashTable *
	e	Variable is optimized away and not available.
◢	iter	{mTable=0x0000000000d6fec0 {mOps=??? mEntryStore={mEntryStore=??? } mGeneration=??? ...} mCurrent={mEntry=...} ...}	PLDHashTable::Iterator
	▶ mTable	0x0000000000d6fec0 {mOps=??? mEntryStore={mEntryStore=??? } mGeneration=??? ...}	PLDHashTable *
	▶ mCurrent	{mEntry=0x000000001451ad00 {...} mKeyHash=0x000000001451ab80 {???} }	PLDHashTable::Slot
	mNexts	0x0000000a	unsigned int
	mNextsLimit	0x00000014	unsigned int
	mHaveRemoved	false	bool
	mEntrySize	0x08 '\b'	unsigned char
◢	request	0x0000000000000000 <NULL>	nsIRequest *
	◢ nsISupports	<struct at NULL>	nsISupports
	__vfptr	<Unable to read memory>	void * *
	▶ mCanceledReason	???	nsTString<char>

I'm not sure how that can happen. The place where we add to the hashtable is preceded by a deref of the request in MergeLoadFlags or MergeDefaultLoadFlags (unless that gets optimized away).

I wonder if we should instead rewrite mRequests as a nsTHashMap?
This kind of weird bugs have been fixed by such refactorings before. Unless we can find the actual source of the bug of course :)

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jesup, maybe it's time to close this bug?
For more information, please visit BugBot documentation.

Comment 15

[Sunil Mayya]

We still observe crashes here.
Keeping in monitor. We could take further actions if the number of recurrence increases.