285647 - The maximum number of entries in the forward button dropdown list is only 14, in the back button dropdown list are up to 15 Elements

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[juergenbinder]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050310 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050310 Firefox/1.0+

The back button dropdown list has 15 or less entries, but the forward button
dropdown list has only 14 or less entries.

Fixing: 
Add in the line 2446 "+ 1" before the colon: "? index + MAX_HISTORY_MENU_ITEMS +
1 :" fixes the bug.
http://lxr.mozilla.org/aviary101branch/source/browser/base/content/browser.js#2446

Reproducible: Always

Steps to Reproduce:
1. Have enough pages in forward and back history
2. click the buttons
3. count
Actual Results:  
The back button dropdown list has 15 or less entries, but the forward button
dropdown list has only 14 or less entries.

Expected Results:  
Both dropdown list should have 15 entries

Comment 1

[Frank Wein [:mcsmurf]]

That fix doesn't look "the right one", see this:
for (j = index - 1; j >= end; j--) vs. for (j = index + 1; j < end; j++)
If we chang end by your fix, we might end up with another bug.

Comment 2

[juergenbinder]

I don't get your point. You should add the "+1" only in the forward case, of
course, as the back case works right. And there it works right. If (count -
index) is less than or equal than MAX_HISTORY_MENU_ITEMS, you get end = count,
so just all available forward item are added. If (count - index) is greater than
MAX_HISTORY_MENU_ITEMS, you should add MAX_HISTORY_MENU_ITEMS items to the
dropdown list. But if end = index + MAX_HISTORY_MENU_ITEMS, you add only
MAX_HISTORY_MENU_ITEMS - 1 items, as the for-loop goes from index + 1 only until
(index + MAX_HISTORY_MENU_ITEMS - 1) < end [=index + MAX_HISTORY_MENU_ITEMS]. If
you set end = index + MAX_HISTORY_MENU_ITEMS + 1, the for loop goes form index +
1 until index + MAX_HISTORY_MENU_ITEMS < end and adds so MAX_HISTORY_MENU_ITEMS. 

I fix it in this manner in my Extension, so you can test it, that it works so.
http://browseimages.mozdev.org 

Comment 3

[Gary van der Merwe]

Attachment: Patch according to reporter.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 179519 [details] [diff] [review]
Patch according to reporter.

This is wrong, and a silly hack, as comment 2 pointed out.

Changing j < end to j =< end to match the back syntax is the right fix for
consistency if we're trying for 15.

whoever attaches a correct fix, please fix
http://lxr.mozilla.org/mozilla/source/toolkit/components/help/content/help.js#4
20 in the same patch.

Comment 5

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 6

[juergenbinder]

Now you get an uncaught exception in the case end == count. In this case in the
last cycle of the for-loop is j == count == sessionHistory.count. That's an
invalid index for sessionHistory.getEntryAtIndex(j, false).

Comment 7

[Asa Dotzler [:asa]]

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

a=asa

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

checked in

Comment 9

[tabmix.onemen]

(In reply to comment #6)
> Now you get an uncaught exception in the case end == count. In this case in the
> last cycle of the for-loop is j == count == sessionHistory.count. That's an
> invalid index for sessionHistory.getEntryAtIndex(j, false).

they need to fix it by change this
end  = ((count-index) > MAX_HISTORY_MENU_ITEMS) ? index + MAX_HISTORY_MENU_ITEMS
: count;

with

end  = ((count-index) > MAX_HISTORY_MENU_ITEMS) ? index + MAX_HISTORY_MENU_ITEMS
: count-1;

Comment 10

[tabmix.onemen]

fix:
        case "forward":
          end  = ((count-index) > MAX_HISTORY_MENU_ITEMS) ? index +
MAX_HISTORY_MENU_ITEMS : count-1;
          if ((index + 1) > end) return false;
          for (j = index + 1; j <= end; j++)
            {
              entry = sessionHistory.getEntryAtIndex(j, false);
              if (entry)
                createMenuItem(aParent, j, entry.title);
            }
          break;

Comment 11

[tabmix.onemen]

Attachment: fix uncaught exception in the case end == count

Comment 12

[Peter van der Woude [:Peter6]]

(In reply to comment #10)
> fix:
>         case "forward":
>           end  = ((count-index) > MAX_HISTORY_MENU_ITEMS) ? index +
> MAX_HISTORY_MENU_ITEMS : count-1;
>           if ((index + 1) > end) return false;
>           for (j = index + 1; j <= end; j++)
>             {
>               entry = sessionHistory.getEntryAtIndex(j, false);
>               if (entry)
>                 createMenuItem(aParent, j, entry.title);
>             }
>           break;

-> Bug 296275