531236 - Crash [@ nsNavBookmarks::GetBookmarksHash]

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[timeless]

Signature	nsNavBookmarks::GetBookmarksHash()
UUID	112cba2f-a439-4e77-b63f-b7eba2091125
Time 	2009-11-25 02:24:37.352899
Uptime	16
Last Crash	18 seconds before submission
Product	Firefox
Version	3.6b3
Build ID	20091115182845
Branch	1.9.2
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
CPU	x86
CPU Info	GenuineIntel family 15 model 2 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x64
User Comments	

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsNavBookmarks::GetBookmarksHash 	toolkit/components/places/src/nsNavBookmarks.cpp:679
1 	xul.dll 	nsNavBookmarks::IsRealBookmark 	toolkit/components/places/src/nsNavBookmarks.cpp:917
2 	xul.dll 	nsNavHistory::AddVisit 	toolkit/components/places/src/nsNavHistory.cpp:2792
3 	xul.dll 	nsNavHistory::AddVisitChain 	toolkit/components/places/src/nsNavHistory.cpp:5231
4 	xul.dll 	nsNavHistory::AddURIInternal 	toolkit/components/places/src/nsNavHistory.cpp:5073
5 	xul.dll 	nsNavHistory::CommitLazyMessages 	toolkit/components/places/src/nsNavHistory.cpp:5950
6 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:427
7 	nspr4.dll 	_PR_MD_UNLOCK 	nsprpub/pr/src/md/windows/w95cv.c:344
8 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:519
9 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:527
10 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
11 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:182
12 	nspr4.dll 	PR_GetEnv 	
13 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:120
14 	firefox.exe 	__tmainCRTStartup 	obj-firefox/memory/jemalloc/crtsrc/crtexe.c:591
15 	kernel32.dll 	BaseProcessStart 	

I'm guessing this relates to bug 496613, but it's just a guess.

Comment 1

[Marco Bonardo [:mak]]

4 crashes reported on Win on 3.6b1
http://is.gd/53XPa

getBookmarksHash is pretty simple

686 nsDataHashtable<nsTrimInt64HashKey, PRInt64>*
687 nsNavBookmarks::GetBookmarksHash()
688 {
689   if (!mBookmarksHash.IsInitialized()) {
690     nsresult rv = FillBookmarksHash();
691     NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "FillBookmarksHash() failed!");
692   }
693 
694   return &mBookmarksHash;

looks like we crash on mBookmarksHash.IsInitialized()

Comment 2

[Marco Bonardo [:mak]]

betterlink  http://is.gd/53ZdB about 30 crash reports on 3.6

previous code was from m-c, code is the same on 1.9.2 just different offset

   679  if (!mBookmarksHash.IsInitialized()) {

requesting blocking to make drivers aware this exists.

Comment 3

[Marco Bonardo [:mak]]

why is crash-stats search so bogus: http://is.gd/53Zjq 69 crashes total so far on 3.6b1-3.6b3 on Windows and Linux.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Marco, is timeless' assertion that this is a new crash regression correct, or are you saying that this is the same as an existing crash, it's stack has just moved with the new code?

Comment 5

[Marco Bonardo [:mak]]

it's a new crash, introduced with bug 496613, even if actually i'm unsure how that can happen.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, can you take the bug?

Comment 7

[Marco Bonardo [:mak]]

taking to try driving this out.

Comment 8

[Marco Bonardo [:mak]]

This happens on startup, when creating a new profile, since moving from 3.6b1 to 3.6b4 clearly does not upgrade schema.

Also uptime confirms this, and varying uptime seem to confirm this happens after migration in some case.

history is initialized most likely by migration, to create places.sqlite, at this point we enqueue an event to the main thread. iirc at this stage there is a partial restart of the application, the stack here tells us that nsAppRun runs the events loop, finds our enqueued event, and it notified "places-init-complete" notification. History service handles the notification, gets bookmarks service and here we crash trying to access to a private member of nsNavBookmarks object. Sounds like pointer to nsNavBookmarks object is pointing to a bad object.

Comment 9

[Marco Bonardo [:mak]]

most likely same as bug 327331, we should bring that fix to other Places services

Comment 10

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This is most likely due to bad bookmarks service initialization in the special situation we have on startup, we fixed some possible race condition in history service init, and we must do the same here.
At the same time there is some code path missing robust error checking, and i'm adding some.

I'll get a green on tryserver before actually considering this done (compilers are picky sometimes), but should be fine for a review.

unfortunatly this crash is untestable, i'd not know how to reach such a situation by will.

Comment 11

[Marco Bonardo [:mak]]

fwiw, not ready for prime-time, i'm crashing in some xpcshell test, looking into it.

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1.1

checkpoint, added some more error checks and found why i was crashing (a wrong QI)

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 415274 [details] [diff] [review]
patch v1.1

compiled and passed tests on Win and Mac.

Comment 14

[Marco Bonardo [:mak]]

this is building on tryserver now
as a side note, in the pushed version i removed a semicolon from +PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsAnnotationService, gAnnotationService);
since it's useless and Gcc on linux sometime is picky about semicolons.

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 415274 [details] [diff] [review]
patch v1.1

looks good, r=me

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Seems to have failed tryserver tests?

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1259645277.1259648111.11119.gz

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Seems to have failed tryserver tests?
Not related to this patch:
ar: libgklayout.a: No space left on device

Comment 18

[Marco Bonardo [:mak]]

tryservers are fine afaict, there are a couple linux errors due to the error reported in comment 17 and a windows random orange in a layout crashtest, that looks really unrelated. Then the classic pthread.h error on winMo (known orange), Talos crash on Linux (known orange) and usual not working talos os X boxes.

I'll push shortly to m-c

Comment 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/422dc39dad74

Comment 20

[Marco Bonardo [:mak]]

Attachment: 1.9.2 patch

m-c patch unbitrotted for 1.9.2

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a1bd08749064

Comment 22

[Marco Bonardo [:mak]]

i marked 1.9.1 as unaffected because we don't see this crash there, even if the changes we have here would be valid for 1.9.0 and 1.9.1, both could indeed reach a bad initialization, but looks like we don't "ping" the crash there.