493538 - Crash in [@ nsNavHistory::RecalculateFrecenciesInternal(mozIStorageStatement*, int)]

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P1)

Description

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

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b5pre) Gecko/20090517 Shiretoko/3.5b5pre ID:20090517031113

Today I crashed in the below stack while having my Linux VM running in the background:

Crash report: bp-1738cc7d-b386-452b-9085-55f192090518

0  	libxul.so  	nsNavHistory::RecalculateFrecenciesInternal  	toolkit/components/places/src/nsNavHistory.cpp:7358
1 	libxul.so 	nsNavHistory::RecalculateFrecencies 	toolkit/components/places/src/nsNavHistory.cpp:7343
2 	libxul.so 	nsNavHistory::Observe 	toolkit/components/places/src/nsNavHistory.cpp:5480
3 	libxul.so 	nsObserverList::NotifyObservers 	xpcom/ds/nsObserverList.cpp:128
4 	libxul.so 	nsObserverService::NotifyObservers 	xpcom/ds/nsObserverService.cpp:181
5 	libxul.so 	nsIdleService::CheckAwayState 	widget/src/xpwidgets/nsIdleService.cpp:214
6 	libxul.so 	nsIdleService::IdleTimerCallback 	widget/src/xpwidgets/nsIdleService.cpp:132
7 	libxul.so 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:420
8 	libxul.so 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:512
9 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
10 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:227

Comment 1

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

Looks like a background task we start in specific time intervals to update the frequence, right? So would there be a way to trigger such a run manually?

Asking for blocking1.9.1.

Comment 2

[Marco Bonardo [:mak]]

we should stop observing idle daily BEFORE our statements are finalized.

Comment 3

[Marco Bonardo [:mak]]

7358   nsresult rv = aStatement->BindInt32Parameter(0, aCount);

i think the problem is that we stop observing idle-daily at xpcom-shutdown, but we finalize statements at quit-application-granted.

Comment 4

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

Happend again, exactly one hour after the first crash.

Comment 5

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

Happens on all platforms:
http://crash-stats.mozilla.com/report/list?product=Firefox&branch=1.9.1&version=Firefox%3A3.5b4&version=Firefox%3A3.5b4pre&version=Firefox%3A3.5b5pre&version=Firefox%3A3.6a1pre&query_search=signature&query_type=startswith&query=nsNavHistory%3A%3ARecalculateFrecenciesInternal&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsNavHistory%3A%3ARecalculateFrecenciesInternal%28mozIStorageStatement*%2C%20int%29

Comment 6

[Marco Bonardo [:mak]]

Attachment: m-c patch v1.0

Comment 7

[Marco Bonardo [:mak]]

Attachment: 1.9.1 patch v1.0

unluckily i'm unable to reproduce the crash in xpcshell.

Comment 8

[Ed Lee :Mardak]

Comment on attachment 378056 [details] [diff] [review]
m-c patch v1.0

>+    // Stop observing idle-daily.  We will finalize our statements during this
>+    // notification, so we can't use them later.
>+    nsCOMPtr<nsIObserverService> observerService =
>+      do_GetService("@mozilla.org/observer-service;1", &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    observerService->RemoveObserver(this, gIdleDaily);
Are we crashing because we're getting the idle notification when shutting down? It seems like it's happening fairly frequently -- are we in "shut down process" for a long time that the user goes idle?

We're crashing because GetDBInvalidFrecencies ends up returning null on 1.9.1, and that would only happen on app quit?

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #8)
> (From update of attachment 378056 [details] [diff] [review])
> >+    // Stop observing idle-daily.  We will finalize our statements during this
> >+    // notification, so we can't use them later.
> >+    nsCOMPtr<nsIObserverService> observerService =
> >+      do_GetService("@mozilla.org/observer-service;1", &rv);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> >+    observerService->RemoveObserver(this, gIdleDaily);
> Are we crashing because we're getting the idle notification when shutting down?
> It seems like it's happening fairly frequently -- are we in "shut down process"
> for a long time that the user goes idle?
> 
> We're crashing because GetDBInvalidFrecencies ends up returning null on 1.9.1,
> and that would only happen on app quit?

I've not been able to reproduce the crash locally, so i can only guess.
In some case shutdown times could be long, not sure if that's long enough to make us consider it an idle valid for idle-daily.

About the fact GetDBInvalidFrecencies returns null, i don't know how that is possible since if it's null it will recreate the statement... unless is null mDBConn.

Comment 10

[Marco Bonardo [:mak]]

oh sure, if we have closed mDBConn, then GetDBInvalidFrecencies will return nsnull trying to create a statement on a closed connection

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 378057 [details] [diff] [review]
1.9.1 patch v1.0

I got it, if you use a trunk profile on branch, due to the new column the GetDBInvalidFrecencies getter will always return nsnull because the query is wrong.
At that point we will try to bind to null, causing the crash.

plus the above added protection.

this MUST block, really.

Comment 12

[Shawn Wilsher :sdwilsh]

Yeah - this should block the RC.

Comment 13

[Marco Bonardo [:mak]]

crash started with bug 488966, was unfortunately not fixed with bug 491954 due to the fact this query is 1.9.1 only, and finished being undetected.

STR:
1. use a minefield profile with current branch
2. let the browser go idle.

Comment 14

[Marco Bonardo [:mak]]

Attachment: m-c patch v1.1

Comment 15

[Ed Lee :Mardak]

Comment on attachment 378067 [details] [diff] [review]
m-c patch v1.1

r=Mardak

Thanks for unshifting and early-returning :)

Comment 16

[Marco Bonardo [:mak]]

Attachment: 1.9.1 patch v1.1

basically the same, plus fix for the broken query

Comment 17

[Ed Lee :Mardak]

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

r=Mardak

Test shouldn't be too difficult to make? Grab a trunk places.sqlite and notify with idle-daily to trigger RecalculateFrecencies.

Comment 18

[Marco Bonardo [:mak]]

yes the test would not be hard but, i guess if it's worth doing it because would test a very specific implementation flaw, in a very specific case... and we don't even have that code anymore on trunk.

Comment 19

[Shawn Wilsher :sdwilsh]

Yeah, I'm not convinced it's worth writing a test in this case.

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b0b77510a0ff

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0b3ba1d05309

Comment 23

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

No crashes are listed anymore since 0519. So this fix made it. Also my builds are running fine in background without a crash.

Marking verified fixed on trunk and 1.9.1 branch:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090522 Minefield/3.6a1pre ID:20090522032716

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre ID:20090521135222