57197 - crash trying to send url to running session when ONLY mail/news window is up.

Status: VERIFIED FIXED

(Core Graveyard :: Cmd-line Features, defect, P3)

Description

[Joseph Elwell]

Steps to reproduce:
1) Launch Mozilla Mail (make sure no browser window is open)
2) Launch X-chat
3) right click on a URL in X-Chat
4) Select "-> Netscape (Existing).
5) Crash

This was tested on linux build 2000101809.

Comment 1

[Joseph Elwell]

stack trace:

#0  0x40c83b8b in nsGtkMozRemoteHelper::GetLastBrowserWindow
(_retval=0xbffff220) at nsGtkMozRemoteHelper.cpp:767
#1  0x40c82d13 in nsGtkMozRemoteHelper::OpenURL (aURL=0x89ca300
"mailto:[e-mail]", aNewWindow=0, raiseWindow=1) at
nsGtkMozRemoteHelper.cpp:569
#2  0x40c82336 in nsGtkMozRemoteHelper::ParseCommand (aCommand=0x84554f8
"openURL(mailto:[e-mail])", aResponse=0xbffff3a8) at
nsGtkMozRemoteHelper.cpp:306
#3  0x40c81ce2 in nsGtkMozRemoteHelper::HandlePropertyChange (aWidget=0x818f628,
aEvent=0x81dfd20) at nsGtkMozRemoteHelper.cpp:142
#4  0x40c7da69 in handle_toplevel_property_change (widget=0x818f628,
event=0x81dfd20, aData=0x814fcc0) at nsWindow.cpp:2783
#5  0x40d53e29 in gtk_marshal_BOOL__POINTER (object=0x818f628, func=0x40c7da4c
<handle_toplevel_property_change(_GtkWidget *, _GdkEventProperty *, void *)>,
func_data=0x814fcc0, args=0xbffff4a0) at gtkmarshal.c:28
#6  0x40d819ed in gtk_handlers_run (handlers=0x8187e90, signal=0xbffff45c,
object=0x818f628, params=0xbffff4a0, after=0) at gtksignal.c:1917
#7  0x40d80e32 in gtk_signal_real_emit (object=0x818f628, signal_id=35,
params=0xbffff4a0) at gtksignal.c:1477
#8  0x40d7ef35 in gtk_signal_emit (object=0x818f628, signal_id=35) at
gtksignal.c:552
#9  0x40db434c in gtk_widget_event (widget=0x818f628, event=0x81dfd20) at
gtkwidget.c:2860
#10 0x40d5307a in gtk_main_do_event (event=0x81dfd20) at gtkmain.c:786
#11 0x40c6a25e in handle_gdk_event (event=0x81dfd20, data=0x0) at
nsGtkEventHandler.cpp:963
#12 0x40dfd56b in gdk_event_dispatch (source_data=0x0, current_time=0xbffff83c,
user_data=0x0) at gdkevents.c:2129
#13 0x40e2a1b6 in g_main_dispatch (dispatch_time=0xbffff83c) at gmain.c:656
#14 0x40e2a781 in g_main_iterate (block=1, dispatch=1) at gmain.c:877
#15 0x40e2a921 in g_main_run (loop=0x81dfce8) at gmain.c:935
#16 0x40d52919 in gtk_main () at gtkmain.c:476
#17 0x40c5f1aa in nsAppShell::Run (this=0x8110118) at nsAppShell.cpp:335
#18 0x40756434 in ?? () from
/builds/branch/mozilla/dist/bin/components/libnsappshell.so
#19 0x8055f55 in main1 (argc=1, argv=0xbffffaf4, nativeApp=0x0) at
nsAppRunner.cpp:1004
#20 0x80565fe in main (argc=1, argv=0xbffffaf4) at nsAppRunner.cpp:1185
#21 0x403849cb in __libc_start_main (main=0x805644c <main>, argc=1,
argv=0xbffffaf4, init=0x8050c74 <_init>, fini=0x8064ee0 <_fini>,
rtld_fini=0x4000ae60 <_dl_fini>, stack_end=0xbffffaec) at
../sysdeps/generic/libc-start.c:92

Comment 2

[sairuh (rarely reading bugmail)]

not sure if this should got to chatzilla or mail. trying the former first...

Comment 3

[sairuh (rarely reading bugmail)]

does this happen running mozilla on mac or win32?

Comment 4

[Robert Ginda]

X-Chat is a standalone unix IRC client, not chatzilla.  Stack trace looks like
an issue with the -remote stuff, blizzard, is this you?

Comment 5

[sairuh (rarely reading bugmail)]

doh! my brain definitely was off, since i misread xchat as chatzilla. my bad.

Comment 6

[timeless]

putting back to xpcommandline features [hoping not to change assignee]

Comment 7

[Christopher Blizzard (:blizzard)]

-> [e-mail]

Comment 8

[Christopher Blizzard (:blizzard)]

Yeah, I've known about this bug for a while even though I haven't filed a bug on
it.  If you have only the mail window open ( no browser windows ) and you open
any kind of url with xremote it crashes.

Comment 9

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 10

[Christopher Blizzard (:blizzard)]

Patch that fixes this problem attached.  This patch has the interesting effect
that if you open a mailto: url via an openurl() and there are no browser windows
open that it will actually open a new browser window first.  I think that this
is actually a feature since it allows the browser handlers to catch the mailto:
load instead of hard coding it into the remote code.

Comment 11

[David Krause]

Fixing QA contact...

Comment 12

[Christopher Blizzard (:blizzard)]

Alec, can you review this?  It's little.

Comment 13

[Mike Shaver (:shaver emeritus)]

I don't think the open-an-unused-browser-window behaviour is a feature.  mscott
and others and I have discussed this at some length, and I think mscott has put
some non-trivial amount of work into making sure that we don't open dummy
windows, or force content types -- let alone protocol types! -- to be tied to
application-window types.

With your patch, what happens if I openURL a mailbox: or news: URL, with just a
messenger window open?  Not the right thing, I predict.  Seems like there should
be a way to just dispatch a given URL, and have the URI-dispatching code figure
out what window should be used or created for the resulting content.  Cc:ing
mscott to find out.

Comment 14

[Christopher Blizzard (:blizzard)]

If mscott has a solution for this I would be more than happy to use it.

Comment 15

[Peter "jag" Annema]

What we seem to want is the URILoader service, construct a nsURI from the url
passed to us, open a channel on it and set its load attribs as if we clicked
(see  nsDocShell::DoChannelLoad, case normal/clicked), and call
loader->OpenURI(channel, nsnull, nsnull, nsnull);

Alternatively, we may be able to (ab)use the hidden window's docShell and call
LoadURI(url, NSIWebNavigation::LOAD_FLAGS_NONE); on it.

Comment 16

[Christopher Blizzard (:blizzard)]

OK, so what is the target of those loads?  If it's the hidden window then no one
will be able to see it. :)

Comment 17

[Peter "jag" Annema]

If we use the hidden window's docshell and it's a http url, we might indeed not
see it...

W.r.t. the URILoader service, what happens is that it will find the right
content handler (if one exists) and hint it that the content was retargeted (the
nsnull context we passed in guarantees that we're always of a different kind
than the selected content handler). Hopefully the content handler will Do The
Right Thing (tm), whatever that is.

There may be some flags you can pass on through the second parameter which can
be used to hint "open in existing, or else in new"... I'd have to dig a bit
deeper...

Comment 18

[Alec Flett]

I am very willing to say [s]r=alecf on this first patch from blizzard, and
nominate for Netscape's rtm (since this is the only way I crash on linux these
days).. we haven't heard from mscott here - maybe we can take a better patch for
the tip, and use blizzard's patch on the branch?

For PDT: Blizzard's patch looks very safe, and otherwise this causes popular
external applications (xchat and gaim) to crash Mozilla just by clicking on a
link in their appplication.

Comment 19

[Christopher Blizzard (:blizzard)]

Well, no one has stepped forward with a better method for fixing this problem.
I'd like to get the crash fixed, even if it isn't perfect.  Like I said, I'm
hoping that someone will tell me a better method that I'm not aware of.

Comment 20

[Scott MacGregor]

Ughhh....I was just looking at the code in ::OpenURL and it scared me. As shaver
pointed out, it's bypassing most of the work the uriloader does for you to
figure out what window to bring up / and what kind of window to create in order
to handle the load!

I think that entire method should be replaced instead with a call to the
URILoader::OpenURI method.

However, this requires you to
1) create a channel
2) have an object which implements nsIURIContentListener and pass that in as the
window context for the load. See nsExternalHelperAppService for an example of a
light weight implementation of nsIURIContentListener.

That would be my recommendation for fixing this problem.

You won't need to looking for the hidden window, or trying to manually create a
browser window nor creating a browser window when we really don't want one (i.e.
in the mailto case).

Comment 21

[Christopher Blizzard (:blizzard)]

> ------- Additional Comments From [e-mail]  2000-10-30 11:38 -------
> Ughhh....I was just looking at the code in ::OpenURL and it scared me. As shaver
> pointed out, it's bypassing most of the work the uriloader does for you to
> figure out what window to bring up / and what kind of window to create in order
> to handle the load!
>
> I think that entire method should be replaced instead with a call to the
> URILoader::OpenURI method.
>
> However, this requires you to
> 1) create a channel

What kind of channel? I'm pretty ignorant of this stuff as you might be able to
tell from my code.

> 2) have an object which implements nsIURIContentListener and pass that in as the
> window context for the load. See nsExternalHelperAppService for an example of a
> light weight implementation of nsIURIContentListener.

This is easy and looks straight forward. What is the lifetime for that object?

>
> That would be my recommendation for fixing this problem.

I'll be happy to do this "the right way."

Comment 22

[Christopher Blizzard (:blizzard)]

OK, I just found the URL Loader service and some examples.  I'll play with it
for a while.

Comment 23

[Scott MacGregor]

coolness...let me know if you run into any questions....

Comment 24

[Christopher Blizzard (:blizzard)]

Attachment: patch 2

Comment 25

[Scott MacGregor]

sr=mscott

Comment 26

[Alec Flett]

r=alecf if you remove the printf's or at least put them in #ifdef DEBUG_blizzard
:)

Comment 27

[Christopher Blizzard (:blizzard)]

Yeah, I saw the printf() right after I made the patch.  I'll remove it. :)  Thanks.

Comment 28

[Christopher Blizzard (:blizzard)]

Checked in.  Someone might want to consider this for the Netscape branch.  It's
a pretty painless patch and fixes a crash.

Comment 29

[Alec Flett]

reopening so that we can get this on the branch
it's nominated for rtm, but it will fall off the radar if it's marked fixed

Comment 30

[selmer (gone)]

rtm-, not ship stopper.

Comment 31

[Jason Eager]

Changing summary to protect the PDT from millions of unix users thinking that
sending a url to a running session would crash ALL the time.

Comment 32

[Jeffrey Baker]

This patch breaks the moz-remote protocol.  Now, Mozilla no longer respects the
noraise argument to the openURL command.  I previously implemented a patch to
respect the noraise argument.  So, something has to change:

1) Ignore the noraise argument.  This means we should remove the code that
checks for noraise and change Bug #45919 to WONTFIX.

2) Fix OpenURI so it isn't as obnoxious about raising the window.

Comment 33

[Christopher Blizzard (:blizzard)]

Mozilla has never respected the noraise argument.  It's only recognised and it
and not returned an error when it's in the arg list.

Comment 34

[Jeffrey Baker]

With my patch (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=13111
), which was in the build for some while, the noraise argument was respected.
If it was not present, GtkMozRemoteHelper would call nsWindow::GetAttention() to
raise the window.  If it was present, the URL was loaded without raising the
window.  Now the window is always raised on a remote call.

Comment 35

[Christopher Blizzard (:blizzard)]

I prefer 2 myself.  Open another bug on it.  Re-closing since this bug is fixed.
 We no longer crash.

Comment 36

[sairuh (rarely reading bugmail)]

vrfy fixed with 2000.11.21.08 comm linux bits.