Framed site doesn't create session history entries [bugscape 18199]

VERIFIED FIXED in mozilla1.4beta

Status

()

VERIFIED FIXED
17 years ago
11 years ago

People

(Reporter: doronr, Assigned: radha)

Tracking

(Blocks: 1 bug, {topembed+})

Trunk
mozilla1.4beta
x86
Windows 2000
topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] [bugscape 18199], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
http://www.mehndi-tempel.com/shop/index.html

Going to this site, and clicking on any link (for example the top links with a 
green background), it does not create an session history entry, and going back
goes to the page you were one prior to visiting said site.

Filed originaly as a bugscape german topsite issue -
http://bugscape.mcom.com/show_bug.cgi?id=18199
(Reporter)

Comment 1

17 years ago
This happens in both 1.0.1 branch and 1.1beta btw.
Whiteboard: [bugscape 18199]

Updated

17 years ago
Keywords: topembed

Updated

17 years ago
Keywords: nsbeta1
Summary: Framed site doesn't create session history entries → Framed site doesn't create session history entries [bugscape 18199]
Whiteboard: [bugscape 18199] → [adt2] [bugscape 18199] [ETA Needed]
when you click on the green link, most of the times the throbber never stops,
indicating that the browser is expecting more data from the site. This prevents
the back and forward buttons from updating. Sometimes however, the throbber
stops, and in those cases sessio history entries are created and the back
forward buttons are enabled. When this happens, the zuruck button works. I think
the problem is with the site. 

Comment 3

17 years ago
Dup of bug 160869?

Comment 4

17 years ago
Created attachment 97292 [details]
simple page that reproduces the error

open frameset.html on a new window.
Click some links and you will notice that history doesn't change.
It has something to do with javascript (specialy with location.replace)

Comment 5

17 years ago
on the sample above, when you view source of central frame,
you see contenido1.html although you are on contenido2.html.
You ever see the first page loaded.

Updated

17 years ago
Blocks: 59387
Marking as nsbeta1+/topembed+ per EDT triage.
Keywords: nsbeta1, topembed → nsbeta1+, topembed+
Whiteboard: [adt2] [bugscape 18199] [ETA Needed] → [adt2] [bugscape 18199]
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.4beta
There are couple of things going on here. looking thro'  the testcase,

1) Doing a location.replace right on the "src=" for the subframe, confuses
docshell and it does not create a entry for the subframe in history. 

2) The left frame links.html itself does not load because javascript fails to
identify a relative url over a blank document. Changing
location.replace('links.html') to a absolute url makes the left frame show the
document. But due to the problem described in 1) back forward navigation stil
won't work. 

Looking thro' how these 2 issues can be resolved....
Created attachment 113749 [details] [diff] [review]
Initial patch

This patch specifically looks for about: as the base uri. Other way to fix this
bug is to simply look for success of NS_NewURI() in SetHrefWithBase() and redo
it with an alternate base url if the first one fails.
Attachment #113749 - Flags: superreview?(darin)
Attachment #113749 - Flags: review?(jst)
Comment on attachment 113749 [details] [diff] [review]
Initial patch

This approach looks reasonable to me, but this diff is broken, looks like a
chunk of the diff is missing, the beginning of the file says nsDocShell.cpp,
but most of the changes are to LocationImpl. Please attach a non-broken diff.
Attachment #113749 - Attachment is obsolete: true
Attachment #113749 - Flags: superreview?(darin)
Attachment #113749 - Flags: superreview-
Attachment #113749 - Flags: review?(jst)
Attachment #113749 - Flags: review-
Attachment #113810 - Flags: superreview?(darin)
Attachment #113810 - Flags: review?(jst)
Comment on attachment 113810 [details] [diff] [review]
patch 1.1

+// Checks if the baseurl is an about: url. If so, we use the parent document's
url
+// as the base url. 
+nsresult
+LocationImpl::CheckBaseURL(nsIURI * aBaseURI, nsIURI ** aAlternateBaseURI)

How about renaming this to soemthing like FindUsableBaseURI()? You're not just
checking something here, you're looking for a usable base URI if one isn't
given... And rename aAlternateBaseURI to something like aUsableURI, and for
consistency's sake, use URI all over, no URL's here.

 {
+    if (!aBaseURI || !mDocShell)
+	 return NS_ERROR_FAILURE;
+    PRBool aboutProtocol = PR_FALSE;

This file uses 2-space indentation, stick with that.

+    // If the current base url is not a "about:" url, we have nothing to do. 
+    if (NS_SUCCEEDED(aBaseURI->SchemeIs("about", &aboutProtocol)) &&
!aboutProtocol)
+	 return NS_OK;

Set *aAlternateBaseURI to aBaseURI here.

>+    if (!docShellAsTreeItem)
>+        return NS_ERROR_FAILURE;
>+	nsCOMPtr<nsIDocShellTreeItem> parentDS;

No tabs!

+    docShellAsTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(parentDS));

I don't think we want to get the root directly here, imagine a nested frameset
case, the base should not necessarily be the root's URI, but in stead maybe the
immediate parent's URI. Write a loop that walks up the parent chain to the same
type parent and use the first base that's a usable base URI.

+    if (webNav)
+	 rv = webNav->GetCurrentURI(aAlternateBaseURI);

I say we want to check that aAlternateBaseURI here is actually something that
can be used as the base URI, if not, don't bother passing it back to the
caller...

+  nsCOMPtr<nsIURI> newUri, alternateBaseURI;
+
+  // Make sure the base url is something that will be useful. 
+  result = CheckBaseURL(aBase, getter_AddRefs(alternateBaseURI));
+  if (!alternateBaseURI)
+    alternateBaseURI = aBase;

How about naming alternateBaseURI simply baseURI here?

I'll stamp my r/sr on this if you make those changes, but I want to see the new
patch first...
Attachment #113810 - Flags: review?(jst) → review-

Updated

16 years ago
QA Contact: claudius → carosendahl

Updated

16 years ago
Attachment #113810 - Flags: superreview?(darin)
Created attachment 114238 [details] [diff] [review]
Patch 1.2
Attachment #97292 - Attachment is obsolete: true
Attachment #113810 - Attachment is obsolete: true
Attachment #114238 - Flags: review?(jst)
Comment on attachment 114238 [details] [diff] [review]
Patch 1.2

+// Walk up the docshell hierarchy and find a usable base URI. Basically 
+// anything other than a "about:"

I just realized that about: isn't the only type of URI that won't work as a
base, what about a javascript: URL, or a data: URL? Any others?

- In LocationImpl::FindUsableBaseURI():

+{
+    if (!aBaseURI || !aParent)
+      return NS_ERROR_FAILURE;
+    NS_ENSURE_ARG_POINTER(aUsableURI);
...

Pull all of this method in two spaces, don't mix 4-space and 2-space
indentation.

+    while(NS_SUCCEEDED(rv) && baseURI) {
+      PRBool aboutProtocol = PR_FALSE;
+      // If the current base uri is a valid one,(anything other than about:),
return it.
+      if (NS_SUCCEEDED(baseURI->SchemeIs("about", &aboutProtocol)) &&
!aboutProtocol) {
+	 *aUsableURI = baseURI;
+	 NS_ADDREF(*aUsableURI);
+	 return NS_OK;
+      }

Make this check for javascrip: and data: URL's too.

r=jst with that change.
Attachment #114238 - Flags: review?(jst) → review+
jst, I thought about other urls that could not be usable base urls. But I wasn't
sure I would have a complete list or I will be able to test them well. That's
why I restricted to about: for now, which I know doesn't work.  I believe that
data: and javascript: can fall into this category, but I'm not going to spend
more time on other protocols. I will take care of the spacing. (I'm so used to
the 4-space indentation in docshell). Thanks.
Attachment #114238 - Flags: superreview?(darin)

Comment 15

16 years ago
Comment on attachment 114238 [details] [diff] [review]
Patch 1.2


>+      // If the current base uri is a valid one,(anything other than about:), return it.
>+      if (NS_SUCCEEDED(baseURI->SchemeIs("about", &aboutProtocol)) && !aboutProtocol) {
>+        *aUsableURI = baseURI;
>+        NS_ADDREF(*aUsableURI);
>+        return NS_OK;
>+      }

yeah, would be good to include data: and javascript: here.  hmm.. could you
check nsIProtocolHandler::protocolFlags for the URI_NORELATIVE flag?  that
should tell you if the current baseURI can be used to make the href absolute,
right?	that would be much better than some sort of whitelist.
Created attachment 114738 [details] [diff] [review]
Patch v 1.3

Darin, I have attached patch that uses the protocol flags to check on validity
of a base uri instead of a white list.	One question for you is, is there a
default value for  protocol flags that can be assigned to the local variable. 
+    PRUint32 pFlags; // Is there a default value for the protocol flags?

It looks like 0 would not make a good a default  value	here.
Comment on attachment 114738 [details] [diff] [review]
Patch v 1.3

Darin, Please see my previous comment.	The latest patch is pertty much the
same as the previous one, except that it uses (as you suggested) the 
protocolhandler flags to identify possibility of relative uris in nsLocation
::FindUsable BaseURI().  Please provide your super review comments. Thanks.
Attachment #114738 - Flags: superreview?(darin)

Comment 18

16 years ago
Comment on attachment 114738 [details] [diff] [review]
Patch v 1.3

this looks right to me.  sr=darin

radha: the protocol flags almost always include either URI_STD or
URI_NORELATIVE.  0 is not a common value, but i don't think it's anything to
worry about :)
Attachment #114738 - Flags: superreview?(darin) → superreview+
Comment on attachment 114738 [details] [diff] [review]
Patch v 1.3

+  nsCOMPtr<nsIIOService> ioService(do_GetService(NS_IOSERVICE_CONTRACTID,
&rv));
+
+  while(NS_SUCCEEDED(rv) && baseURI && ioService) {

Loose the double error check here, if ioService is non-null, do_GetService()
*did* succeed.

...
+    protocolHandler->GetProtocolFlags(&pFlags);
+    if (!(pFlags & nsIProtocolHandler::URI_NORELATIVE)) {
+      *aUsableURI = baseURI;
+      NS_ADDREF(*aUsableURI);
+      return NS_OK;
+    }
+
+  // Get the same type parent docshell
+  nsCOMPtr<nsIDocShellTreeItem>
docShellAsTreeItem(do_QueryInterface(parentDS));
+  if (!docShellAsTreeItem)
+    return NS_ERROR_FAILURE;
+  nsCOMPtr<nsIDocShellTreeItem> parentDSTreeItem;
+  docShellAsTreeItem->GetSameTypeParent(getter_AddRefs(parentDSTreeItem));     
+  nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(parentDSTreeItem));
+
+  // Get the parent docshell's uri
+  if (webNav) {
+    rv = webNav->GetCurrentURI(getter_AddRefs(baseURI));
+    parentDS = do_QueryInterface(parentDSTreeItem);
+  }
+  else
+    return NS_ERROR_FAILURE;

Fix the indentation of the later part of this loop, indent it to line up with
the first part...

+  }  // while 

r/sr=jst if you change that.
Attachment #114738 - Flags: review+

Updated

16 years ago
Attachment #114238 - Flags: superreview?(darin)
Fix checked in. 
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

16 years ago
verified on today's windows trunk, works fine. thanks!
Status: RESOLVED → VERIFIED
(Reporter)

Comment 22

16 years ago
The networking issue (never stops loading the page) has been filed as
http://bugzilla.mozilla.org/show_bug.cgi?id=196259
*** Bug 178344 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Component: History: Session → Document Navigation
QA Contact: carosendahl → docshell
You need to log in before you can comment on or make changes to this bug.