window.open("") opens home page

VERIFIED FIXED in mozilla0.9.3

Status

()

VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: jbrunette, Assigned: jag-mozilla)

Tracking

({dom1})

Trunk
mozilla0.9.3
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PDT+)

Attachments

(7 attachments)

(Reporter)

Description

18 years ago
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?
Over to DOM level 1 for triage.  Seeing this behavior on Linux build
2001-01-18-08, so OS -> All
Assignee: asa → jst
Component: Browser-General → DOM Level 1
OS: Windows 2000 → All
QA Contact: doronr → janc

Comment 2

18 years ago
Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

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

Comment 4

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

Comment 5

18 years ago
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

18 years ago
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

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

Comment 8

18 years ago
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?

Updated

18 years ago
Blocks: 65714

Comment 9

18 years ago
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

18 years ago
Created attachment 24506 [details] [diff] [review]
[patch] Fix till we think of something better

Comment 11

18 years ago
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

18 years ago
r=blake

Comment 14

18 years ago
The attached patch causes http://bugzilla.mozilla.org/show_bug.cgi?id=69521 to
occur.
Keywords: dom1
Component: DOM Level 1 → DOM HTML

Comment 15

18 years ago
QA contact Update
QA Contact: janc → desale

Comment 16

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

Marking fixed.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 17

18 years ago
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala

Comment 18

18 years ago
verified on build 2001-06-21-11-0.9.1 windows 2000. marking as verified
Status: RESOLVED → VERIFIED
(Assignee)

Comment 19

17 years ago
Reopening, this regressed a while back, probably with alecf's recent checkin.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 20

17 years ago
yes jag looks like it is broken again...
(Assignee)

Comment 21

17 years ago
Created attachment 42980 [details] [diff] [review]
[patch] Clean up navigator.js, fix nsAppRunner.cpp
(Assignee)

Comment 22

17 years ago
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.
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?
(Assignee)

Comment 24

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

Comment 26

17 years ago
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?
(Assignee)

Comment 27

17 years ago
Created attachment 43091 [details] [diff] [review]
[patch] This also grabs the -P jag and --profilemanager (etc.) cases
(Assignee)

Comment 28

17 years ago
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.
Assignee: jst → jaggernaut
Status: REOPENED → NEW
(Assignee)

Comment 29

17 years ago
Created attachment 43092 [details] [diff] [review]
[patch-really-now] This also grabs the -P jag and --profilemanager (etc.) cases
(Assignee)

Comment 30

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

Comment 31

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

I wonder if racham's netscape build needs to be clobbered.
r=jst, but do you really want to add the #define DEBUG_CMD_LINE?
(Assignee)

Comment 33

17 years ago
Doh, nope. Meant to take that out.
(Assignee)

Comment 34

17 years ago
Created attachment 43126 [details] [diff] [review]
[result matrix] Various tests done on a build with the last patch applied
(Assignee)

Comment 35

17 years ago
Created attachment 43128 [details]
[result matrix] Various tests done on a build with the last patch applied
(Assignee)

Comment 36

17 years ago
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

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

Comment 38

17 years ago
> 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).
Status: NEW → ASSIGNED
(Assignee)

Comment 39

17 years ago
Created attachment 43135 [details] [diff] [review]
[patch-branch] Adapted to branch with old string api
(Assignee)

Updated

17 years ago
Target Milestone: --- → mozilla0.9.3

Comment 40

17 years ago
Is the matrix posted by you generated when tested with trunk build..?
(Assignee)

Comment 41

17 years ago
Yes.

Comment 42

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

Comment 43

17 years ago
Pulling and building.

Comment 44

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

Comment 45

17 years ago
On the branch the results matrix is identical to the one on the trunk.
(Assignee)

Comment 47

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

Updated

17 years ago
Blocks: 91793
and sr=ben@netscape.com for patch-really-now for the trunk too

Comment 49

17 years ago
PDT+, if you're comfortable that this is ready, let's get it in.
Whiteboard: PDT+

Comment 50

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

Comment 52

17 years ago
Checked in on trunk and branch. Please test builds with this patch to see if it
fixed your favorite "homepage loads" bug too.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 53

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

Comment 54

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

Comment 55

17 years ago
verified ...
Status: RESOLVED → VERIFIED

Updated

10 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.