All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla2.0b7

Status

()

--
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jdm, Assigned: sdwilsh)

Tracking

({crash})

unspecified
mozilla2.0b7
x86
Android
crash
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(fennec2.0b2+)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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
(Reporter)

Updated

8 years ago
tracking-fennec: --- → ?
(Assignee)

Comment 1

8 years ago
Looks like somebody isn't calling unregister when they go away like they are supposed to...
(Reporter)

Updated

8 years ago
Severity: normal → critical

Comment 2

8 years ago
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

8 years ago
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

8 years ago
Created attachment 486260 [details] [diff] [review]
patch v.1
Assignee: nobody → doug.turner
Attachment #486260 - Flags: review?(sdwilsh)
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

8 years ago
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.
We see several of these crashes on crash-stats for Fennec. Can we escalate this bug? I'd like it for b2 (ASAP)
tracking-fennec: ? → 2.0b2+
(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.
(Assignee)

Updated

8 years ago
Summary: Crash [@ mozilla::places::History::NotifyVisited] → IPC-only Crash [@ mozilla::places::History::NotifyVisited]
(Assignee)

Comment 9

8 years ago
Created attachment 486388 [details] [diff] [review]
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.
Assignee: doug.turner → sdwilsh
Attachment #486260 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #486388 - Flags: review?(doug.turner)
Attachment #486260 - Flags: review?(sdwilsh)

Updated

8 years ago
Attachment #486388 - Flags: review?(doug.turner) → review+
http://hg.mozilla.org/mozilla-central/rev/16b8b0c76065
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b7
Crash Signature: [@ mozilla::places::History::NotifyVisited]
You need to log in before you can comment on or make changes to this bug.