Unintended (?) differences between LOAD_NORMAL and LOAD_NORMAL_EXTERNAL

VERIFIED FIXED in mozilla1.8beta4

Status

()

Core
Document Navigation
P2
normal
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: bz, Assigned: Biesinger)

Tracking

({fixed1.8})

Trunk
mozilla1.8beta4
fixed1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b4 +
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
We should really get this fixed before we ship these changes...
Flags: blocking1.8b4?

Updated

13 years ago
Flags: blocking1.8b4? → blocking1.8b4+
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?
(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
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

13 years ago
probably causes bug 300114

Comment 6

13 years ago
bz, biesi, can one of you look into this? Dan's on vacation and we'd like to get
this. 
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.

Updated

13 years ago
Blocks: 295058
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.
Assignee: dveditz → cbiesinger
Target Milestone: --- → mozilla1.8beta4

Updated

13 years ago
Flags: blocking1.8b4rc?

Updated

13 years ago
Flags: blocking1.8b4rc? → blocking1.8b4rc+

Comment 9

13 years ago
Bz and/or Biesi - either of you going to take a look at this?  If not who else
can help?  
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.
>    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.
(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?
Created attachment 194496 [details] [diff] [review]
part I: Fix fastback

Sorry for splitting this up in two parts, but I distributed the patch over two
computers...
Attachment #194496 - Flags: superreview?(bzbarsky)
Attachment #194496 - Flags: review?(bryner)
Created attachment 194497 [details] [diff] [review]
part II: Fix anchor scrolling
Attachment #194497 - Flags: superreview?(bzbarsky)
Attachment #194497 - Flags: review?(bryner)
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Attachment #194496 - Flags: review?(bryner) → review+
Attachment #194497 - Flags: review?(bryner) → review+
> CreateAboutBlankContentViewer sets mCurrentURI to about:blank

Ah, ok.  Clearly I should have read CreateAboutBlankContentViewer() a lot more
carefully... :(
Attachment #194496 - Flags: superreview?(bzbarsky) → superreview+
Attachment #194497 - Flags: superreview?(bzbarsky) → superreview+
This has nothing to do with bug 295058 and bug 300114
No longer blocks: 295058, 300114
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 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
Attachment #194496 - Flags: approval1.8b4?

Comment 19

13 years ago
once that first fix is verified on the trunk, please re-request approval for the
branch. thanks.
Attachment #194496 - Flags: approval1.8b4?
marking fixed, if someone thinks I should check in the shistory patch please
speak up.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
I meant the anchor scrolling one.
I think both patches should go on the branch, if they test well on the trunk.

/be
(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?

Updated

13 years ago
Attachment #194496 - Flags: approval1.8b4?

Updated

13 years ago
Attachment #194497 - Flags: approval1.8b4?

Comment 24

13 years ago
after this is verified on the trunk, we'll consider for branch approval.
need a tescase.  How do we test/verify this?
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

13 years ago
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 . . .
(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)

Updated

13 years ago
Attachment #194497 - Flags: approval1.8b4? → approval1.8b4+

Comment 29

13 years ago
Alright - verified best I could.  
Status: RESOLVED → VERIFIED
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.
Attachment #194497 - Attachment is obsolete: true

Updated

13 years ago
Attachment #194496 - Flags: approval1.8b4? → approval1.8b4+
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
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.