376581 - ASSERT: null node when deleting last history item

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Arie Paap [:wildmyron]]

I get an Assertion Failed with recent trunk nightly builds on WinXP SP1 when deleting the last item in the history. Tested with new profile.

Steps to reproduce:
1) Create New Profile
2) Load Firefox
3) Open History sidebar
4) Expand History for today
5) Right click and Delete each item

After deleting the last item I get the assertion in a dialog:
ASSERT: null node
Stack Trace:
0:PU_nodeIsFolder(null)
1:PC__hasRemovableSelection()
2:PC_isCommandEnabled(cmd_cut)
3:isCommandEnabled(cmd_cut)
4:goUpdateCmd(cmd_cut)
...
15:onCommand([object XULCommandEvent])

(Sorry, couldn't be bothered typing the rest out)

If I dismiss the dialog, Fx hangs and eventually displays the Unresponsive script warning. Dismissing it either way just sees it coming back in another ~10s. 

Using Clear Private Data to clear the history also Asserts, but the behaviour is slightly different.
ASSERT: null node
Stack Trace:
0:PU_nodeIsContainer(undefined)
1:PTV_isContainer(0)

Followed by a barrage of similar Assertions (some of them with the same stack) until the browser becomes responsive again. It's pretty much screwed at this point and I generally need to restart or kill the process.

Regression some time between 20070320 and 20070326

Comment 1

[tabmix.onemen]

WFM

no problem on build 2007040504

but there is another bug https://bugzilla.mozilla.org/show_bug.cgi?id=376706

Comment 2

[tabmix.onemen]

(In reply to comment #1)
> WFM
> 
> no problem on build 2007040504
> 
> but there is another bug https://bugzilla.mozilla.org/show_bug.cgi?id=376706
> 

sorry the bug still exist but only in 
Date and Site view or Date view

Comment 3

[tabmix.onemen]

can you test this (sorry that i can't post the patch at the moment)

fix in controller.js

  _removeRowsFromHistory: function PC__removeRowsFromHistory() {
    // Other containers are history queries, just delete from history
    // history deletes are not undoable.
    var nodes = this._view.getSelectionNodes();
    for (var i = 0; i < nodes.length; ++i) {
      var node = nodes[i];
      var bhist = PlacesUtils.history.QueryInterface(Ci.nsIBrowserHistory);
      if (PlacesUtils.nodeIsHost(node))
        bhist.removePagesFromHost(node.title, true);
      else if (PlacesUtils.nodeIsURI(node))
        bhist.removePage(PlacesUtils._uri(node.uri));
    }

+   // make sure the index for restore selection is valid
+   // when we deleting the last node in the last container from the view 
+   var range = this._view._nextSelection[0];
+   var index = Math.min(range.max, this._view.view.rowCount - 1);
+   this._view._nextSelection = [{ min: index, max: index }];
  },

Comment 4

[[:Aleksej]]

Confirmed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a4pre) Gecko/20070409 Minefield/3.0a4pre, please change the OS to All.

Comment 5

[Arie Paap [:wildmyron]]

Sorry for delay, I was away for Easter.

The patch in comment 3 fixes the issue using my debug build on WinXP SP2. Tested deleting the last history item in both "Date" and "Date and Site" views. I haven't tested rigorously but nothing appeared to break.

Comment 6

[tabmix.onemen]

Attachment: patch

since in history view we removing the container after last child was removed we need to fix the saved "_nextSelection" and make sure the index for restore selection is valid.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 262504 [details] [diff] [review]
patch

this make controller js assume a tree-view consumer, could we rather fix restoreSelection in tree.xml to check whether the saved-selection is valid?

Comment 8

[tabmix.onemen]

Attachment: patch restoreSelection in tree.xml

Comment 9

[Asaf Romano (gone)]

Comment on attachment 262559 [details] [diff] [review]
patch restoreSelection in tree.xml

s/fix/fixes

r=mano, thanks.

Comment 10

[Asaf Romano (gone)]

mozilla/browser/components/places/content/tree.xml 1.62

Comment 11

[Arie Paap [:wildmyron]]

Verified fixed with debug build from cvs pulled at ~"2007-04-24 09:00 PDT"

Comment 12

[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