96230 - back button fails to restore scroll position on local file bookmarks.html

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[James Paige]

Using build 2001082008 on Windows 95 (First noticed in a 20010817 build)

View your bookmarks.html file. I am not talkign about viewing it in the sidebar
or the bookmarks menu or anything, I mean pick "Open File..." from the "File"
menu and open your bookmarks.html file from your profile

Scroll down a little bit, and click any link, and wait for whatever destination
page to load

Now click "back" and you will be taken to the top of the bookmarks.html file,
rather than whatever position you had scrolled down to

Comment 1

[James Paige]

Attachment: bookmarks.html file for a testcase (in case yall are too lazy to find your own :)

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** This bug has been marked as a duplicate of 58785 ***

Comment 3

[James Paige]

I disagree with this bug being marked duplicate of bug 58785

Although the symptom is similar, the description for that bug talks about pages
that contain a "large amount of varied content" and it has comments which talk
about reproducability only on complex table structures or only when using the
back button when the page has not fully loaded.

This bug is 100% reproducable on this simple bookmarks.html file with no tables
regardless of whether anything has finished loading or not. This bug is also
new, In the August 10 2001 build this worked fine

Comment 4

[James Paige]

Adding the regression keyword

Please see bug 60075 for a description of symptoms very very sumilar to this bug
which have been mysteriously fixed and mysteriously regressed over and over and
over. The only reason I opened this bug instead of re-opening bug 60075 is
because the simplified testcase on that bug is no longer useful.

Comment 5

[James Paige]

grrr... What on earth is happening?
My testcase attachment 46599 [details] will not reproduce the symptom, but when I view it
off my harddrive, it NEVER remembers the scroll position

Yarg. My head hurts.

Comment 6

[James Paige]

Okay. This only happens when you view the bookmarks.html from a local file. It
does not work if you have loaded the bookmarks.html via http://

reducing severity, and adjusting the description for better accuracy

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

cc:evaughan

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

evaughan, radha: any idea what's going on here?

Comment 9

[Radha on family leave (not reading bugmail)]

I will take a look t make sure that the problem is not session history or in
docshell.

Comment 10

[Radha on family leave (not reading bugmail)]

cc'ing darin.
Here's what's going on:

when a page is loaded from the local filesystem, cache is not creating a
cachetoken for it. If a page has no cacheToken, sessionhistory will not save
form values and scrollbar positions in it, because I understand cacheToken is
not created when a page has no-store directives in it. 

Darin,  why are cache tokens not created for local files? How can docshell
differentiate this scenario from a no-store situation? I guess this is one of
those situations where docshell or session history can use some convinience
routines from cache to arrive at right conclusions. 

Comment 11

[Darin Fisher]

local files are loaded via the file protocol.  nsFileChannel does not implement
nsICachingChannel because there is no caching involved.

you may simply want to check the URL scheme to determine if the URL references a
local file.  you could alternatively QI the nsIChannel to nsIFileChannel.

Comment 12

[Radha on family leave (not reading bugmail)]

Darin, nsICachingChannel is created/not created  for which other protocols?

Comment 13

[Darin Fisher]

http://lxr.mozilla.org/seamonkey/search?string=nsICachingChannel

Comment 14

[Radha on family leave (not reading bugmail)]

Attachment: patch to docshell

Comment 15

[Darin Fisher]

ok, so you're special casing httpChannels that do not have a cache entry.  this
we know to correspond to data sent with a 'cache-control: no-store' header. 
this also could correspond to the case in which http caching has been disabled
(which can be controlled via a pref).  is this a problem?

Comment 16

[Radha on family leave (not reading bugmail)]

Darin: That depends on what happens to back/forward operations when cache is
disabled.  if we would be hitting the servers for back/forward (when cache is
disabled) then the proposed patch will provide proper behavior. Otherwise, I
need other tools to distinguish this situation from 'cache-control:no store' cases.

Comment 17

[Darin Fisher]

radha: yes, when the cache is disabled, we should be hitting the server for
back/forward operation.

Comment 18

[Darin Fisher]

Comment on attachment 52598 [details] [diff] [review]
patch to docshell

sr=darin

Comment 19

[Radha on family leave (not reading bugmail)]

*** Bug 103555 has been marked as a duplicate of this bug. ***

Comment 20

[Bradley Baetz (:bbaetz)]

Nit: Don't you want to check for cacheChannel && !cacheToken?

This is irrelevent for now, since only http (and view-source) implements that
interface. (not sure why ftp doesn't - darin?)

Why do we not want to save the position for no-store, anyway?

Comment 21

[Darin Fisher]

radha: actually, we may also be hitting the server for back/forward even when
the cache is enabled and the server did not send 'cache-control: no-store' ...
perhaps, if we really want to disable saving the scroll position for no-store
content, then we should check the httpchannel for the 'cache-control: no-store'
header.  checking the cache entry only happens to correspond to the no-store
case right now... in the future, it could correspond to other cases as well.

however, come to think of it, i too am forgetting why we don't want to save the
scroll position for no-store content.  i think i may be missing something about
what the saveLayoutStateFlag attribute means.

Comment 22

[Radha on family leave (not reading bugmail)]

Saving scrollbar position is currently tied with  saving form element values, 
done by the Presentation Shell. One call to PresShell does both the job. Since
form value restoration is not done on pages with 'cache-control: no store',
scroll bar position is also not saved and restored. Eric pollmann wrote that
code. Now that he is gone, I don't want to venture too much in to that area,
(atleast until I take care of my other important bugs)

Comment 23

[Darin Fisher]

make sense, but how about checking for the 'cache-control: no-store' header
directly, instead of inferring it from a NULL cacheToken?

  nsXPIDLCString val;
  httpChannel->GetResponseHeader("Cache-Control", getter_Copies(val));
  if (PL_strcasestr(val, "no-store"))
    entry->SetSaveLayoutStateFlag(PR_FALSE);

Comment 24

[Bradley Baetz (:bbaetz)]

radha: This was done as part of the 6.1 just-before-release stuff for
cache-control: no-store (bug 93027)

The problem was that we were storing form data in session history, and then
going back reloaded form elements. Several fixes went onto the trunk after 6.1
(including basing this on the autocomplete attr, rather than the cache headers,
bug 93972). I do not know if it is still needed. We could take it out and see if
the relevent testcases still pass, I guess, but thats a separate bug.

However, after sicking/jkieser's form rewrite, this will all change anyway, I
suppose.

Comment 25

[Radha on family leave (not reading bugmail)]

Attachment: Updated patch

Comment 26

[Radha on family leave (not reading bugmail)]

I have submitted a patch incorporating darin's suggestions. This patch works on
sites with cache control: no-store as well as in simple  ftp or directory viewer.

Comment 27

[Darin Fisher]

Comment on attachment 52786 [details] [diff] [review]
Updated patch

sr=darin

Comment 28

[Adam Lock]

Comment on attachment 52786 [details] [diff] [review]
Updated patch

r=adamlock

Comment 29

[Radha on family leave (not reading bugmail)]

Fix checked in.

Comment 30

[James Paige]

This seems *partly* fixed testing with Build 2001101203 on Windows 95

Starting from my local bookmarks.html file, If I click on a link, let the page
load completely, and then click back, scroll position is remembered, but if I
click back  before the page finishes loading, when the barberpole is still
spinning, my scroll position is lost, and I end up back at the top of my
bookmarks.html file.

Comment 31

[Arun Ranganathan]

If Darin's comment in bug 104181 is correct, then in order for bug 104181 to
land on the 0.9.4 branch (which we want at this stage) this *also* has to land
on that branch.  Bug 104181 has PDT+; therefore this one may need that too.
We've got r | sr | on this.  Let's get PDT/a=.  I'm slapping a PDT+ on it; if
this poses problems, spam me.

Comment 32

[Arun Ranganathan]

removing PDT+ -- waiting for jaimejr or others to do this.

Comment 33

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]