404496 - Assert in Organizer Views-show columns

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007111918 Minefield/3.0b2pre Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007111918 Minefield/3.0b2pre Firefox/3.0

Getting an Assert when using Views->show columns and trying to add a column. 

ASSERT: menu item for column that doesn't exist?! id = menucol_
Stack Trace: 
0:VM_showHideColumn([object XULElement])
1:oncommand([object XULCommandEvent])


Reproducible: Always

Steps to Reproduce:
1.
2.
3.



Vista HP 
This appears to have started with the landing of the Places update to sidebar.
https://bugzilla.mozilla.org/show_bug.cgi?id=387746

Comment 1

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

i can confirm on XP
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112005 Minefield/3.0b2pre

Comment 2

[Asaf Romano (gone)]

Attachment: patch

Comment 3

[Dietrich Ayala (:dietrich)]

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

>? browser/components/places/tests/log-file.dat
>Index: browser/components/places/content/places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
>retrieving revision 1.113
>diff -u -p -8 -r1.113 places.js
>--- browser/components/places/content/places.js	21 Nov 2007 01:35:15 -0000	1.113
>+++ browser/components/places/content/places.js	21 Nov 2007 02:20:10 -0000
>@@ -1399,23 +1399,24 @@ var ViewMenu = {
>     // so track whether or not we find a column that is sort-active.
>     var isSorted = false;
>     var content = document.getElementById("placeContent");
>     var columns = content.columns;
>     for (var i = 0; i < columns.count; ++i) {
>       var column = columns.getColumnAt(i).element;
>       var menuitem = document.createElement("menuitem");
>       menuitem.id = "menucol_" + column.id;
>-      menuitem.setAttribute("column", column.id);
>+      menuitem.column = column;
>       var label = column.getAttribute("label");
>       if (propertyPrefix) {
>         var menuitemPrefix = propertyPrefix;
>         // for string properties, use "name" as the id, instead of "title"
>         // see bug #386287 for details
>-        menuitemPrefix += (column.id == "title" ? "name" : column.id);
>+        var columnId = column.getAttribute("anonid");
>+        menuitemPrefix += columnId == "title" ? "name" : columnId;
>         label = PlacesUtils.getString(menuitemPrefix + ".label");
>         var accesskey = PlacesUtils.getString(menuitemPrefix + ".accesskey");
>         menuitem.setAttribute("accesskey", accesskey);
>       }
>       menuitem.setAttribute("label", label);
>       if (type == "radio") {
>         menuitem.setAttribute("type", "radio");
>         menuitem.setAttribute("name", "columns");
>@@ -1472,21 +1473,17 @@ var ViewMenu = {
>   },
> 
>   /**
>    * Shows/Hides a tree column.
>    * @param   element
>    *          The menuitem element for the column
>    */
>   showHideColumn: function VM_showHideColumn(element) {
>-    const PREFIX = "menucol_";
>-    var columnID = element.id.substr(PREFIX.length, element.id.length);
>-    var column = document.getElementById(columnID);
>-    NS_ASSERT(column,
>-              "menu item for column that doesn't exist?! id = " + element.id);
>+    var column = element.column;
> 
>     var splitter = column.nextSibling;
>     if (splitter && splitter.localName != "splitter")
>       splitter = null;
> 
>     if (element.getAttribute("checked") == "true") {
>       column.removeAttribute("hidden");
>       if (splitter)
>@@ -1511,45 +1508,51 @@ var ViewMenu = {
>       var sortDirection = column.getAttribute("sortDirection");
>       if (sortDirection == "ascending" || sortDirection == "descending")
>         return column;
>     }
>     return null;
>   },
> 
>   /**
>-   * Sorts the view by the specified key.
>-   * @param   aColumnID
>-   *          The ID of the colum that is the sort key. Can be null - the
>-   *          current sort column id or "title" will be used.
>+   * Sorts the view by the specified column.
>+   * @param   aColumn
>+   *          The colum that is the sort key. Can be null - the
>+   *          current sort column or the title column will be used.
>    * @param   aDirection
>    *          The direction to sort - "ascending" or "descending".
>    *          Can be null - the last direction or descending will be used.
>    *
>    * If both aColumnID and aDirection are null, the view will be unsorted.
>    */
>-  setSortColumn: function VM_setSortColumn(aColumnID, aDirection) {
>+  setSortColumn: function VM_setSortColumn(aColumn, aDirection) {
>     var result = document.getElementById("placeContent").getResult();
>-    if (!aColumnID && !aDirection) {
>+    if (!aColumn && !aDirection) {
>       result.sortingMode = Ci.nsINavHistoryQueryOptions.SORT_BY_NONE;
>       return;
>     }
> 
>-    var sortColumn = this._getSortColumn();
>-    if (!aDirection) {
>-      aDirection = sortColumn ?
>-                   sortColumn.getAttribute("sortDirection") : "descending";
>+    var columnId;
>+    if (aColumn) {
>+      columnId = aColumn.getAttribute("anonid")
>+      if (!aDirection) {
>+        var sortColumn = this._getSortColumn();
>+        aDirection = sortColumn ?
>+                     sortColumn.getAttribute("sortDirection") : "descending";
>+      }
>+    }
>+    else {
>+      var sortColumn = this._getSortColumn();
>+      columnId = sortColumn ? sortColumn.getAttribute("anonid") : "title";
>     }

need to set a default for aDirection in this case?

r=me otherwise.

Comment 4

[Asaf Romano (gone)]

[05:11]	<Mano>	dietrich: if (!aColumn && !aDirection) { is handled in the function top

Comment 5

[Asaf Romano (gone)]

mozilla/browser/components/places/content/places.js 1.114
mozilla/browser/components/places/content/places.xul 1.97

Comment 6

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007112604 Minefield/3.0b2pre

Comment 7

[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