Last Comment Bug 662200 - nsISHistory::requestedIndex is not reset after page reload through nsSHistory::Reload
: nsISHistory::requestedIndex is not reset after page reload through nsSHistory...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla8
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on: 670318 680344
Blocks: 622315
  Show dependency treegraph
 
Reported: 2011-06-05 16:09 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2012-05-18 12:25 PDT (History)
5 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.10 KB, patch)
2011-06-07 07:37 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bzbarsky: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-05 16:09:51 PDT
Start on page A, navigate to page B, then page C.  Click back.  Refresh (what is now page B).  The navigation buttons will be greyed out when they should not be.
Comment 1 Justin Lebar (not reading bugmail) 2011-06-05 19:27:24 PDT
I couldn't reproduce on a few sites I tried.  Can you try a more detailed str?
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-05 19:28:51 PDT
Are you trying a build with the patch from bug 622315?
Comment 3 Justin Lebar (not reading bugmail) 2011-06-05 19:30:54 PDT
Ah, nope.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-05 19:32:04 PDT
Yeah, this is a regression from that.  I backed it out since I didn't have any clue how to fix this.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-06 09:54:32 PDT
So the issue, presumably, is that nothing actually resets the requested index when the navigation completes?
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-06 09:54:54 PDT
Should we be comparing requested index to current index as well, not just to -1?
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-07 07:37:03 PDT
Created attachment 537785 [details] [diff] [review]
Patch

This fixes this behavior for me and passes the tests in docshell/

I'm going to try to write a mochitest for this behavior.
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-07 07:38:36 PDT
(In reply to comment #5)
> So the issue, presumably, is that nothing actually resets the requested
> index when the navigation completes?

Yes, this is correct.  The attached patch does that.

(In reply to comment #6)
> Should we be comparing requested index to current index as well, not just to
> -1?

I don't think so.  I think we should avoid navigating whenever there is a pending navigation.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-07 08:55:21 PDT
Comment on attachment 537785 [details] [diff] [review]
Patch

Hrm...  Changing this stuff is scary as hell, but ok...
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-07 09:48:50 PDT
Yeah, I'm terrified too :-)
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-07 12:49:58 PDT
So, fun thing, this only occurs if the reload is triggered through nsSHistory.  If the reload goes through nsDocShell we do an internal load and never muck with history.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-07 13:25:37 PDT
Patch:

http://hg.mozilla.org/projects/cedar/rev/580ddd2f1267

Regression test:

http://hg.mozilla.org/projects/cedar/rev/ea5ed0f2a952
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-10 06:55:48 PDT
This was backed out of Aurora to fix Bug 670318.

http://hg.mozilla.org/releases/mozilla-aurora/rev/cd5b516e9a39
http://hg.mozilla.org/releases/mozilla-aurora/rev/5cf936e830cf

Note You need to log in before you can comment on or make changes to this bug.