412751 - Crash after clearing browsing history with query result nodes

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ondrej Brablc]

Attachment: Crash Fix

In nsNavHistoryQueryResultNode::OnClearHistory() a Refresh() was called on node
which has been already unregistered from the history observer. However,
Refresh() calls FillChildren() what registers it again, just before it is
removed from memory. A call to nsNavHistoryResult::OnVisit() accessed invalid
memory when enumerating registered nodes. 

The fix is not to call FillChildren() when Refresh() is called from OnClearHistory().

This bug can most likely only happen when bug 385245 is implemented. I was not able to reproduce it in plain trunk.

Steps to reproduce
------------------
1) Open History sidebar
2) Sort "By Date".
3) Visit page A.
4) Click on "Today" label which appeared in the sidebar.
5) Clear the browsing history.
6) Visit page B.
7) A crash occurs.

Comment 1

[Marco Bonardo [:mak]]

i haven't yet looked at the code yet (will do as soon as i can), but would not be possibile to block the crashing in fillchildren or intercept some kind of exception without changing refresh function?

Comment 2

[Ondrej Brablc]

The problem is that we are deleting the node, so it really does not make much sense to fill it up with children at this moment. May be we could replace the call to Refresh() in OnClearHistory with the ClearChildren code from Refresh(), which would probably lead to a duplicate code.

Comment 3

[Marco Bonardo [:mak]]

i think that fillchildren should return without doing anything if the node is not valid...

imho don't call a function to avoid a crash is not the solution, the function itself should take care of that with an early return

Comment 4

[Ondrej Brablc]

OK, I can do that, I just do not know how to recognize that the node is not valid.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 297515 [details] [diff] [review]
Crash Fix

removing review request pending resolution of comment #4

Comment 6

[Ondrej Brablc]

Attachment: Prevent observer calls on cleared nodes

I have found a place closer to the source of the crash. The reason was, that a refresh was called on a node, which has already been cleared, this added the node to the weak array mHistoryObservers right before the node has been released. Next enumeration of this array accessed invalid memory.

You can see that the check for null pointer (which will never be negative) has been replaced with check, that the node must be a root node or it must have a parent. When an item is cleared its parent is set to null.

Follow STR in comment #0.

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
...
> The reason was, that a
> refresh was called on a node, which has already been cleared, this added the
> node to the weak array mHistoryObservers right before the node has been
> released. Next enumeration of this array accessed invalid memory.
...
> When an item is cleared its parent is set to null.

thanks for following this up Ondrej.

given the comments above, could we instead check mParent in Refresh() and return if null, thereby not adding the observer in the first place? seems better to kick out early if we're sure that the node is invalid for some reason.

Comment 8

[Ondrej Brablc]

Attachment: Move the check to the Refresh method

(In reply to comment #7)
> ...
> given the comments above, could we instead check mParent in Refresh() and
> return if null, thereby not adding the observer in the first place? seems
> better to kick out early if we're sure that the node is invalid for some
> reason.

The previous patch prevented observers from being added on invalid nodes. It actually prevented any method to be called on such nodes. I have moved the check inside of Refresh() now. This way it is the invalid node who does the decision whether it should do something or not. I'm not sure what is better, but this looks nicer.

Comment 9

[Ondrej Brablc]

Attachment: Move the check to the Refresh method (fixed comment)

Sorry for spamming, just fixing the comment.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 302601 [details] [diff] [review]
Move the check to the Refresh method  (fixed comment)

r=me, thanks!

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 302601 [details] [diff] [review]
Move the check to the Refresh method  (fixed comment)

drivers: low impact change that fixes a crash scenario.

Comment 12

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  nsNavHistoryResult.cpp
new revision: 1.133; previous revision: 1.132
done

Comment 13

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv