885666 - crash in mozilla::places::::NotifyPlaceInfoCallback::Run

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Scoobidiver (away)]

It first showed up in 24.0a1/20130620. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d2a7cfa34154&tochange=8ea92aeab783

Signature 	mozilla::places::`anonymous namespace''::NotifyPlaceInfoCallback::Run() More Reports Search
UUID	f61e744e-fb4a-4de2-856c-475202130621
Date Processed	2013-06-21 06:17:43
Uptime	2763
Last Crash	8.7 weeks before submission
Install Age	8.6 hours since version was first installed.
Install Time	2013-06-20 21:38:42
Product	Firefox
Version	24.0a1
Build ID	20130620031010
Release Channel	nightly
OS	Windows NT
OS Version	6.2.9200
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 58 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x11c0, AdapterSubsysID: 354e1458, AdapterDriverVersion: 9.18.13.1422
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
Processor Notes 	sp-processor04_phx1_mozilla_com_29267:2012
EMCheckCompatibility	True
Adapter Vendor ID	0x10de
Adapter Device ID	0x11c0
Total Virtual Memory	2147352576
Available Virtual Memory	1618272256
System Memory Use Percentage	25
Available Page File	3545165824
Available Physical Memory	2557546496

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::places::`anonymous namespace'::NotifyPlaceInfoCallback::Run 	toolkit/components/places/History.cpp:712
1 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:626
2 	xul.dll 	NS_ProcessNextEvent 	obj-firefox/xpcom/build/nsThreadUtils.cpp:238
3 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
4 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:212
5 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:186
6 	xul.dll 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
7 	xul.dll 	nsAppShell::Run 	widget/windows/nsAppShell.cpp:113
8 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:269
9 	xul.dll 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3856
10 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3924
11 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4137
12 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp:272
13 	firefox.exe 	NS_internal_main 	browser/app/nsBrowserApp.cpp:632
14 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:105
15 	firefox.exe 	__tmainCRTStartup 	crtexe.c:552
16 	kernel32.dll 	BaseThreadInitThunk 	
17 	ntdll.dll 	__RtlUserThreadStart 	
18 	ntdll.dll 	_RtlUserThreadStart

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Aplaces%3A%3A%60anonymous+namespace%27%27%3A%3ANotifyPlaceInfoCallback%3A%3ARun%28%29

Comment 1

[Scoobidiver (away)]

More reports also at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Aplaces%3A%3A%3A%3ANotifyPlaceInfoCallback%3A%3ARun

Comment 2

[Scoobidiver (away)]

More reports also at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Aplaces%3A%3A%28anonymous+namespace%29%3A%3ANotifyPlaceInfoCallback%3A%3ARun%28%29

Comment 3

[Scoobidiver (away)]

It might be a regression from bug 834539.

Comment 4

[Josh Matthews [:jdm]]

We have a callback argument that call clearly be null: http://hg.mozilla.org/mozilla-central/annotate/76820c6dff7b/toolkit/components/places/History.cpp#l834 (and in several other places)
We explicitly can't pass null callbacks to NotifyPlaceInfoCallback: http://hg.mozilla.org/mozilla-central/annotate/76820c6dff7b/toolkit/components/places/History.cpp#l672

Not rocket science.

Comment 5

[Marco Bonardo [:mak]]

NotifyPlaceInfoCallback should never be invoked if there's no callback, I think the problematic callpoint is here and was indeed added in that bug
http://hg.mozilla.org/mozilla-central/annotate/76820c6dff7b/toolkit/components/places/History.cpp#l877

Comment 6

[Marco Bonardo [:mak]]

Mano is looking at this.

Comment 7

[Asaf Romano (gone)]

Attachment: patch

Comment 8

[Asaf Romano (gone)]

Attachment: patch

Comment 9

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/07284493feb4

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/07284493feb4

Comment 11

[bhavana bajaj [:bajaj]]

(In reply to Mano from comment #9)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/07284493feb4

Mano, this was got to our attention in the stability meeting today and is a rising top-crasher,looks like this is fixed on FF25 and is uplift away for it to be fixed on Firefox 24 .Can you please request aurora uplift nomination here considering there are no outstanding dependencies ?

Comment 12

[Asaf Romano (gone)]

Comment on attachment 766771 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New API introduced in bug 834539
User impact if declined: top-crash
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): the patch is just a null-check, should be very safe.
String or IDL/UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Simona Badau, Desktop QA]

I found several crashes post fix on Firefox 24 beta and Firefox 26.0a1 that seems related with the first signature from this bug.
http://bit.ly/15B9j9K

Is this expected in any way?

Comment 15

[Marco Bonardo [:mak]]

could be a different reason, the original crash was due to a null check, but actually these may still indicate a null dereference due to GC. We should better handle mCallback ownership in NotifyPlaceInfoCallback. File a bug please?

Comment 16

[Simona Badau, Desktop QA]

(In reply to Marco Bonardo [:mak] from comment #15)
> could be a different reason, the original crash was due to a null check, but
> actually these may still indicate a null dereference due to GC. We should
> better handle mCallback ownership in NotifyPlaceInfoCallback. File a bug
> please?

Filed Bug 914653. 
Considering the low volume of crashes and that Bug 914563 was filed, setting the status for Firefox 24 to verified.

Comment 17

[u279076]

Seeing no crashes in Firefox 25 on crashstats with the signatures in this bug. Marking verified fixed.