startup crash in nsPermissionManager::InitDB, spiking in 41 beta

RESOLVED FIXED in Firefox 41

Status

()

Core
Permission Manager
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Robert Kaiser, Assigned: Ehsan)

Tracking

({crash})

unspecified
mozilla43
x86
Windows NT
crash
Points:
---

Firefox Tracking Flags

(firefox41+ fixed, firefox42 fixed, firefox43 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-f3c7c6bd-1519-42e8-b1bf-3526e2150813.
=============================================================

Top Stack Frames:
0 	xul.dll 	nsPermissionManager::InitDB(bool) 	extensions/cookie/nsPermissionManager.cpp
1 	xul.dll 	nsPermissionManager::Init() 	extensions/cookie/nsPermissionManager.cpp
2 	xul.dll 	nsPermissionManager::GetXPCOMSingleton() 	extensions/cookie/nsPermissionManager.cpp
3 	xul.dll 	nsIPermissionManagerConstructor 	extensions/cookie/nsCookieModule.cpp
4 	xul.dll 	mozilla::GenericFactory::CreateInstance(nsISupports*, nsID const&, void**) 	xpcom/glue/GenericFactory.cpp
5 	xul.dll 	nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) 	xpcom/components/nsComponentManager.cpp
6 	xul.dll 	nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) 	xpcom/components/nsComponentManager.cpp
7 	xul.dll 	nsTHashtable<nsBaseHashtableET<nsCStringHashKey, nsFactoryEntry*> >::s_HashKey(PLDHashTable*, void const*) 	xpcom/glue/nsTHashtable.h
8 	xul.dll 	nsContentBlocker::Init() 	extensions/permissions/nsContentBlocker.cpp
9 	xul.dll 	mozilla::GenericFactory::CreateInstance(nsISupports*, nsID const&, void**) 	xpcom/glue/GenericFactory.cpp
10 	xul.dll 	nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) 	xpcom/components/nsComponentManager.cpp
[...]

This signature seems to have been around in low volume on other versions, but seems to spike in 41 beta 1. It's on startup, which makes it rather nasty, and consistently seems to have an address of 0x0, which smells like a null deref.
(Reporter)

Comment 1

3 years ago
[Tracking Requested - why for this release]:
High-ranking startup crash in 41.0b1
tracking-firefox41: --- → ?

Comment 2

3 years ago
Tracked as it is a top crash.
status-firefox41: --- → affected
tracking-firefox41: ? → +
mDBConn is null.

From blame on nearby lines, one possibility I see is:
Bug 1124126 - Retry database connection for the case of corrupted permissions.sqlite. r=bsmedberg 

    1.59    rv = storage->OpenDatabase(permissionsFile, getter_AddRefs(mDBConn));
    1.60 -  NS_ENSURE_SUCCESS(rv, rv);
    1.61 +  if (rv == NS_ERROR_FILE_CORRUPTED) {

What if rv was some other error code?

Another possibility may be:
Bug 967812 - Pref to make Permissions Manager memory-only. r=ehsan
Flags: needinfo?(benjamin)
Needinfo sitting for several days (I tried pinging on IRC). This is a startup crash so we need to act quickly. Ehsan, maybe you can look at this sooner? I don't expect that this is related to the change you reviewed, but perhaps you could point me to someone who knows this code.
Flags: needinfo?(ehsan)
(Assignee)

Comment 5

3 years ago
The most likely theory that I can come up with is the DB initialization failing with something other than NS_ERROR_UNEXPECTED, which is entirely possible.
Assignee: nobody → ehsan
Blocks: 967812
Flags: needinfo?(ehsan)
(Assignee)

Comment 6

3 years ago
Created attachment 8649468 [details] [diff] [review]
Correctly deal with all possible ways that initializing the permission manager DB connection can fail
Attachment #8649468 - Flags: review?(michael)

Updated

3 years ago
Attachment #8649468 - Flags: review?(michael) → review+
(Assignee)

Comment 7

3 years ago
Comment on attachment 8649468 [details] [diff] [review]
Correctly deal with all possible ways that initializing the permission manager DB connection can fail

Approval Request Comment
[Feature/regressing bug #]: Probably bug 967812
[User impact if declined]: I'd like to see if this patch will fix the startup crash, so I'd like to take this into beta ASAP.
[Describe test coverage new/current, TreeHerder]: This component has some tests, but we don't know how to reproduce the crash, so I'm just guessing for now.
[Risks and why]: Very low risk.
[String/UUID change made/needed]: None.
Attachment #8649468 - Flags: approval-mozilla-beta?
Attachment #8649468 - Flags: approval-mozilla-aurora?

Comment 9

3 years ago
Comment on attachment 8649468 [details] [diff] [review]
Correctly deal with all possible ways that initializing the permission manager DB connection can fail

Approved for uplift to Beta and Aurora as it's a top ranking crash.
Attachment #8649468 - Flags: approval-mozilla-beta?
Attachment #8649468 - Flags: approval-mozilla-beta+
Attachment #8649468 - Flags: approval-mozilla-aurora?
Attachment #8649468 - Flags: approval-mozilla-aurora+

Updated

3 years ago
status-firefox42: --- → affected
https://hg.mozilla.org/mozilla-central/rev/9cf478be459a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

3 years ago
Flags: needinfo?(benjamin)
(Assignee)

Comment 13

3 years ago
Kairo, what's the next step here?  I want to make sure that this doesn't fall through the cracks, and someone would end up verifying my guess.  :-)
Flags: needinfo?(kairo)
There isn't enough volume to verify this on nightly or aurora, so we'll need to wait for 41b3 data to come in. Don't worry, this is definitely on my radar and I bet KaiRo's too :-)
(Reporter)

Comment 15

3 years ago
(In reply to Ehsan Akhgari (don't ask for review please) from comment #13)
> Kairo, what's the next step here?  I want to make sure that this doesn't
> fall through the cracks, and someone would end up verifying my guess.  :-)

We are closely watching beta crash stats all the time. If it continues to appear there, you can be sure we come back and nag here. If it doesn't, it may just disappear off our radar or we may remember to report back that it's successfully gone. That's at least the usual thing that happens with beta crash fixes.
Flags: needinfo?(kairo)
(Assignee)

Comment 16

3 years ago
Great, so I'll wait to see if this keeps showing up on beta!  Fingers crossed.  :-)
(Reporter)

Comment 17

3 years ago
And FWIW, both the signature summary behind the link in the Crash Signature field of this bug, and my searches for crashes on 41.0b3 confirm that this signature is not seen any more in beta 3, so this fix seems to have worked. :)
(Assignee)

Comment 18

3 years ago
\m/
You need to log in before you can comment on or make changes to this bug.