433525 - crash [@ nsNavHistoryQueryResultNode::IsContainersQuery()]

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Samuel Sidler (old account; do not CC)]

Crash stats shows a new topcrash that started in the 2008050600 builds. This is topcrash #8 in the last week.

Sample stack from bp-f90a22cc-20c7-11dd-83ba-0013211cbf8a:

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsNavHistoryQueryResultNode::IsContainersQuery 	mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:2098
1 	xul.dll 	nsNavHistoryQueryResultNode::CanExpand 	mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:2071
2 	xul.dll 	nsNavHistoryQueryResultNode::GetHasChildren 	mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp:2164
3 	xul.dll 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
4 	xul.dll 	XPCWrappedNative::CallMethod 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2369

There aren't many comments yet, but uptime seems to imply that this is happening at startup for many people or within the first few minutes of usage.

Comment 1

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

Guys, any ideas what can call this? I tend to suspect the Most Visited smartfolder.

Comment 2

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

FYI, this has dropped off to topcrash 27, with the following frequency over the past two weeks: 

All     Win     OSX    Linux    Sol
48  	 42 	 6 	 0 	 0 

and seems to be dropping off in terms of frequency with later builds.

Comment 3

[Samuel Sidler (old account; do not CC)]

Err... beltzner, this is #8 in the past week with that frequency. It's lower in the last two weeks (#27) because it didn't occur until the 2008050600 builds.

It does seem like there's a drop off, but I think that's just nightly users switching to RC1 (like, QA). Looking at the Table in the URL above, you'll see the drop off. But I think we should wait until RC1 crash feedback before decidedly blocking on this.

Comment 4

[Samuel Sidler (old account; do not CC)]

Oh, and URL for top crashes in the past week:

http://crash-stats.mozilla.com/?do_query=1&product=Firefox&version=Firefox%3A3.0pre&query_search=signature&query_type=contains&query=&date=&range_value=1&range_unit=weeks

Comment 5

[Dietrich Ayala (:dietrich)]

If anyone can reproduce this, it'd be very helpful to get the places.sqlite for that profile.

Comment 6

[Dietrich Ayala (:dietrich)]

Looks like it's a back-end bug exposed by the fix for bug 431817. Fix coming up.

Comment 7

[Carsten Book [:Tomcat]]

just to confirm this crash, i was running during my RC1 FFT run now 2 times into this crash on XP SP3. 

The Crash happened out of the blue on different scenarios and also when i just surfing. 

http://crash-stats.mozilla.com/report/index/588d5b51-2147-11dd-bfbf-0013211cbf8a?p=1

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: potential fix

looks like mOptions can be accessed before it's initialized, via GetHasChildren().

builds here:

https://build.mozilla.org/tryserver-builds/2008-05-13_17:[e-mail]-bug433525-isContainersQueryCrash/

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 320842 [details] [diff] [review]
potential fix

Index: toolkit/components/places/src/nsNavHistoryResult.cpp
===================================================================
+//  Safe options getter, ensures queries are parsed first.
+nsNavHistoryQueryOptions*
+nsNavHistoryQueryResultNode::Options()
+{

nit: this file comment style has an empty line between comment and function def

+  nsresult rv = VerifyQueriesParsed();
+  if (NS_FAILED(rv))
+    return nsnull;
+  NS_ASSERTION(mOptions, "Options invalid, cannot generate fro mURI");

nit: from URI

+
+function run_test() {
+  var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsINavHistoryService);
+  var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].getService(Ci.nsINavBookmarksService);

nit: cut at 80 and try catch

+  var options = histsvc.getNewQueryOptions();
+  options.maxResults = 1;
+  options.resultType = options.RESULTS_AS_URI
+  var query = histsvc.getNewQuery();

missing ; after RESULTS_AS_URI

+
+  // now check via the saved search path
+  var queryURI = histsvc.queriesToQueryString([query], 1, options);
+  bmsvc.insertBookmark(bmsvc.toolbarFolder, uri(queryURI), 0 /* first item */, "test query");

nit: 80 chars


the code is ok but the crash test does not crash for me without the patch, isn't it supposed to verify the crash?

Comment 10

[Dietrich Ayala (:dietrich)]

> 
> the code is ok but the crash test does not crash for me without the patch,
> isn't it supposed to verify the crash?
> 

doesn't crash for me either. tomcat is going to try the build, since he can reproduce the crash (i tried on windows, couldn't reproduce).

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: comments fixed (checked in)

Comment 12

[Mike Connor [:mconnor]]

Would be good to get this fixed ASAP, not a showstopper on its own.

Comment 13

[Jesse Ruderman]

This is now topcrash #1 for Firefox 3 RC1.

http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0

Comment 14

[Dietrich Ayala (:dietrich)]

I was able to reproduce this crash with the steps in bug 434590, and able to confirm that this patch fixes it.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 320917 [details] [diff] [review]
comments fixed (checked in)

r=mano

Comment 17

[Bill Gianopoulos [:WG9s]]

(In reply to comment #12)
> Would be good to get this fixed ASAP, not a showstopper on its own.
> 

Well, under windows, I am unable to get the RC1 build to run more than an hour or two without hitting this crash.

Comment 18

[Marcia Knous [:marcia]]

During testing of  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0 I got in this state and was consistently crashing until I started a new session and then the crash went away. I was not doing anything in Places when I crashed. The profile I am running with contains data from Firefox 2.

Comment 19

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

Marcia, do you still have the affected sessionstore.js? Can you share it?

Comment 20

[Bill Gianopoulos [:WG9s]]

(In reply to comment #18)
> During testing of  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
> rv:1.9) Gecko/2008051202 Firefox/3.0 I got in this state and was consistently
> crashing until I started a new session and then the crash went away. I was not
> doing anything in Places when I crashed. The profile I am running with contains
> data from Firefox 2.
> 

What do you mean by you were not doing anything in Places when you crashed?  Almost everything you do in the browser involves a history update which uses Places.

Comment 21

[Marcia Knous [:marcia]]

I meant I was not doing anything like the STR in Bug 434590. First crash was clicking on the Dock icon.

Comment 22

[Mike Schroepfer]

Comment on attachment 320917 [details] [diff] [review]
comments fixed (checked in)

a+ schrep for 3.0.1 or RC2 please land on cvs trunk.

Comment 23

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.147; previous revision: 1.146
done
Checking in toolkit/components/places/src/nsNavHistoryResult.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.h,v  <--  nsNavHistoryResult.h
new revision: 1.57; previous revision: 1.56
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_433525_hasChildren_crash.js,v
done
Checking in toolkit/components/places/tests/unit/test_433525_hasChildren_crash.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_433525_hasChildren_crash.js,v  <--  test_433525_hasChildren_crash.js
initial revision: 1.1
done

Comment 24

[Tomas]

Fixed?

Comment 25

[Ed Lee :Mardak]

http://hg.mozilla.org/cvs-trunk-mirror/index.cgi/rev/6b5944d1c06d

Comment 27

[Al Billings [:abillings - ex-MoCo]]

There is no real way to verify this, is there, other than looking at the crash data?

Comment 28

[Samuel Sidler (old account; do not CC)]

Should be able to verify with the steps in bug 434590 (see comment 14). Possibly in bug 435059 too.

But yes, the full verification will be that it disappears from the topcrash report for RC2.

Comment 29

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

There are still crashes reported by users who are still running Firefox 3.0. These are 66 crashes within the last month. For later builds this stack isn't listed anymore. Now more than 4 month later I believe we can mark this bug as verified. At least I cannot reproduce it with the latest builds.

Comment 30

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv