296275 - forward button history throws error

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050601
Firefox/1.0+ ID:2005060113

repro:
1.click any link on this page
2.press menu back button
3.press menu forward button

Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsISHistory.getEntryAtIndex]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://browser/content/browser.js :: FillHistoryMenu :: line 2457"  data: no]

regression bug 285647

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;

credits: [e-mail]

Comment 1

[Grayson Mixon (ColdFusion650)]

the fix works for me. im nominating blocking1.8b3.

Comment 2

[tabmix.onemen]

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

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 185065 [details] [diff] [review]
fix uncaught exception in the case end == count

This regresses bug 285647, so it's not acceptable.

Comment 4

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

bug 296305 is for suite.

Comment 5

[tabmix.onemen]

(In reply to comment #3)
> (From update of attachment 185065 [details] [diff] [review] [edit])
> This regresses bug 285647, so it's not acceptable.
> 

no no no
it does not regresses bug 285647, i can still count from 1.... to 15. :)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I've tested it, and it does regress the bug. Are you sure you're testing
correctly? Visit more than fifteen pages, so that the back menu has more than 15
entries, then use the back button to go back to the first page you visited. You
then see only 14 entries in the forward button dropdown.

Comment 7

[tabmix.onemen]

(In reply to comment #6)
> I've tested it, and it does regress the bug. Are you sure you're testing
> correctly? Visit more than fifteen pages, so that the back menu has more than 15
> entries, then use the back button to go back to the first page you visited. You
> then see only 14 entries in the forward button dropdown.

i'm very sure. can you check again..... 

Comment 8

[Jesse Ruderman]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050607
Firefox/1.0+

Using the steps in comment 0, I see the error message in the JavaScript console,
but clicking an item in the forward menu works anyway.

Comment 9

[Caleb]

(In reply to comment #6)
> I've tested it, and it does regress the bug. Are you sure you're testing
> correctly? Visit more than fifteen pages, so that the back menu has more than 15
> entries, then use the back button to go back to the first page you visited. You
> then see only 14 entries in the forward button dropdown.

It does not regress the bug, I've tested it and it makes perfect sense!

Comment 10

[Caleb]

(In reply to comment #9)

Let me explain :)

Basically, in the case of having less than 16 items in the history |end| is
being assigned to |count|, and the for loop uses j <= end, which is invalid
because count is one past the last item in the session history.

So by returning count - 1 (once again, this is only for the case of having less
than 16 items) will give us a proper |end| which points to the last item in the
history.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: Updated

Sorry, this does appear to work. I must have made a mistake in testing it
earlier. Here's the same patch, but unbitrotted.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: Fix help and browser

Urgh, forgot the help portion. Sorry for the spam.

Comment 13

[Caleb]

Comment on attachment 187997 [details] [diff] [review]
Fix help and browser

Requesting approval for a small/safe patch that deals with an uncaught
exception.

Comment 14

[Asaf Romano (gone)]

Checking in toolkit/components/help/content/help.js;
/cvsroot/mozilla/toolkit/components/help/content/help.js,v  <--  help.js
new revision: 1.33; previous revision: 1.32
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.453; previous revision: 1.452
done

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Reopening to reassign. [e-mail] fixed this, not me :).