607469 - IPC-only Crash [@ mozilla::places::History::NotifyVisited]

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Josh Matthews [:jdm]]

http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&date=2010-10-26%2010%3A00%3A00&signature=mozilla%3A%3Aplaces%3A%3AHistory%3A%3ANotifyVisited&version=Fennec%3A4.0b2pre

Sample: http://crash-stats.mozilla.com/report/index/526dbd77-fd36-42ba-986f-4f96b2101026

0 	libxul.so 	mozilla::places::History::NotifyVisited 	toolkit/components/places/src/History.cpp:1015
1 	libxul.so 	mozilla::places::::UpdateFrecencyAndNotifyStep::Callback 	nsCOMPtr.h:492
2 	libxul.so 	mozilla::places::Step::HandleCompletion 	nsCOMPtr.h:492
3 	libxul.so 	mozilla::storage::::CompletionNotifier::Run 	storage/src/mozStorageAsyncStatementExecution.cpp:165
4 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:547
5 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:250
6 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:111
7 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:220
8 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:512
9 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:186
10 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:192
11 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3684
12 	libxul.so 	GeckoStart 	toolkit/xre/nsAndroidStartup.cpp:131
13 	libc.so 	libc.so@0x10f47 	
14 	libc.so 	libc.so@0x10a33

Comment 1

[Shawn Wilsher :sdwilsh]

Looks like somebody isn't calling unregister when they go away like they are supposed to...

Comment 2

[Doug Turner (:dougt)]

maybe.... it could also be that we are getting a null uri somehow.  the preconditions would be noops, and we'd end up dereferencing null.

Comment 3

[Doug Turner (:dougt)]

When we startup, we load http://www.firefox.com/m/feedback/ in the chrome process.  This goes through the normal single process history flow.  We end up in RegisterVisitedCallback() and we pass a non-null aURI and a non-null aLink.

The mObserver has an entry for the given aURI, and it also has on aLink on the observer list.

Later on, if the user goes to a page that has an anchor that is the same as http://www.firefox.com/m/feedback/, we will end up in the same code in the chrome process but now we don't have a aLink.  Recall that a null aLink is perfectly fine in the chrome process as that indicates that the request is from a content process.

The code follows that if the chrome process is already looking for certain visited urls, it doesn't need to do anything, so we skip over this block:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/History.cpp#1207

However, that ends up with us adding our null aLink to the observer list.

There are a couple things we could do here ranging from the simple to the not so simple.  Given this is a crash and we like this to be fixed for fennec b2, I like to simple prevent null aLink in the observer list.


So, i want to add a null check here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/History.cpp#1228

sdwilsh, mak, what say you?

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 5

[Marco Bonardo [:mak]]

If the chrome process is sending in a null link (Looks like we are talking about http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#429) it makes much sense that we don't add it to the observers list.  I'd just clarify the reasons why we get a null link in the comment, what you wrote above looks fine to add "a null aLink is perfectly fine in the chrome process as that indicates that the request is from a content process"

Btw, since Shawn wrote this code in the first time, I think makes more sense for him to review.

Comment 6

[Doug Turner (:dougt)]

mak - thanks for the suggestion.  To avoid going back and forth, could you provide me with the exact comment you would like to use?  Otherwise, I can just use the comment I used when I posted this patch.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

We see several of these crashes on crash-stats for Fennec. Can we escalate this bug? I'd like it for b2 (ASAP)

Comment 8

[Marco Bonardo [:mak]]

(In reply to comment #6)
> mak - thanks for the suggestion.  To avoid going back and forth, could you
> provide me with the exact comment you would like to use?

As I said, what you wrote above is fine, I just care that when someone will look at that null check, he won't have to search in code to figure out why we need it.
That said, I'd wait for Shawn's review before touching the patch to add the comment.

Shawn, I don't recall if you changed this code piece on Places branch, in case it should be merged there to avoid regressing it on merge.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

I think this is a better/safer approach with some more comments in the code to explain how we'd end up in this state.  Also includes a test.

I don't have a build with IPC, so it'd be great if someone could check that the test passes on it.

Comment 10

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/16b8b0c76065