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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Places
--
major
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({crash, fixed1.9.1})

Trunk
mozilla1.9.2a1
crash, fixed1.9.1
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
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);
(Assignee)

Comment 1

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

Comment 2

10 years ago
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.
(Assignee)

Comment 3

10 years ago
Actually now I continue from there it removes the second observer too.
(Assignee)

Comment 4

10 years ago
Created attachment 354007 [details] [diff] [review]
Proposed patch

This seems to fix it for me.
It looks wrong not to have the batch scoping the removal anyway.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #354007 - Flags: review?(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
Attachment #354007 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 6

10 years ago
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...
Keywords: helpwanted
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.
(Assignee)

Comment 8

10 years ago
Pushed changeset 96a66c1a50bd to mozilla-central.
This was backed out for failing unit tests.
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.)
pushed again
http://hg.mozilla.org/mozilla-central/rev/412e50a4e320
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: helpwanted
Resolution: --- → FIXED

Updated

10 years ago
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 354007 [details] [diff] [review]
Proposed patch

requesting approval for 1.9.1 branch
Attachment #354007 - Flags: approval1.9.1?
this is low risk, and potentially fixing a quite common crash cause.
Flags: wanted1.9.1?
Attachment #354007 - Flags: approval1.9.1? → approval1.9.1+
What happened to the test asked for in comment 7?
Flags: in-testsuite?
(Assignee)

Comment 16

9 years ago
(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.
You need to log in before you can comment on or make changes to this bug.