470586 - Crash calling removePagesFromHost when sidebar is showing pages grouped by day and site

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[[e-mail]]

Stack backtrace:
>nsNavHistoryResult::OnEndUpdateBatch()  Line 4115      ENUMERATE_HISTORY_OBSERVERS(OnEndUpdateBatch());
>nsNavHistory::EndUpdateBatch()  Line 4183  ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver, OnEndUpdateBatch())
>UpdateBatchScoper::~UpdateBatchScoper()  Line 280  mNavHistory.EndUpdateBatch();
>nsNavHistory::RemovePagesFromHost(const nsACString_internal & aHost={...}, int aEntireDomain=0x00000001)  Line 4550  UpdateBatchScoper batch(*this);

Comment 1

[[e-mail]]

At the point of the crash, observerCopy.Length() > mHistoryObservers.Length() so it looks like the first observer removed the second.

Comment 2

[[e-mail]]

Slightly different crash just about to happen this time:

>nsNavHistoryResult::RemoveHistoryObserver(nsNavHistoryQueryResultNode * aNode=0x0587bf90)  Line 3935  mHistoryObservers.RemoveElement(aNode);
>nsNavHistoryQueryResultNode::ClearChildren(int aUnregister=0x00000001)  Line 2440  result->RemoveAllBookmarksObserver(this);
>nsNavHistoryQueryResultNode::OnRemoving()  Line 2125  ClearChildren(PR_TRUE);
>nsNavHistoryQueryResultNode::ClearChildren(int aUnregister=0x00000001)  Line 2433  mChildren[i]->OnRemoving();
>nsNavHistoryQueryResultNode::OnRemoving()  Line 2125  ClearChildren(PR_TRUE);
>nsNavHistoryQueryResultNode::ClearChildren(int aUnregister=0x00000000)  Line 2433  mChildren[i]->OnRemoving();
>nsNavHistoryQueryResultNode::Refresh()  Line 2485  (void)FillChildren();
>nsNavHistoryQueryResultNode::OnEndUpdateBatch()  Line 2578  return Refresh();
>nsNavHistoryResult::OnEndUpdateBatch()  Line 4115  ENUMERATE_HISTORY_OBSERVERS(OnEndUpdateBatch());
>nsNavHistory::EndUpdateBatch()  Line 4183  ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver, OnEndUpdateBatch())
>UpdateBatchScoper::~UpdateBatchScoper()  Line 280  mNavHistory.EndUpdateBatch();
>nsNavHistory::RemovePagesFromHost(const nsACString_internal & aHost={...}, int aEntireDomain=0x00000000)  Line 4550  UpdateBatchScoper batch(*this);

In this case the nsNavHistoryResult has three history observers. It's currently calling OnEndUpdateBatch on its first observer which removes the third observer.

Comment 3

[[e-mail]]

Actually now I continue from there it removes the second observer too.

Comment 4

[[e-mail]]

Attachment: Proposed patch

This seems to fix it for me.
It looks wrong not to have the batch scoping the removal anyway.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 354007 [details] [diff] [review]
Proposed patch

r=sdwilsh, but please add an xpcshell test case that will crash before this fix so we don't break this in the future please

Comment 6

[[e-mail]]

Well, apart from not knowing how to write crash tests in general, particularly not chrome crash tests, this test also needs to have the history sidebar open and expanded, and I don't know how to do that in xpcshell either...

Comment 7

[Shawn Wilsher :sdwilsh]

You don't need an actual crash test - if xpcshell crashes when the test is running, it will fail.

I'm not 100% sure which query the history sidebar uses, but I do believe that dietrich can point you in the right direction.

Comment 8

[[e-mail]]

Pushed changeset 96a66c1a50bd to mozilla-central.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This was backed out for failing unit tests.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I was the one who identified this patch as guilty, but I was wrong --- the test had been failing randomly for a week. Sorry! (I filed bug 473700 to find the real culprit.)

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

pushed again
http://hg.mozilla.org/mozilla-central/rev/412e50a4e320

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 354007 [details] [diff] [review]
Proposed patch

requesting approval for 1.9.1 branch

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this is low risk, and potentially fixing a quite common crash cause.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 354007 [details] [diff] [review]
Proposed patch

a191=beltzner

Comment 15

[Shawn Wilsher :sdwilsh]

What happened to the test asked for in comment 7?

Comment 16

[[e-mail]]

(In reply to comment #15)
> What happened to the test asked for in comment 7?
Running the query alone was insufficient to reproduce the crash. The crash wasn't 100% reproducible anyway, it must depend on the tree view and GC etc.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/aeb04030bf2f