791562 - crash in PlacesFolderConversion::AppendFolder (Protect bookmarks roots from being deleted through the API)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Scoobidiver (away)]

It's #54 top browser crasher in 15.0.1, #65 in 16.0b2, #146 in 17.0a2, and #273 in 18.0a1.

There are no crashes after 18.0a1/20120907. The working range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=36427d4b2cf6&tochange=1d4fc0c60063

Signature 	PlacesFolderConversion::AppendFolder(nsCString&, __int64) More Reports Search
UUID	39e67ea3-eb81-44bf-ae13-720132120915
Date Processed	2012-09-15 01:05:40
Uptime	86033
Last Crash	23.9 hours before submission
Install Age	4.6 days since version was first installed.
Install Time	2012-09-10 08:23:08
Product	Firefox
Version	18.0a1
Build ID	20120907030554
Release Channel	nightly
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 15 stepping 6
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x27a2, AdapterSubsysID: 00000000, AdapterDriverVersion: 6.14.10.4543
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x27a2
Total Virtual Memory	2147352576
Available Virtual Memory	1557901312
System Memory Use Percentage	45
Available Page File	2530414592
Available Physical Memory	1174216704

Bugzilla - Report this bug in Firefox, Core, Plug-Ins, or Toolkit
Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	PlacesFolderConversion::AppendFolder 	toolkit/components/places/nsNavHistoryQuery.cpp:220
1 	xul.dll 	nsNavHistory::QueriesToQueryString 	toolkit/components/places/nsNavHistoryQuery.cpp:475
2 	xul.dll 	nsNavHistoryQueryResultNode::VerifyQueriesSerialized 	toolkit/components/places/nsNavHistoryResult.cpp:2259
3 	xul.dll 	nsNavHistoryQueryResultNode::GetUri 	toolkit/components/places/nsNavHistoryResult.cpp:2159
4 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
5 	xul.dll 	XPCWrappedNative::GetAttribute 	js/xpconnect/src/xpcprivate.h:2817
6 	xul.dll 	XPC_WN_GetterSetter 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1518
7 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:344
8 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:388
9 	mozjs.dll 	js::GetPropertyOperation 	js/src/jsinterpinlines.h:270
10 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2284
11 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:355
12 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:388
13 	mozjs.dll 	js::CrossCompartmentWrapper::call 	js/src/jswrapper.cpp:648
14 	mozjs.dll 	proxy_Call 	js/src/jsproxy.cpp:2990
15 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:337
16 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2405
17 	xul.dll 	XPCConvert::NativeData2JS 	js/xpconnect/src/XPCConvert.cpp:230
18 	xul.dll 	XPCConvert::NativeData2JS 	js/xpconnect/src/xpcprivate.h:3307

More reports at:
https://crash-stats.mozilla.com/report/list?signature=PlacesFolderConversion%3A%3AAppendFolder%28nsCString%26%2C+__int64%29

Comment 1

[Marco Bonardo [:mak]]

(In reply to Scoobidiver from comment #0)
> There are no crashes after 18.0a1/20120907.

I can find crashes after that... Am I reading it wrongly?

Comment 2

[Scoobidiver (away)]

(In reply to Marco Bonardo [:mak] from comment #1)
> (In reply to Scoobidiver from comment #0)
> > There are no crashes after 18.0a1/20120907.
> I can find crashes after that... Am I reading it wrongly?
There weren't when I wrote this comment but there are now in 18.0a2, 19.0a1 and 20.0a1.

Comment 3

[Marco Bonardo [:mak]]

The only strange things I see in that code:

1. we don't check OOM from GetFolders, though the method doesn't touch inputs if that happens, so this is not an issue.

2. we don't null check nsNavBookmarks *bs = nsNavBookmarks::GetBookmarksService(); in AppendFolder()... Though if this happens means bookmarks don't work at all, so I'm not sure how we arrive here... Maybe something is trying to access bookmarks but the profile is heavily corrupt. I heard about an increase in bookmarks loss due to add-ons and synchronization, but most of these crashes don't have add-ons. I'd also expect a short uptime, or comments saying "crashes everytime I open that menu", while here I see random crashes. The kind of corruption should be something wrongly moving or deleting one of the 4 root folders (removeItem, moveItem, removeFolderChildren), synchronization software may cause this (and would be extremely sad).  We can add protection against this.

3. I double checked array management but looks good, we don't seem to access anything out of boundaries

This I think I'll take this bug and add protections for case 2, since I can't find any other plausible cause of crashes.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 5

[Marco Bonardo [:mak]]

And I'd like to backport this at least to Aurora, due to the increasing number of support tickets about bookmarks lost.

Comment 6

[Marco Bonardo [:mak]]

Or if possible even beta, the number of reported bookmarks dataloss is increasing scarily.

Comment 7

[Asaf Romano (gone)]

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

r=mano

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Marco Bonardo [:mak]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no regression
User impact if declined: We are seeing an increase in the number of bookmarks corruptions (they completely stop working) due to lost root folders. This crash is just one of the consequences. The possible cause of corruption has been probably blocked in Sync, but we'd prefer to also add this layer of protection in Places.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): It's merely added arguments and null checks. risk is minimal.
String or UUID changes made by this patch: none

Comment 11

[bhavana bajaj [:bajaj]]

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

Approving on aurora only.
Not approving for beta at this time considering the risk given how close we are to FF18's release vs the reward.

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb81bd72390d

Comment 13

[Scoobidiver (away)]

It's #7 top browser crasher in 17.0.1esr.

Comment 15

[Richard Newman [:rnewman]]

(In reply to Scoobidiver from comment #13)
> It's #7 top browser crasher in 17.0.1esr.

Well, that's interesting. Might indicate that Sync is not the culprit — even though Sync is available on ESR, it's hard to imagine that it's heavily used enough on ESR to provoke that much impact.

Comment 16

[Marco Bonardo [:mak]]

well, once it starts crashing it will crash continuously, I'm not sure if the top brash algorithm excludes duplicates.
Btw, so far we didn't find any other possible cause for that.

Comment 17

[Robert Kaiser]

(In reply to Marco Bonardo [:mak] from comment #16)
> I'm not sure if
> the top brash algorithm excludes duplicates.

It doesn't.

Comment 18

[Alex Keybl [:akeybl]]

mak - can we prepare a patch for the mozilla-esr17 branch? This is a top crasher there as well. Thanks!

Comment 19

[Marco Bonardo [:mak]]

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

The patch applies cleanly.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: top crash
User impact if declined: crash
Fix Landed on Version: 19
Risk to taking this patch (and alternatives if risky): low risk, null checks + test
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 20

[Robert Kaiser]

FYI, this is slightly rising on 18 now, only #42 on the topcrash list so not in topcrash list right now, but I'm happy we have fixed it for 19. ;-)

Comment 21

[bhavana bajaj [:bajaj]]

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

Approving on esr17.Please go ahead with the landing .

Comment 22

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/4059b706e4db

Comment 23

[Paul Silaghi, QA [:pauly]]

No crashes on FF > 18 based on crash stats. I guess we can call this verified fixed.

Comment 24

[Paul Silaghi, QA [:pauly]]

This was fixed on ESR on 2013-01-23, but there are no crashes on ESR > 17.0.2esr 20130107124423 (release) - probably not a lot of users use ESR nightly. I suggest waiting for ESR 17.0.3 before marking this verified.