300800 - Unintended (?) differences between LOAD_NORMAL and LOAD_NORMAL_EXTERNAL

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

The patch checked in for bug 298255 makes LOAD_NORMAL and LOAD_NORMAL_EXTERNAL
differ in some ways that seem pretty major to me.  At least, we have:

1)  LOAD_NORMAL_EXTERNAL isn't allowed to just scroll for anchor loads (because
    the old page is gone by that point, it can't really happen; best case is
    that we just reload the page, worst case is that we just show about:blank
    when this is tried).
2)  fastback isn't allowed for a page left via a LOAD_NORMAL_EXTERNAL load
    (again, because the page is gone).  More precisely, because we nuke the old
    viewer, I think fastback will end up finding the about:blank viewer, so
    going back to a page that we left via LOAD_NORMAL_EXTERNAL wouldn't work at
    all.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We should really get this fixed before we ship these changes...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Issue (3): Loading the same URI via LOAD_NORMAL_EXTERNAL will convert the load
to LOAD_NORMAL_REPLACE and reuse the old session history entry.  Has someone
tested that this actually works correctly given that we nuke the old viewer?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #1)
> We should really get this fixed before we ship these changes...

Too late -- 1.0.5 escaped the lab, and the patch for bug 298255 is in the 1.0.6
candidate builds; it caused bug 301073 as well.

/be

Comment 4

[Brian Ryner (not reading)]

I'm not retargetting this since it covers multiple issues, but the bfcache
portion of this is not a release blocker IMO since it just means we miss a cache
opportunity.  I was not able to reproduce any case where we return to the
about:blank viewer.

Comment 5

[Chris Beard]

probably causes bug 300114

Comment 6

[Asa Dotzler [:asa]]

bz, biesi, can one of you look into this? Dan's on vacation and we'd like to get
this. 

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm not going to have a chance to look at this for at least a week and a half. 
I suspect the right fix is to:

1)  Nuke the content viewer much later (after we try anchor scrolling, to be
precise).
2)  Ensure the resulting code plays nice enough with bfcache.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I probably won't be able to look for at least a week.  Note also bug 295058 and
bug 300114 which seem to be related and for which I have no idea why they happen.

Comment 9

[Mike Schroepfer]

Bz and/or Biesi - either of you going to take a look at this?  If not who else
can help?  

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

I'll try to look at this tomorrow. I want to note, though, that there is code in
docshell that changes LOAD_NORMAL_EXTERNAL to LOAD_NORMAL. this doesn't affect
the anchor scroll part of comment 0; haven't looked at the other parts yet.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

>    going back to a page that we left via LOAD_NORMAL_EXTERNAL wouldn't work at
>    all.

that should still work, as CreateAboutBlankContentViewer nulls out mOSHE.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #2)
> Issue (3): Loading the same URI via LOAD_NORMAL_EXTERNAL will convert the load
> to LOAD_NORMAL_REPLACE and reuse the old session history entry.  Has someone
> tested that this actually works correctly given that we nuke the old viewer?

bz, I'm not sure how you arrived at this conclusion... I don't see a way for
this to happen; CreateAboutBlankContentViewer sets mCurrentURI to about:blank,
thus the OnNewURI code that does that resetting should not fire. Am I missing
something?

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: part I: Fix fastback

Sorry for splitting this up in two parts, but I distributed the patch over two
computers...

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: part II: Fix anchor scrolling

Comment 15

[Boris Zbarsky [:bzbarsky]]

> CreateAboutBlankContentViewer sets mCurrentURI to about:blank

Ah, ok.  Clearly I should have read CreateAboutBlankContentViewer() a lot more
carefully... :(

Comment 16

[Daniel Veditz [:dveditz]]

This has nothing to do with bug 295058 and bug 300114

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 194497 [details] [diff] [review]
part II: Fix anchor scrolling

hm... this patch shouldn't actually be needed, now that I thought about it some
more: clearly, the URIs don't compare equal (see above about
CreateAboutBlankContentViewer setting mCurrentURI)

it's a bit of a perf enhancement I guess, and might make the code clearer...
should I check it in? I'm leaning towards no.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 194496 [details] [diff] [review]
part I: Fix fastback

patch checked in to trunk

This should be a pretty low risk patch to make fastback work when a page is
loaded externally (e.g. XRemote, or the windows remoting code)

Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.738; previous revision: 1.737
done

Comment 19

[Asa Dotzler [:asa]]

once that first fix is verified on the trunk, please re-request approval for the
branch. thanks.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

marking fixed, if someone thinks I should check in the shistory patch please
speak up.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

I meant the anchor scrolling one.

Comment 22

[Brendan Eich [:brendan]]

I think both patches should go on the branch, if they test well on the trunk.

/be

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #22)
> I think both patches should go on the branch, if they test well on the trunk.

the anchor scrolling one isn't currently checked in on trunk; see comment 17 as
for why. or, did you mean that I should check it in on trunk?

Comment 24

[Asa Dotzler [:asa]]

after this is verified on the trunk, we'll consider for branch approval.

Comment 25

[Tracy Walker [:tracy]]

need a tescase.  How do we test/verify this?

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

testcase:
- run seamonkey (or firefox)
- load a url in it
- click on a uri in another program (say, thunderbird)
  or: start/run, type in a uri
- wait for uri to load in firefox
- click back: Fastback should be used for going back here

testcase2:
- load a url that includes named anchors (e.g. lxr)
  e.g. http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp
- start/run, use the URI from before, but make sure that it includes a named anchor
  e.g. http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5160
The end result should be that the URI is loaded + scrolled to that anchor

Comment 27

[Mike Schroepfer]

Sorry if this is a stupid question - but:

testcase1: how do you tell if fastback is being used
testcase2: This passes for me on the 9-4 branch build.   Must be doing something
wrong . . .

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #27)
> testcase1: how do you tell if fastback is being used

That's a good question :) "it should be fast"

> testcase2: This passes for me on the 9-4 branch build.   Must be doing something
> wrong . . .

Nah, that's ok, just wanted to make sure (like comment 17 said, I didn't think
that that part needed any work)

Comment 29

[Mike Schroepfer]

Alright - verified best I could.  

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 194497 [details] [diff] [review]
part II: Fix anchor scrolling

actually, per the results of comment 27 this patch isn't needed... but approval
on the other one would still be good.

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

attachment 194496 [details] [diff] [review] checked in to MOZILLA_1_8_BRANCH:
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.719.2.14; previous revision: 1.719.2.13
done