65993 - window.open("") opens home page

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[jbrunette]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    2001011408

Starting with build 2001011408, when window.open is called with a blank url, 
the new window loads the configured home page for the browser.

Reproducible: Always
Steps to Reproduce:
1. Enter "javascript:window.open("") into url bar of Mozilla and submit


Actual Results:  New window opens with browser's configured home page.

Expected Results:  New window should be empty.

Only tested on Windows 2000, but probably others as well.

Any window.open call with a empty url arg is affected.

Caused by fix for 64526?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Over to DOM level 1 for triage.  Seeing this behavior on Linux build
2001-01-18-08, so OS -> All

Comment 2

[Keyser Sose]

Marking NEW.

Comment 3

[Jesse Ruderman]

Bug 65714 might be related (home page displays before or after the page that 
wnidow.open is trying to display).

Comment 4

[HJ]

This might indeed be related to the resent changes in navigator.js Build before
that change do not have this issue.

Comment 5

[Peter "jag" Annema]

A fix would be easy enough... But I consider this a feature rather than a bug.
If you want about:blank to open, write window.open("about:blank"). However, if
this is one of those major "can't change this 'coz of backward compatibility"
thingies, let me know and I'll fix it.

Comment 6

[Matthew T (active 1999-2002)]

If a developer opens a new window and doesn't immediately say what should go in 
it, expected behavior would be for nothing to go in it. They shouldn't have to 
specify `about:blank', which is browser-specific.

Comment 7

[Peter "jag" Annema]

D'oh. Okay, I see the problem, and what's causing the related bug. Fix coming.

Comment 8

[Peter "jag" Annema]

I said, a bit too eagerly.

I've changed the model from "loading an URI in a navigator window all over the
code" to "the navigator window loads an URI based on some simple priority
rules". Part of this has been shifting code from other places into navigator.js
and then provide hints from those other places what navigator should do. This
puts all knowledge about the URI loading in one place, however it seems I
overlooked a few places in which this "hint passing" needs to be done. One of
them is nsGlobalWindow::OpenInternal.

jst: navigator expects any URI to load (instead of say the homepage or what's on
the commandline) to be passed in as the first argument (window.arguments[0]).
What would the impact be of ::OpenInternal doing that instead of calling
nsDocShell::LoadURI itself?

Comment 9

[Peter "jag" Annema]

Btw, an empty URI string ("") should be passed in on arguments[0] to show a
blank page. Not passing in anything will cause the home page (or blank page or
last page visited) to be loaded.

Comment 10

[Peter "jag" Annema]

Attachment: [patch] Fix till we think of something better

Comment 11

[Peter "jag" Annema]

This makes the homepage only be loaded after the browser is started. Opening a
new page manually already has logic to load the homepage in the new window (we
were actually planning on removing that, never got around to it, makes it easier
for me now), and opening a page through window.open() will now start with a
blank page (about:blank, implementation detail) again.

Comment 12

[Blake Ross]

r=blake

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

a=[e-mail]

Comment 14

[Becki Sanford]

The attached patch causes http://bugzilla.mozilla.org/show_bug.cgi?id=69521 to
occur.

Comment 15

[Prashant Desale]

QA contact Update

Comment 16

[Peter "jag" Annema]

I checked this in a while back (as Becki's comment confirms).

Marking fixed.

Comment 17

[Prashant Desale]

Updating QA contact to Shivakiran Tummala.

Comment 18

[Sivakiran Tummala]

verified on build 2001-06-21-11-0.9.1 windows 2000. marking as verified

Comment 19

[jag (Peter Annema)]

Reopening, this regressed a while back, probably with alecf's recent checkin.

Comment 20

[Sivakiran Tummala]

yes jag looks like it is broken again...

Comment 21

[jag (Peter Annema)]

Attachment: [patch] Clean up navigator.js, fix nsAppRunner.cpp

Comment 22

[jag (Peter Annema)]

So, the change to navigator.js will fix a whole bunch of bugs with the home page
opening before/while/after loading the url we really wanted to load. Amazingly,
most of the support is already (or still) in place.

The change to nsAppRunner.cpp will make sure we still load a url passed in on
the command line, e.g.: mozilla http://slashdot.org/

I've tried adding support for -height and -width since the old code seemed to do
that, but couldn't test it on my linux box. Both the old and new code seem to
completely ignore the specified height and width.

Comment 23

[(not reading, please use [e-mail] instead)]

seems fine to me.

jag, is there any other places we need similar fixes?  there seem to be a bunch
of places where we try to open a browser window.

like:

http://lxr.mozilla.org/seamonkey/source/xpfe/bootstrap/nsNativeAppSupportWin.cpp#1615

http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/tasksOverlay.js#106


racham, notice jag is removing a call to "cmdLineHandler.defaultArgs;" from
navigator.js


perhaps that was the second call to GetDefaultArgs() that morse was trying to
work around in that bugscape bug.   (and his work around was causing you
activation problems.)

racham, can you try out jag's patch and see if it fixes your activation
problems?

Comment 24

[jag (Peter Annema)]

http://lxr.mozilla.org/seamonkey/source/xpfe/bootstrap/nsNativeAppSupportWin.cpp#1615

This one is similar to the one in nsAppRunner, which was already doing the right
thing (passing in the url to load through window.arguments[0]).

http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/tasksOverlay.js#106

The one in tasksOverlay already correctly passes in the url to load through
window.arguments[0], which is exactly what we want.

I knew about the tasksOverlay one, thanks for pointing out the
nsNativeAppSupportWin one, I didn't know about it.

Comment 25

[(not reading, please use [e-mail] instead)]

ok, so r=sspitzer

I'd really like to have racham try this and see if it fixes his problems.

1) I'm pretty sure nsNativeAppSupportWin is for DDE stuff, which is windows 
only.

2) about -height and -width on linux, I vaguely remember some specific code 
that is turned off on linux so that the window manager on linux will handle / 
remember that stuff.  (pavlov or blizzard would know for sure.)

Comment 26

[jag (Peter Annema)]

So I can't find racham, but played with activation on his machine after applying
the navigator.js part of my patch. Apparently activation takes us through a
different path, where apparently they don't get the DefaultArgs to hand to the
browser window when opening it. With my patch, that means the user won't see the
Welcome to 6.1 screen after activation. I'll try and fix that. Does anyone on
this bug know enough about the activation code to help me out here real quick?

Comment 27

[jag (Peter Annema)]

Attachment: [patch] This also grabs the -P jag and --profilemanager (etc.) cases

Comment 28

[jag (Peter Annema)]

So I forgot that profile manager goes through the same path to which I added the
command line url support. Now, if there's no command line url, it will get the
default url to load (GetDefaultArgs) like we used to in navigator.js.

I suspect this will also fix the activation path, will check that with racham.

Comment 29

[jag (Peter Annema)]

Attachment: [patch-really-now] This also grabs the -P jag and --profilemanager (etc.) cases

Comment 30

[jag (Peter Annema)]

Racham and I tested this patch for the activation bugs they were seeing, it
seems to fix their problem, but this was with Racham's work-around patch still
in the tree, so he'll test it without that.

In the mean time, when we tried to load a page from Real Player in the case
where Netscape isn't running, it came up blank. With a window open, the right
thing happened. Investigating this (seeing if it's an old bug or a result of my
patch).

Comment 31

[jag (Peter Annema)]

Hrm. So it works just fine on my machine (with my patch).

I wonder if racham's netscape build needs to be clobbered.

Comment 32

[Johnny Stenback (:jst)]

r=jst, but do you really want to add the #define DEBUG_CMD_LINE?

Comment 33

[jag (Peter Annema)]

Doh, nope. Meant to take that out.

Comment 34

[jag (Peter Annema)]

Attachment: [result matrix] Various tests done on a build with the last patch applied

Comment 35

[jag (Peter Annema)]

Attachment: [result matrix] Various tests done on a build with the last patch applied

Comment 36

[jag (Peter Annema)]

Sorry 'bout that, first one I marked as patch instead of text/html. Oops :-)

Anyway, as this results matrix shows with this patch we're doing the right thing
in all cases, with the exception of the two cases marked with a [2], where I'm
not sure about the correct behaviour, but we're (at least) doing the same as
without this patch.

Comment 37

[racham]

jag,

Compiling (clobber or not, clobber is always better though) xpfe isn't
sufficient here as bootstrap dir isn't part of it. You might want to clobber
xpfe/bootstrap separately and build it there. Only then, we capture
nsAppRunner.cpp changes.

The current code isn't compiling at line, 
+    if (!urlToLoad.IsEmpty()) {

you might want to change that to 
+    if (urlToLoad.get()) {

as there is no IsEmpty() for nsXPIDLCString. 

May be you want to revisit that portion. Otherwise, patch looks fine.

I still have to test activation cases. I will do that soon (I will proceed with
.get() as I mentioned above, or will use your updated patch if I see one before
I start my testing). I will clobber my ns tree and mozilla/xpfe,
mozilla/xpfe/bootstrap before testing scenarios.

Please post a new patch (now you can get rid of debug statements once you are
done with the testing).

If I missed anything, please do let me know.

Comment 38

[jag (Peter Annema)]

> Compiling (clobber or not, clobber is always better though) xpfe isn't
> sufficient here as bootstrap dir isn't part of it. You might want to clobber
> xpfe/bootstrap separately and build it there. Only then, we capture
> nsAppRunner.cpp changes.

Yes, I know :-) I was suggesting however that when we tried my patch on your
machine the changes weren't picked up for some reason or other, which could be
remedied by doing a clobber build on your whole netscape tree.

> The current code isn't compiling at line, 
> +    if (!urlToLoad.IsEmpty()) {
> 
> you might want to change that to 
> +    if (urlToLoad.get()) {
> 
> as there is no IsEmpty() for nsXPIDLCString. 

Ah, of course, the branch doesn't have the changes to nsXPIDLString which added
IsEmpty(). I'll come up with a new patch for the branch, for the trunk I'll keep
the version which uses IsEmpty() (minus the #define).

Comment 39

[jag (Peter Annema)]

Attachment: [patch-branch] Adapted to branch with old string api

Comment 40

[racham]

Is the matrix posted by you generated when tested with trunk build..?

Comment 41

[jag (Peter Annema)]

Yes.

Comment 42

[racham]

oh..we need testing and the matrix generation on branch primarily, to get this
onto  092branch. Otherwise, results may be skewed with differences in the code
base. Are you building branch now..?

I am starting my activation test cases now (bug 7443). If you can cover your the
matrix generation you did on the trunk with commercial branch build also, it
will be really useful. 

Comment 43

[jag (Peter Annema)]

Pulling and building.

Comment 44

[racham]

After applying the latest patch (+after backing out morse's changes i.e., fix in
91585), all the results in the startpage matrix (for bugscape 7443) are as
expected (will post the matrix html file in the next update). 

Jag is now covering several other test cases and generating a matrix (similar to
the one he did in his earlier updates) with the latest patch (on a branch build)
to make sure we get the expected results with no side effects. Jag, thanks for
making this better.

Please post the new matrix & make sure we are ok on all cases -> r=bhuvan.

Adding Conrad to the cc list.

Comment 45

[jag (Peter Annema)]

On the branch the results matrix is identical to the one on the trunk.

Comment 46

[Ben Goodger (use ben at mozilla dot org for email)]

ok. sr=[e-mail] for the branch patch. 

Comment 47

[jag (Peter Annema)]

And of course the most important test: javascript:window.open(); now does the
right thing (which is to open a blank window) :-)

Also tested mozilla /turbo, it too works just fine.

Selmer, to answer your question about what happened to the |if !turboMode)| in
javascript, why it's not in the "corresponding" section in nsAppRunner.cpp:
we didn't want that code to be executed in the turboMode case. In nsAppRunner,
turbo mode goes through a different path than where I moved the code, so
effectively you end up with the same result.

Comment 48

[Ben Goodger (use ben at mozilla dot org for email)]

and sr=[e-mail] for patch-really-now for the trunk too

Comment 49

[selmer (gone)]

PDT+, if you're comfortable that this is ready, let's get it in.

Comment 50

[Blake Ross]

Furthermore, that turbo js code is obsolete on the branch because we close the
window in C++ now elsewhere. I haven't yet checked that change onto the trunk.

Comment 51

[Conrad Carlen (not reading bugmail)]

> I haven't yet checked that change onto the trunk.

The turbo window will be closed from navigator.js on the trunk. The way it's
being done from C++ on the branch, while not harmful, is not quite right.

Comment 52

[jag (Peter Annema)]

Checked in on trunk and branch. Please test builds with this patch to see if it
fixed your favorite "homepage loads" bug too.

Comment 53

[jag (Peter Annema)]

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

Comment 54

[Jesse Ruderman]

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

Comment 55

[Sivakiran Tummala]

verified ...