191145 - If loading cancelled, or page is slow to load and you switch tabs, tab/window remains with empty url location

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[nicolas berloquin]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.0.1) Gecko/20030124 Chimera/0.6+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.0.1) Gecko/20030124 Chimera/0.6+

The biggest hassle with this bug is in tabbed browsing, see below.
Since the url is not shown in the location bar until a page has started
rendering, when one opens many tabs at a time, and a few tabs can't be loaded
(host unreachable, temporary pb etc), when you go to the tab, you only see an
empty location bar, and no info whatsoever about what was not loaded...
And if you were browsing a site (say slashdot), and tab-opening interesting
links that you meant to read later on, when you reach those, you have no way of
knowing what wasn't loaded.

Reproducible: Always

Steps to Reproduce:
1.find an url that doesn't work (you could fake a wrong domain name)
2.add this url in a tab group and open it from the bookmarks
3.you get an alert that tells you what went wrong, but who remembers those when
there could be 2 or three alerts, or complicated urls ???

Actual Results:  
the tab with the bad url is empty, and its url location bar also. No way to hit
enter to try again, for example

Expected Results:  
I believe netscape or exploder used to handle this case better, ie it would fill
the location bar as soon as the tab/window was opened. In case of pb, or lag,
one could hit stop then hit enter in the location bar to have it reloaded right away

I think I have repeated myself enough already ;-)

Comment 1

[Mike Pinkerton (not reading bugmail)]

this is pretty hard to fix because gecko forgets about the broken url and
there's no way for the front-end to tell if there was an error loading the page.

Comment 2

[bugzilla2005]

You can set browser.xul.error_pages.enabled to true, which displays an error
page instead of a popup window in case of a page load error, when there is no
server-side error page available (connection refused, dns error, etc).  The
error page even includes the original URL, altough in a pretty obscured format.

However, this is not even a workaround for this bug in certain cases.  Let's
suppose loading a page just takes a long time, and during this, the user will
have no information in the tab/window about what URL is being loaded.  If he
choses to stop loading and try again later, the URL is lost, and an
`about:blank' is displayed instead.

Mozilla 1.4 happens to display the URL in the location bar before starting to
load the page, so it shouldn't be that hard to implement in Firebird too, I
suppose.  (Note that this part has nothing to do with good or bad URL's, or they
being reported to the frontend; just the timing of filling in the location bar.)

Comment 3

[bugzilla2005]

Er, sorry.  I was about submitting a very similar bug for Firebird, the `search
before submit' page happened to pop up this bug, and I supposed it belongs to
Firebird.

Comment 4

[Michel Meyers]

This sounds like bug 103720 (opening page that doesn't work causes Mozilla &
Mozilla Firebird to forget the URI). The corresponding Firebird bug would be bug
203102.

Comment 5

[Jon Stevens]

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

Comment 6

[calia]

I thing of this one as very annoing and would be pleased to see it fixed. I hope
somebody finds his/her time to have a look at this one and teach mozilla to
display the url in the location bar the whole time not only after the loading of
the page has been finished.

This is the same for all mozilla products including the mozilla browser itself,
not only firefox or camino.

Mike, is there really no way to fix this?

Comment 7

[calia]

>[set browser.xul.error_pages.enabled to true]
This is a very good hint I didn't know about - it helps until the guys/girls
here at mozilla.org fix it. Thx.

Comment 8

[lsof]

I find it a bit worrying that Firefox is almost at it's "final" release and this
is still broken. It makes tabbed browsing look unfinished. Maybe it is!

The alternative of setting browser.xul.error_pages.enabled to true isn't a fix:
 1. It's not the default
 2. It changes the contents of the Location bar. True - the error message is
better, but it changed the bug from "empty location bar" to "location bar full
of a long amount of ugly text".

Comment 9

[Stuart Morgan]

This is a Camino bug, not a Firefox bug.

Comment 10

[brian]

This IS a firefox bug.

This is NOT ONLY a Macintosh bug. I can reproduce it on Windows.

This bug is missing the 'dataloss' keyword present in its previous pre-firebird
incarnation bug 103720.

This is NOT a 'minor' bug. It should have severity 'normal'. (bug 103720 went
through exactly the same deliberations--see comment 32, 33).

This bug is a dataloss bug: a typical use case for using tabs is to open a
number of tabs in the background (e.g. from a news headline page), then switch
to the loaded tabs to view their contents. By the time the user becomes aware
that a tab failed to load, they no longer know which location that particular
tab was loading -- very frustrating!

I haven't changed any of the fields (Hardware/OS/severity/keywords). If no one
disagrees with my assessment of these fields -- can I?

Finally, is the patch from bug 103720 relevant?

Comment 11

[Jasper]

Bug 103720 seems to be indeed the exact issue Camino has aswell. Mike, Josh
could one of you take a closet look.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is fixed by bug 292646, or will be when Mike is sufficiently pleased and
bug 292646 is marked FIXED.

Comment 13

[Samuel Sidler (old account; do not CC)]

This bug is only partially fixed by bug 292646. As it stands in the current
nightlies, if you visit a failed domain, the URL remains in the location bar.
However, if you cancel loading the page, the URL disappears. So: cancel page, no
URL; error page, yes URL. :)

Comment 14

[Mike Pinkerton (not reading bugmail)]

i have a patch for this (for camino), upcoming.

Comment 15

[Mike Pinkerton (not reading bugmail)]

Attachment: remember the url we load in case of failure

remember the uri we start loading because gecko doesn't store it anywhere until
the onLocationChanged status change. Return our uri if asked and it's not
about:blank (which is what gecko will return before the first location change).


smfr, can i get a quick sr? i just want to make sure i'm not doing anything
stoopid.

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 191135 [details] [diff] [review]
remember the url we load in case of failure

>Index: src/embedding/CHBrowserView.h

>+// returns a copy of the currently loaded url, or the last URI passed to
>+// |-loadURI...| if it hasn't yet loaded (or failed to load). Clients may
>+// modify this string.
> - (NSString*)getCurrentURI;

The "clients may modify" confuses me. Is this an NSMutableString? If so, it
should be returned as one, but then what does modifying it imply?

>@@ -225,4 +230,7 @@
> - (void)doAfterPromptDismissal;
> 
>+// returns a copy of the currently loaded url, or the last URI passed to
>+// |-loadURI...| if it hasn't yet loaded (or failed to load). Clients may
>+// modify this string.
> -(NSString*)getCurrentURLSpec;

Ditto.

>Index: src/embedding/CHBrowserView.mm

>@@ -261,4 +263,5 @@
>       NS_IF_RELEASE(_webBrowser);
>       NS_IF_RELEASE(_listener);
>+      [mPendingURI release];

Set it to nil (since we're not in -dealloc yet)

>+//
>+// -getCurrentURI
>+//
>+// returns a copy of the currently loaded url, or the last URI passed to
>+// |-loadURI...| if it hasn't yet loaded (or failed to load). Clients may
>+// modify this string.
>+// XXX how does this differ from getCurrentURI?

We should figure this out.


>+// -getCurrentURLSpec
>+//
>+// returns a copy of the currently loaded url, or the last URI passed to
>+// |-loadURI...| if it hasn't yet loaded (or failed to load). Clients may
>+// modify this string.
>+// XXX how does this differ from getCurrentURI?

Ditto.

Maybe we should release and nil out mPendingURI when we get the
onLocationChange for the load? Then it's only non-nil between loadURI and the
onLocationChange, so you can just return it, if non-nil, from the
"getCurrentLocation" getters.

BTW, more context in the diff would be nice :)

Comment 17

[Mike Pinkerton (not reading bugmail)]

duh, i didn't mean that the string was mutable. i was thinking about other
things and got sidetracked. i'll update that. 

as far as the two different methods, i agree we should figure that out but i
didn't want to do that as part of this bug -- too easy to screw up. It appears
we should be able to get rid of one, but that's beyond the scope of this issue. 

Also, the CHBrowserView doesn't appear to get the listener callbacks from gecko,
i'll have to dig around more there. i took a look to see where to hook in for
that when i was writing it, but didn't see anything.

Comment 18

[Mike Pinkerton (not reading bugmail)]

yeah, we create a CHBrowserListener specifically to get those state changes,
this view never gets them. I don't think we can do much about that. new patch
upcoming.

Comment 19

[Mike Pinkerton (not reading bugmail)]

the right way to do this is to move the code to the BrowserWrapper, but there's
way too much code that goes straight to the CHBrowserView and bypasses (or
doesn't know anything about) the wrapper so it just doesn't work.

i've attached what i have, but it's incomplete for things like opening a new tab
(at the very least).

Comment 20

[Mike Pinkerton (not reading bugmail)]

Attachment: remember at the browserWrapper layer, not 100%

patch that still needs fixing for tabs which know nothing about the browser
wrapper.

Comment 21

[Mike Pinkerton (not reading bugmail)]

now that we have error pages, if the page actually _fails_ the url is preserved.
We now only get into this situation when you _cancel_ a load, or switch to a tab
before it's failed or loaded.

neither approach we have is very good and requires a lot of complexity. pushing
off again. Sorry.

Comment 22

[Jasper]

Bump, it's a minor thing I know but it bugs the hell out of me. Please guys
let's not forget this one.

Comment 23

[Simon Fraser [no longer active]]

I think Mike's latest patch is pretty close. I'm not sure what "patch that still
needs fixing for tabs which know nothing about the browser wrapper." means.

Comment 24

[Mike Pinkerton (not reading bugmail)]

(In reply to comment #23)
> I think Mike's latest patch is pretty close. I'm not sure what "patch that
> still
> needs fixing for tabs which know nothing about the browser wrapper." means.
> 

see comment 19. a lot of our FE code bypasses the browser wrapper.

Comment 25

[Simon Fraser [no longer active]]

(In reply to comment #24)
> (In reply to comment #23)
> > I think Mike's latest patch is pretty close. I'm not sure what "patch that
> > still
> > needs fixing for tabs which know nothing about the browser wrapper." means.
> > 
> 
> see comment 19. a lot of our FE code bypasses the browser wrapper.

That should be fixable. I'll try to code up a patch.

Comment 26

[Stuart Morgan]

Taking. I did some of this for session restore, and I want to rip out as much of the pass-through behavior of comment 19 as possible anyway.

Comment 27

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 28

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I still see what I think is part of this bug fairly often: load a bunch of tabs that for whatever reason are very slow to load, or which timeout poorly (and never generate an error page when timing out, or never time out!?)

Once you switch away from a "loading" tab, we drop the URL that we "blasted in" when making the tab (at least until the page actually starts to load, or an error page loads).  The end result is that I have a bunch of (effectively about:blank) tabs that have no URL in the location bar, so I have no idea what sites I didn't see in order to try them again later when the sites or the internet are sucking less.

(In reply to comment #19)
> i've attached what i have, but it's incomplete for things like opening a new tab
> (at the very least).

Alas, it sounds like pink's old patch wouldn't have fixed what I experience :(

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: remember at the browserWrapper lauyer (currentURI part, kinda-updated), not 100%

Here's the salient remaining part of pink's patch, unbitrotted.  (As Stuart noted in comment 26, he did most of this during session saving work, e.g. bug 361092.)

This patch does allow you to cycle through loading/connecting tabs and have a URL, which is the part of this bug that annoys me the most.

Some problems:

1) Reload is enabled, but doesn't work if we haven't yet made a connection (e.g., on example.com:8080; but when opening a bunch of bugs at once, reload seemed to work, because bmo does actually respond--but this may be a race condition).  Instead, it blanks the location bar and doesn't reload anything.

Porting pink's changes to reload (and retaining the pending referrer and pending load flags, maybe, like pink's patch did) might fix that.  We'd have to decide if we want reload enabled that early (I vaguely remember some old bugs with froodian discussing this).

2) Stop is also enabled, and clicking it stops the load attempt (as expected), but blanks the location bar.  This is one of the original parts of the bug :-(

If we can reset the location bar to the pending URL in the Gecko callback we get for cancelling, we might to be able to fix this.  We do preserve the URL of a cancelled-during-pageload cancellation, but we've also gotten the start of the "page" by then, so dunno.

3) I noticed the view source button was enabled now during the pre-connection portion of the loading process; we don't want that.  I'm sure other validation assumptions are broken by this change, too, and who knows what else.

We can probably fix this by auditing our validation code--or, alternatively, rather than changing currentURI like pink did, we could add a new method (pendingOrCurrentURI?) for just the location bar, stop, reload, and maybe bug 569175 to call.

This has been really lightly tested, and I haven't audited the callers, and it's really late now, but I do think these bits of pink's old patch (combined with what Stuart already implemented) do provide a way forward and can fix at least part of this bug, if not all of it.

Comment 30

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I'm going to try to get back to this for 2.1.2.