73141 - Clicking link in mail window brings up browser on home page

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P1)

Description

[David :Bienvenu]

If you start up mail w/o the browser, and click on a bugzilla link in a mail
message, the browser window opens and goes to the home page, not the bugzilla
page. Subsequent clicks on the bugzilla link does the right thing. CC'ing Alec
because he made some recent changes to the browser's uricontent listener code,
according to mscott.

Comment 1

[David :Bienvenu]

actually, it starts opening the bugzilla page; then replaces it with the home
page. Reassigning to Alec per mscott's wish.

Comment 2

[Alec Flett]

I think this was jag's delayedInit stuff, which he just backed out.. 

Comment 3

[Eric Hedström]

looks like you get the same thing clicking on a URL in a mail message, or the
"Go to Bugzilla" or "Look at Bonsai" links on the bottom of the window (sorry, i
don't know all the terminology), after starting mozilla with "mozilla -mail"

Also for me it brings up a blank window instead of my home page, because that is
how my browser is configured to start up. In case that is useful information.

Comment 4

[Alec Flett]

it seems that this SPECIFICALLY happens for mozilla -mail, not for just running
mozilla, launching mail, and closing all browser windows. This may be a command
line handling thing, not sure. Jag, any thoughts?

Comment 5

[Peter "jag" Annema]

Yeah... It's because the browser doesn't know better than that it (the browser,
not mozilla as a whole) wasn't run yet and will thus try to do the "first time
browser starts up" thing. The solution is simple, just move the arguments[0]
check before the cmdLineURLUsed check (I've got it in a bug somewhere, let me
dig it up), but it will still do the "first time per session" thing when you
open a browser window with Accel-N. A better solution would be to move the first
time per session "url to load" magic into AppRunner, and just pass the url to
load in as arguments[0] (thus automatically getting the once per session).

Comment 6

[Scott MacGregor]

hey alec, i never launch from mozilla -mail
I always bring up the browser first then launch mail.

I just happen to close the browser window later on. So no browser windows are
open when I see this. 

Comment 7

[Alec Flett]

so, I am still not seeing this, I've tried with and without -mail, and I have a
homepage. I have noticed that the url of my homepage appears in the urlbar just
before the bug appears... does it start loading for some people?

Comment 8

[David :Bienvenu]

yes, it starts loading the home page, and then the second url comes along and
interrupts the first, it seems.

Comment 9

[Eric Hedström]

Using 2001041104/win2k, I no longer see this bug by running "mozilla -mail" and
clicking on a linkified bugzilla URL in a mail message.

However, I do still see it by running "mozilla -mail" and clicking "Go to
Bugzilla" or "Look at Bonsai" on the pop-up Mozilla menu on the Taskbar.

Still have Navigator set to launch to a blank page, so by "see the bug" I mean
that the Navigator window opens but stays blank instead of going to Bugzilla or
Bonsai.

Comment 10

[Alec Flett]

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

Comment 11

[Alec Flett]

might be a dupe of bug 69521, or at least the patch there might fix this problem

Comment 12

[Alec Flett]

unfortunately, I did not even have time to look at this for moz 0.9. Moving to
0.9.1 and maintaining priority.. sorry!

Comment 13

[Paul Chen]

nav triage team:

Marking nsbeta1+

Comment 14

[Gilles Durys]

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

Comment 15

[Gilles Durys]

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

Comment 16

[Luke Morey]

You also get the home page / blank page / last visited (as per preferences) the 
first time you choose "View Source" on an email message.

Comment 17

[Peter Trudelle]

Doesn't necessarily just interrupt; it can prevent the load of the clicked
link.  If your home page loads quickly (e.g., Google), it can finish loading and
displaying before the loading the link you clicked. What I'm currently seeing on
Win98 is that Google loads and displays, then the URL for the link I clicked on
flashes for maybe 1/5 second in location bar, then Google remains as URL and
page.  Perhaps JS on the page that sets the focus prevents the correct link from
loading at all? Also, this isn't just bugzilla, clarifying summary.

Comment 18

[Scott MacGregor]

this is really eating my lunch in mail. I run into this every day =(. If you
can't reproduce it that way, bring up an aim window in the commercial tree.
Click on an http url in an an aim window (make sure you don't have a browser
window up). Happens every time then too. Just like in mail. 

Comment 19

[Scott MacGregor]

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

Comment 20

[Alec Flett]

so I finally spent some time investigating this. the problem is basically that
the code checks if .cmdLineURLUsed has been set, even if there was no url given
on the command line. the fix should be pretty easy, but might end up
reorganizing some code.

Comment 21

[Viswanath Ramachandran]

I think we need to bring this back onto the mozilla0.9.1 list. Alec, can you 
look at it. thanks!

Comment 22

[Gilles Doyen]

I'm using Linux, Mozilla build 2001053108.
When I have just upgraded to the most recent build, I stumble upon a problem
which seems a lot like bug 73141 (or 75689).

Here's what happens:

With only the mail open,
1. Send yourself a message with hyperlinks (http://www.oeone.com)
2. Open that email and click on the hyperlink
3. It opens a Browser window, but it doesn't load the link (it remains blank,
but the link we clicked on shows in the URl field)

In case it's important, the default settings for the browser is to open on
www.mozilla.org.

Is my problem related to this one? Thanks for any help!

Comment 23

[Alec Flett]

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

Comment 24

[Alec Flett]

ok, I have found this to be more chaotic than I first thought. It seems that
when a link gets clicked on from mail, two things happen:
- a new browser window is opened
- the uri loader loads a url in that window

the problem is that these two actions happen asynchronously (don't even ask me
how we make sure the window has finished opening, in order to kick off the load)

after poking at this for a few hours, I've decided that ANY fix is going to be
very risky - there are just too many states that a new window can open in, and
its not worth risking those other cases to fix this one with a hacky fix.

I'm continuing to work on this, will hopefully have a patch for m0.9.2 soon

Comment 25

[Scott MacGregor]

do we know what broke that caused this regression? maybe if we could figure that
out, a less risky fix would present itself?

Comment 26

[Alec Flett]

we basically re-wrote the code which decides which url to load to account for
all the different cases (opening from the command line, opening via dde, opening
via uri loader, opening from a menu, etc, plus taking into account the users
preferences w.r.t. blank/homepage/last page visited) and this is the one edge
case. my concern is that if we fix this case, we will break another one. This
will be fixed for rtm. 

My take on this is (sorry mail guys) that most people keep a browser window open
- i.e. you're not likely to be using mail if you're not using the browser as
well... so in the case of the average user, they will likely already have a
browser window open, and thus will not be plagued by this bug.

Comment 27

[Eric Hedström]

how web-centric of you. :)  I use e-mail and keep it open all day, while browser
windows come and go as I need them only once in a while.

Comment 28

[Alec Flett]

well, you're not the "most people" that I referred to above.

look, I'm going to fix it, I'm just providing the reasons for pushing off to 0.9.2

Comment 29

[Peter Trudelle]

I'm not sure Alec's supposition is true of mail users in general, and certainly
not of me :-) (although we are admittedly a small minority in any case). When I
launch the product, I almost always bring up mail first, since my new messages
are much more interesting to me than any web page.  I don't bring up a browser
window until I have a need for one, and generally that reason is to view links
in my mail, so this bites me a lot. That said, I agree with the current target;
this should not be a showstopper for beta.

Comment 30

[jag (Peter Annema)]

If we really want this bug gone for 0.9.1, the simplest way to do that is to 
create a browser instance object in the mail/news startup code and set 
cmdLineURLUsed to true. Add similar code to composer if needed. This (ugly) 
hack would be 3 lines of code (create object, set boolean, explicitely destroy 
object).

The right way to fix this though is to make a browser window open to about:blank 
unless it's passed an URI to load through window.arguments[0]. This however 
requires a whole bunch of changes and is definitely post-0.9.1 material.

Comment 31

[Seak, Teng-Fong]

Well, Peter Trudelle, you can add my name to your "small minority" which is more
interested in Mailer than Browser.  For the time being, Mozilla Mail is the only
mail client which can compete with Outlook Express, with its support of many
different languages as well as Unicode and most of all, Mozilla Mail is VB
Virus-free!

Comment 32

[timeless]

i haven't switched yet, but i'm like trudelle i use mail for quite a while 
before i start browsing.

Comment 33

[Gilles Doyen]

Add my name to the lsit of minority: in a work environment, emails are used
all-day, whereas web browsing is totally secondary. I don't consider Alec's
argument valid, although some other reasons (for example complexity of the fix
or the risk associated with it) may very well warrant the need to postpone to
0.9.2   Just my 2 cents worth...

Comment 34

[Alec Flett]

please stop polluting this bug with "me too!" 
for the 900th time, I'm going to fix this freakin' bug.

Comment 35

[Alec Flett]

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

Comment 36

[Alec Flett]

ok, an update: I have a fix for this bug, but I leave for Boston in about an
hour. I will attach a fix the evening of June 18th (monday)

The quick 'n dirty solution, for those listening, is at the same time that we
check appCore.cmdLineURLUsed, we also check cmdLineService.URLToLoad to see if
there is any url passed on the command line

Comment 37

[Alec Flett]

a further update - my fix didn't work as I thought, but I finally figured out
what's going on. When you click a link from mail that opens a new window, we're
passing in the URL via the nsBrowserContentHandler, but it's being ignored in
favor of the homepage.. 

again, I know how to fix this but I ran out of time today due to the barrage of
end-of-milestone super-review requests.. delaying this further...eta 6/20

Comment 38

[Alec Flett]

Attachment: the ultimate fix

Comment 39

[Alec Flett]

ok, there were so many problems here, it's not even funny:
In nsBrowserContentHandler::HandleContent, we were calling wwatch->OpenWindow
with the spec of the url, and no arguments. When there are no arguments, then
the dialog is opened as if it is called from the standard window.open(), which
creates a window that has no element "window.arguments", which meant the newly
created window never saw the spec. This basically threw spec into a black hole
never to be seen again. 

The result was that a browser window would be open, but the window would have no
clue as to what url it was supposed to be loading. 

So, it would do its best guess and try to load the browser's homepage under some
circumstances, and by some wierd fluke, if there was no URL on the command line,
all subsequent windows after the first one would magically manage to load the
url that the user clicked on in mail.. but this is TOTALLY random and lucky that
that ever worked.

So, we needed to be able to create a navigator window with chrome, that
specified the URI that was clicked on in window.arguments[0].. but we don't want
the browser to try loading that uri, because the docloader will actually load
the uri moments later... and in fact, the load of that URI has already begun, so
it would be wrong of the browser to try to load it anyway!

So, we use another argument, window.arguments[2] (window.arguments[1] has other
uses) to pass a special string "dontload" which means that we should put the url
in the title bar, but not actually load it... 

The patch attached does all of these things... please ignore the CMDLINEURL*
macro changes, I won't check those in - they are a part of another bug.

ideally, I'd get an r= from jag, and and sr= from mscott, but I welcome other
reviewers as well :)

Comment 40

[Alec Flett]

actually, I think one thing I just forgot is that I think we're supposed to be
getting the browser chrome url from prefs.. investigating that now, but I'd
appreciate at least one reviewer on this stuff anyway.

Comment 41

[Alec Flett]

ok, technically I am supposed to load this from prefs ("browser.chromeURL" for
my own reference), but there are about a million instances that don't...:) but
I'll fix this one instance anyway..

Comment 42

[jag (Peter Annema)]

I have a few questions about this patch. Let's discuss this tomorrow.

Comment 43

[Viswanath Ramachandran]

Alec/Jag - how is this one going? shd we be still shooting for m0.9.2? I think
we should,  as it is critical to get some good testing coverage on this patch.
thanks, Vishy

Comment 44

[Alec Flett]

Jag and I discussed 2 possible modifications to the above patch.. I tried the
first one, but suddenly my build has started acting wierd... still hope to have
chosen one of the two by the end of today

Comment 45

[Asa Dotzler [:asa]]

pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)

Comment 46

[Alec Flett]

Attachment: continue to repair faulty code

Comment 47

[Alec Flett]

ok, I finally got tired of trying to make my other patch work and went back to
trying to repair the continually broken logic.. and somehow, I found the correct
combination and ordering of if() statements this time (I don't know why it was
so hard the first few times I tried)

The other patch is on the way to the correct solution, but this patch at least
makes the bug in question go away.

Basically what I'm doing is making sure that there actually is a url on the
command line to load, before we go trying to make use of the "cmdLineURLUsed"
attribute... and I copied the !turboMode logic into the 2nd if() so logically
turboMode will do the same thing as before.. 

looking for r=, sr= and then I'll land this on the trunk. jag, mscott?

Comment 48

[Scott MacGregor]

looks good to me alec. r/sr=mscott

Comment 49

[timeless]

should !appCore.cmdLineURLUsed 
be !(cmdLineURLUsed in appCore && appCore.cmdLineURLUsed) ?

Comment 50

[jag (Peter Annema)]

timeless: no. Appcore is known to have that attribute.

alecf:
+    var cmdLineService =
Components.classes["@mozilla.org/appshell/commandLineService;1"]
+      .getService(Components.interfaces.nsICmdLineService);

I think the code is more readable when the dots match up...

+    var cmdLineUrl = cmdLineService.URLToLoad;
+    if (cmdLineUrl && !appCore.cmdLineURLUsed  && !turboMode) {
+      uriToLoad = cmdLineUrl;
       appCore.cmdLineURLUsed = true;
+    }
+    
+    if (!uriToLoad && !turboMode) {
+      var cmdLineHandler =
Components.classes["@mozilla.org/commandlinehandler/general-startup;1?type=browser"]
+        .getService(Components.interfaces.nsICmdLineHandler);
+      uriToLoad = cmdLineHandler.defaultArgs;
     }



I think the logic becomes more clear when you move the !turboMode out of the two
ifs into one if like this:

var uriToLoad;
if (!turboMode) {
  var cmdLineService =
Components.classes["@mozilla.org/appshell/commandLineService;1"]
                                
.getService(Components.interfaces.nsICmdLineService);
  var cmdLineUrl = cmdLineService.URLToLoad;
  if (cmdLineUrl && !appCore.cmdLineURLUsed) {
    uriToLoad = cmdLineUrl;
    appCore.cmdLineURLUsed = true;
  }
  
  if (!uriToLoad) {
    var cmdLineHandler =
Components.classes["@mozilla.org/commandlinehandler/general-startup;1?type=browser"]
                                  
.getService(Components.interfaces.nsICmdLineHandler);
    uriToLoad = cmdLineHandler.defaultArgs;
  }
}

This would (theoretically) still not fix it if someone went and did something
like ./mozilla --mail http://myhomepage.com/

I wonder though if that would be considered a cmdLineUrl.

Assuming that you've tested browser startup; mail startup -> browser; mozilla -f
bloaturls.txt; r=jag

Comment 51

[Scott MacGregor]

alec just checked this into the trunk. Adding the vtrunk keyword so we can get
this QAed tomorrow and hopefully we can get on the nsBranch+ list. 

Comment 52

[R.K.Aa.]

"open link in new window" just broke on Linux.
Can't open via context-menu - can't open with middle-mouse click.
I do get a new window, but with about:blank, not desired url.

Comment 53

[NilsE]

Bug 89345 filed for regression.

Comment 54

[Alec Flett]

argh.. I need to move the window.arguments check above all the code that I just
checked in... I tested all the other options, but not anything involving
window.arguments. patch forthcoming in bug 89345

Comment 55

[Alec Flett]

Attachment: combined fix that went into trunk

Comment 56

[Alec Flett]

ok, I've attached a better patch which also fixes bug 89345, which was the
regression caused by the original patch.

Assuming the 2001-07-06 builds look good, then I'd recommend this patch goes
into the netscape branch.

Comment 57

[Viswanath Ramachandran]

How does this look on the trunk with the latest patch? 

Comment 58

[Eric Hedström]

much better! open in new window and viewing the source of a mail message work
again, too.

Comment 59

[Viswanath Ramachandran]

-> vishy

Comment 60

[Viswanath Ramachandran]

--> morse. Steve - could you take the last patch which Alec checked into the 
trunk and keep it ready to check in on a branch tree. Once we get more testing 
data, we'll see if we have an opportunity to merge this to the branch. thanks. 

Comment 61

[esther]

I've testeded this using trunk builds 2001-07-06 on linux and mac and 2001-07-09
on windows using the -mail to start.  (for Mac I drop a bbedit page with ARGS:
-mail) on the Netscape icon).  The scenarios mentioned in this bug and others
regarding blank pages for message source, links not opening up on first try all
seem to work now.  I even clicked on a link in an IM from the Sidebar with only
Mail up.  I then launched with Browser, selecting Mail app and closing all
Browser windows.   This scenario works too.  One thing I noticed on Linux, we
get an enabled Back button even though there is no Back, earlier today it went
into a unending loop, but now with the same build the Back goes to the Netscape
Home page.  Not sure what was wrong when the loop was going on, maybe the Home
page was broken.  In any case, that is different from Windows and Mac.  I don't
think this should stop the bug.  Still testing...will update if I find more.  

  

Comment 62

[Viswanath Ramachandran]

-> morse

Comment 63

[esther]

OK, I finished testing the -mail scenarios.  I downloaded trunk 2001-07-09 on
linux and mac and tested again.  The probem with the new bug (89345) caused by
this one is gone too using -mail for launching.
The outstanding issue mentioned above with linux is still there (Back button
enabled).  However, it only happens when you click on the link, not when you use
the context menu to open the link in a new window.  Again, this shouldn't hold
up this bug unless someone see's an underlying problem with this behavior.  
Verified on trunk, trunkverified

Comment 64

[Scott MacGregor]

Is there a reason why we haven't gotten this PDT'plussed to check into the
branch yet? Looks like the testing happened back on 7/9. We are running out of
time. Can we make sure this comes up at today's meeting so it can get checked
in? morse/Vishy, can you guys bring this up at the meeting at 4pm today? Thanks.

Comment 65

[Stephen P. Morse]

I already made a request for pdt approval and am awaiting a reply

Comment 66

[Scott MacGregor]

apparently PDT is very worried about possible side effects of this bug. I spent 
some time looking at this code and I believe we've tested the potential code 
paths that come down this route. We are as confident as we can be about the 
fix. 

Comment 67

[Viswanath Ramachandran]

as per PDT discussion today, we are not going to take this on the branch as it 
is not a stopper per se and there is a workaround (click the link again). 
marking fixed as this fix is on the trunk. 

Comment 68

[Paul Chen]

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

Comment 69

[scottputterman]

adding PDT+ per PDT.  This can go on the branch now.

Comment 70

[Blake Ross]

Reopening, in that case.

Comment 71

[Stephen P. Morse]

Fix checked in on branch.

Comment 72

[lchiang]

doron - I'm going to change the QA contact to Esther for verification on the
branch. (She's already verified on the trunk.)

If you want to take this bug back for your verifications, pls feel free to do
so. Thanks.

Comment 73

[scalkins]

I still see this on branch builds win32 2001-07-15-08Branch..only the page which
pops up is blank, not the home page.
(The bug 90393 was duped for this one which covers the test case where the
browser windpws are closed and clciking a link in mail invokes a blank web page
with the correct link url in the search bar.)

Comment 74

[esther]

Using build 2001-07-15 branch on windows and 2001-07-16 on mac and linux,
Stephen and I don't see this problem.  Not sure why Suzanne is.  We tested the
page source, open link in new window and clicking on a link in a mail message
after launching using -mail.  I will check with Suzanne to see how she tested it.

Comment 75

[esther]

The bug Suzanne logged is still happening.  This bug was specific to launching
with -mail and then the first time use of a browser page for the source or a
link in a mail message brought up a blank page or a Netscape home page there
after it gave the correct content.  Those (3) scenarios are working OK now with
this build.  Bug 90393 is different and is still broken.  Suzanne will reopen
that one, this one will be verified.

Comment 76

[Christopher Aillon (sabbatical, not receiving bugmail)]

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