1120395 - crash in nsUrlClassifierStreamUpdater::OnStartRequest(nsIRequest*, nsISupports*)

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

This bug was filed from the Socorro interface and is 
report bp-c456674d-4e54-46c6-8cfa-a07642150112.
=============================================================

We got this while running one of our functional mozmill tests.
Report: http://mozmill-daily.blargon7.com/#/functional/report/5bb4668cfa6af53a0ba505835d6355fa

Comment 1

[Patrick McManus [:mcmanus]]

monica - related to your recent changes maybe?

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

It may be -- also see https://bugzilla.mozilla.org/show_bug.cgi?id=1120145

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

After further investigation, this is not related to https://bugzilla.mozilla.org/show_bug.cgi?id=1100024.

These crashes date from at least 12/26, before https://bugzilla.mozilla.org/show_bug.cgi?id=1100024 landed (the first time). Most likely they are due to the timer in nsUrlClassifierStreamUpdater not setting the callback when it sets its own timer in https://bugzilla.mozilla.org/show_bug.cgi?id=1110891.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Some crash details:

Crash Reason 	SIGSEGV
Crash Address 	0x0

0 	libxul.so 	nsUrlClassifierStreamUpdater::OnStartRequest(nsIRequest*, nsISupports*) 	toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
1 	libxul.so 	mozilla::net::HttpBaseChannel::DoNotifyListener() 	netwerk/protocol/http/HttpBaseChannel.cpp
2 	libxul.so 	nsRunnableMethodImpl<void (mozilla::net::HttpBaseChannel::*)(), void, true>::Run() 	xpcom/glue/nsThreadUtils.h
3 	libxul.so 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
4 	libxul.so 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
5 	libxul.so 	mozilla::dom::indexedDB::::QuotaClient::ShutdownTransactionService 	dom/indexedDB/ActorsParent.cpp
6 	libxul.so 	mozilla::dom::quota::QuotaManager::Observe(nsISupports*, char const*, char16_t const*) 	dom/quota/QuotaManager.cpp
7 	libxul.so 	nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) 	xpcom/ds/nsObserverList.cpp
8 	libxul.so 	nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) 	xpcom/ds/nsObserverService.cpp
9 	libxul.so 	nsXREDirProvider::DoShutdown() 	toolkit/xre/nsXREDirProvider.cpp
10 	libxul.so 	ScopedXPCOMStartup::~ScopedXPCOMStartup() 	toolkit/xre/nsAppRunner.cpp

Given by Soccorro it's a cross-platform crash.

Comment 5

[brandonjakes]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

[Tracking Requested - why for this release]:

[Blocking Requested - why for this release]:

Comment 6

[brandonjakes]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

[Tracking Requested - why for this release]:

[Blocking Requested - why for this release]:

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please do not play around with bug flags. Thanks.

Comment 8

[Mats Palmgren (inactive)]

[Tracking Requested - why for this release]:

The signature has spiked in v37:
Product	        Version	Percentage	Number Of Crashes
Firefox 	37.0a1 	91.67 %	1057
FennecAndroid 	37.0a1 	4.08 %	47
Firefox 	34.0.5 	2.08 %	24
Firefox 	35.0b99 0.61 %	7
FennecAndroid 	34.0.1 	0.61 %	7
Firefox 	34.0 	0.52 %	6
Firefox 	35.0 	0.17 %	2
FennecAndroid 	35.0b8 	0.09 %	1
FennecAndroid 	35.0b11 0.09 %	1
Firefox 	35.0b3 	0.09 %	1

Comment 9

[Mats Palmgren (inactive)]

My mistake, the above table is for "nsUrlClassifierPrefixSet::Contains(unsigned int, bool*)"
but I suspect it might be the same underlying problem.  Here's the table for
"nsUrlClassifierStreamUpdater::OnStartRequest(nsIRequest*, nsISupports*)":
Product	Version	Percentage	Number Of Crashes
Firefox 	37.0a1 	76.92 %	10
Firefox 	37.0a2 	15.38 %	2
Firefox 	18.0b5 	7.69 %	1

Comment 10

[Andreea Matei [:AndreeaMatei]]

Submitted another crash on OSX 10.7 and also I have this one, is this related too? On the same machine:
https://crash-stats.mozilla.com/report/index/1195a38a-b6e5-417a-bdad-a777e2150120

Doesn't have this bug at relevant bugs, I'll file a different one if it's unrelated, I see it's also about UrlClassifier.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

This signature is showing up as the #2 topcrasher for e10s-enabled Firefox 37 with 500 crashes in the last 7 days. It isn't on the list of topcrashers for 37 as a whole, though.  So it may be e10s related. 

https://crash-stats.mozilla.com/search/?product=Firefox&dom_ipc_enabled=!__null__&version=37.0a1&process_type=browser&date=%3E%3D2015-01-12&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking 37 until we know that this doesn't impact when e10s is disabled.

Comment 13

[Mats Palmgren (inactive)]

My crash in nsUrlClassifierPrefixSet::Contains was with e10s disabled.

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Mats Palmgren (:mats) from comment #13)
> My crash in nsUrlClassifierPrefixSet::Contains was with e10s disabled.

The crash in nsUrlClassifierPrefixSet::Contains was fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe8067063a0 (https://bugzilla.mozilla.org/show_bug.cgi?id=1120145#c5) probably in 1/13's build. Are you still seeing this signature?

Comment 15

[Mats Palmgren (inactive)]

No, I haven't seen it in newer than 1/13 builds, so it's likely my crashes were that other bug.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible patch

Looking through the stacks, it seems like this is strictly a shutdown crash. It's definitely happening because we're clearing mDownloadErrorCallback before receiving OnStartRequest, which can happen when we're shutting down (xpcom-shutdown-threads causes nsUrlClassifierDBService to cancel any pending updates, which clears mDownloadErrorCallback, as well as most of the rest of our state). This patch tries to detect this state and bail out without doing further stuff.

Monica, does this seem reasonable to you?

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

This is probably not related to e10s, based on the stacks and is a shutdown crash.

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8554932 [details] [diff] [review]
Possible patch

Review of attachment 8554932 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. gcp may want to take a look.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8554932 [details] [diff] [review]
Possible patch

Review of attachment 8554932 [details] [diff] [review]:
-----------------------------------------------------------------

From reading the code a bit, the shutdown ends up forcing an UpdateError in
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#605
which calls into
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#65 which suggests that mIsUpdating or the callback != nullptr is what you are supposed to check. At least it's not obvious to me mChannel being null there is guaranteed to be directly related.

I think mChannel == nullptr is coming from StreamUpdater noticing the shutdown event too:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#589

The only other place that clears it is https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#565 but that's one that you patched, so this doesn't look right.

I'm going to change my review based on this, I think you need to check mIsUpdating because currently you're dependent on the observer noticing the shutdown event, but I can be convinced otherwise if I missed anything.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible patch v2

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fb0e24dba89

Comment 22

[[:mmc] Monica Chew (no longer reading bugmail)]

Looks like you are hitting this assert a bunch of times:

https://hg.mozilla.org/try/file/8ea8d7e2fe98/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#l465

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible patch v3

I have no idea how that assertion can hit. I have stared at this code
for way too long now and am giving up. This patch is safer and is green
on try <https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3dace903c55>.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3fa9d6b1310

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Daniela Domnici]

Crashed again over the weekend(2/7/2015	4:36 AM), with aurora 37.0a2 zh-CN on windows 8-32-2.

https://crash-stats.mozilla.com/report/index/ed8ad7d3-d62a-48f5-b224-065e12150209.

Comment 27

[Gian-Carlo Pascutto [:gcp]]

...which is why it says fixed in Firefox 38 not in Firefox 37...

Comment 28

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8558684 [details] [diff] [review]
Possible patch v3

Approval Request Comment
[Feature/regressing bug #]: Long standing bug exposed by e10s.
[User impact if declined]: Occasional crashes on shutdown.
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Low, added check to prevent the crash.
[String/UUID change made/needed]: N/A

Comment 29

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8558684 [details] [diff] [review]
Possible patch v3

Trivial fix. Aurora+

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84030546d496

Comment 31

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

This seems to have gone away with no more crashes reported by Socorro [1] over the past 4 weeks in builds after the fix landed.

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=nsUrlClassifierStreamUpdater%3A%3AOnStartRequest%28nsIRequest%2A%2C+nsISupports%2A%29#tab-reports