Closed Bug 607469 Opened 12 years ago Closed 12 years ago

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


(Toolkit :: Places, defect)

Not set



Tracking Status
fennec 2.0b2+ ---


(Reporter: jdm, Assigned: sdwilsh)


(Keywords: crash)

Crash Data


(1 file, 1 obsolete file)


0 	mozilla::places::History::NotifyVisited 	toolkit/components/places/src/History.cpp:1015
1 	mozilla::places::::UpdateFrecencyAndNotifyStep::Callback 	nsCOMPtr.h:492
2 	mozilla::places::Step::HandleCompletion 	nsCOMPtr.h:492
3 	mozilla::storage::::CompletionNotifier::Run 	storage/src/mozStorageAsyncStatementExecution.cpp:165
4 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:547
5 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:250
6 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:111
7 	MessageLoop::RunInternal 	ipc/chromium/src/base/
8 	MessageLoop::Run 	ipc/chromium/src/base/
9 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:186
10 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:192
11 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3684
12 	GeckoStart 	toolkit/xre/nsAndroidStartup.cpp:131
tracking-fennec: --- → ?
Looks like somebody isn't calling unregister when they go away like they are supposed to...
Severity: normal → critical
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.
When we startup, we load 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, 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:

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:

sdwilsh, mak, what say you?
Attached patch patch v.1 (obsolete) — Splinter Review
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 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.
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.
Summary: Crash [@ mozilla::places::History::NotifyVisited] → IPC-only Crash [@ mozilla::places::History::NotifyVisited]
Attached patch v2.0Splinter Review
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
Attachment #486388 - Flags: review?(doug.turner)
Attachment #486260 - Flags: review?(sdwilsh)
Attachment #486388 - Flags: review?(doug.turner) → review+
Closed: 12 years ago
Resolution: --- → FIXED
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.