Closed Bug 607469 Opened 14 years ago Closed 14 years ago

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

Categories

(Toolkit :: Places, defect)

x86
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: jdm, Assigned: sdwilsh)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
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 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?
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 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.
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
Status: NEW → ASSIGNED
Attachment #486388 - Flags: review?(doug.turner)
Attachment #486260 - Flags: review?(sdwilsh)
Attachment #486388 - Flags: review?(doug.turner) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 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.

Attachment

General

Created:
Updated:
Size: