137698 - [unix] re-enable the cached compose window on linux

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

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

[unix] re-able the cached compose window on linux

until the problem in bug #130581 is fixed ("Cannot enter addresses and subject 
of new mail until focus changes"), we've disabled the cached compose window.

this means that message compose is going to be slower on linux.

Comment 1

[Aleksey Nogin]

Patch in bug 82534 may fix bug 130581 properly (see bug 82534, comment #256 and
bug 130482, comment #5), so I've marked it as a dependency.

Comment 2

[Andrew Schultz]

yes.  I just tested it out.  Bug 130482 was fixed by the patch in 130581, and
the patch in bug 82534 fixes both.

Comment 3

[Andrew Schultz]

I testing linux build 20020620 (with bug 82534 fixed), but the compose window
seems to have serious problems that prevent me from determining whether this is
fixed.  Even the first compose window does not have focus on the address,
although clicking in the textarea works.  If I type in an address and hit cancel
so that the bug triggers, the alert window does not come up.  In the Javascript
console there was over a page of errors.

My CVS build seems to behave correctly, but backing out bug 130581 re-introduced
the bug (fixing bug 82534 did not fix it).  I applied/backed out bug 82534
multiple times during testing before and always saw consistent behavior, so I'm
more than a bit confused.

Comment 4

[Aleksey Nogin]

Abdrew, bug 82534 is *not* fully fixed, the patch there is *Windows-only*! So
not wonder it's still not working on Linux...

Comment 5

[Andrew Schultz]

it was targetted only at windows, and specifically at the window minimization
weirdness, but there were a few parts that were not win32-specific.

I applied the patch (before) and problems went away on Linux.  I suspect that
either I was high on goof-balls everytime I applied the patch or something
happened between 6-10 and now that is causing this bug to still be a problem.

the compose problems was bug 153354 (fixed)

Comment 6

[Andy Ross]

What's the status of this bug?  It still isn't fixed, and appears to have been
forgotten. I just submitted what appears to be a duplicate bug 200206 yesterday.

Popping the compose window up on my machine is pathetically slow: six seconds on
an Athlon XP 1800.  To anyone on a low end machine, it must be completely unusable.

Is anyone working on this at all?

Comment 7

[Mike Fedyk]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030326

model name      : Pentium II (Klamath)
stepping        : 3
cpu MHz         : 233.866
cache size      : 512 KB

MemTotal:       158960 kB
  PID USER      PR  NI  VIRT  RES  SHR SWAP S %CPU %MEM    TIME+  CODE DATA nFLT
nDRT Command                        
 9111 mfedyk    15   0 88304  72m  44m  14m S  0.0 46.5  29:10.32  156  71m  43k
 13k mozilla-bin                    

With this setup, it only takes 8 seconds.  While compose window recycling might
help speed up mozilla for this instance, I suspect that what you are seeing is a
memory footprint issue, [Bug 92580], or the swap-in slowness problem [Bug 76831].

Comment 8

[Andy Ross]

Nope.  Definitely not a VM issue (this 512MB machine has zero swap
space).  Could be a too-many-round-trips-to-the-X-server issue (which
wouldn't scale linearly with CPU speed), since the window lays itself
out several times (!) in the process of creation.

My guess is you are timing based the moment the window pops up on the
desktop instead of waiting for the ability to actually enter data.  It
thrashes around quite a bit before it will let you type.  The split
seems to be about 50/50 on my box, which would make your number 16
seconds.

And: you can't seriously be claiming that an 8 second delay (!) every
time you send an email on a low end machine is not a bug?  This needs
to be fixed.  No other email client I am aware of is even within an
order of magnitude of Mozilla's slowness here.

Comment 9

[Mike Fedyk]

Nope.  That is from the moment the compose toolbar button is pressed until the
moment all window drawing is finished and I can enter an email address.  So no
doubling please, thank you.  And no, I don't think 8 secs is good either, but
it's close to 6 secs on your athlon (surprising even).

I'm running Debian Sarge (testing) with Xfree86 4.2.1 with a sorry ass S3
VirgeDX.  And a SMP kernel on a UP box.  So my setup isn't exactly made for
speed.  I'm really surprised that this is so slow on your system.

# lspci 
00:00.0 Host bridge: Intel Corp. 440FX - 82441FX PMC [Natoma] (rev 02)
00:07.0 ISA bridge: Intel Corp. 82371SB PIIX3 ISA [Natoma/Triton II] (rev 01)
00:07.1 IDE interface: Intel Corp. 82371SB PIIX3 IDE [Natoma/Triton II]
00:07.2 USB Controller: Intel Corp. 82371SB PIIX3 USB [Natoma/Triton II] (rev 01)
00:08.0 VGA compatible controller: S3 Inc. ViRGE/DX or /GX (rev 01)
00:09.0 Ethernet controller: 3Com Corporation 3c905B 100BaseTX [Cyclone] (rev 30)
00:0a.0 Ethernet controller: Digital Equipment Corporation DECchip 21140
[FasterNet] (rev 22)
00:0b.0 Ethernet controller: 3Com Corporation 3c905B 100BaseTX [Cyclone] (rev 30)
00:0c.0 RAID bus controller: CMD Technology Inc PCI0680 (rev 02)

# uname -a
Linux mis-mike-wstn 2.4.20aa1-reiser #1 SMP Tue Jan 21 09:20:47 PST 2003 i686
Pentium II (Klamath) GenuineIntel GNU/Linux

# dpkg -l xserver-xfree86
ii  xserver-xfree86         4.2.1-3                 the XFree86 X server

Comment 10

[Brian Ryner (not reading)]

It might be worth testing whether the problems described in 130581 still happen
with the cached compose window enabled.  Some focus and editor fixes have gone
in since then.

Comment 11

[Andrew Schultz]

bug 130581 still occurs with cached compose window on linux trunk 20030402  :(

Comment 12

[Andrew Schultz]

with cached compose window enabled, I get this when I close Mozilla (if I have
opened even one compose window):
Gtk-CRITICAL **: file gtkmain.c: line 582 (gtk_main_quit): assertion `main_loops
!= NULL' failed.

Comment 13

[sairuh (rarely reading bugmail)]

when using brian's test build (from 4/28), i think bug 130581 is fixed, after i
enable cached compose windows on linux.

i'll run through my other focus/keyboard nav tests with the linux testbuild with
this pref enabled --perhaps we could enable this on linux if things look fine?

Comment 14

[sairuh (rarely reading bugmail)]

so far this looks good with brian's test build, after running keyboard
navigation tests for focus and selection.

would it be worth turning this on for 1.4final? esther / ninoschka, what would
you think?

Comment 15

[sairuh (rarely reading bugmail)]

oh, hrm, i'm using gnome, not kde (where people have seen problems here).

Comment 16

[sairuh (rarely reading bugmail)]

tested with brian's build in KDE v3.0.3-8.3 under redhat 8.0, with "focus
follows mouse". i do see the problem mentioned in bug 130581 comment 21
*erratically*, with this particular scenario:

1. open a new mail compose window.
2. notice that the caret is blink in To: field. enter some text --you'll notice
that you're able to (focus is fine).
3. close mail compose window (don't save it when prompted).
4. open another mail compose window.

results: no caret or focus in the To: field. if you kit the tab key, eventually
the caret appears in the composition area.

strangely, after this occurrence (if it occurs at all), it doesn't seem occur
again with subsequent new mail compose windows in the same session. weird.

Comment 17

[Mike Fedyk]

I'm seeing a major slowdown here now that I have upgraded to GTK2, maybe that's
part of what people are seeing here.

I also run mozilla in KDE.  If someone would be willing to post binaries I could
run, I'll help test.

Comment 18

[sairuh (rarely reading bugmail)]

i see gtk2 build directories:

http://ftp.mozilla.org/pub/mozilla/nightly/experimental/gtk2/ has several builds
and more recent ones, too.

but there's also http://ftp.mozilla.org/pub/mozilla/nightly/experimental/gtk2/
which has one build from earlier this month.

which is more appropriate to use here?

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

sairuh: you pasted two identical links in.  I assume that wasn't the intent...

Comment 20

[Andrew Schultz]

I've discovered what seems to be the problem (at least currently).  When a
recycled window is opened...

http://lxr.mozilla.org/mozilla/source/mailnews/compose/resources/content/MsgComposeCommands.js#230
  onReopen: function(params) {
>   //Reset focus to avoid undesirable visual effect when reopening the winodw
>   var identityElement = document.getElementById("msgIdentity");
>   if (identityElement)
>     identityElement.focus();

    InitializeGlobalVariables();
    ComposeStartup(true, params);
-------------------------------------------------------------
The identityElement.focus code is from bug 152895 and bug 141648.  But if I take
it out, this bug goes away.  I'm also not seeing bug 152895 or 141648 regress,
but they might have been mac/win only.

Comment 21

[sairuh (rarely reading bugmail)]

oops, right. the second link should be

http://ftp.mozilla.org/pub/mozilla/nightly/experimental/gtk2_native_theme/

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Andrew: sounds like the thing to do is to attach a patch to this bug, and then
get folks to test on Mac and Windows...

Comment 23

[Andrew Schultz]

Attachment: patch

this patch fixes the recycled compose window for Linux.  It needs testing for
Mac and Windows to see if it regresses bug 141648.

Comment 24

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

it would be great to be able to enable the cached compose window on linux.

but, as [e-mail] points out, we need to test and make sure that old
work around can safely be removed.

It would be a nice perf win for 1.5 on linux, it worked out.  (setting blocking
1.5? for asa to consider).

unfortunately, I don't have cycles for this right now.  asa, can you help?

(it would be nice for the next rev of tbird too, so cc mscott)

Comment 25

[Asa Dotzler [:asa]]

This is going to need more widespread testing than we're likely to get in time
for 1.5. Let's get it in first thing in 1.6alpha.

Comment 26

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

it would be great to enable this on linux, now that #130581 is fixed and we're
in 1.6 alpha.

over to mscott, to make that decision (and enable it.)

Comment 27

[Benjamin Smedberg]

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

Comment 28

[Asa Dotzler [:asa]]

Seth, Scott, time is short for 1.6a.  If this is going to happen, it needs to
happen quickly. 

Comment 29

[Scott MacGregor]

I don't understand what this patch means vs. the pref which I would presume
needs to be changed to turn the cached compose window on.

Not worth holding 1.6a to have someone test this out on linux. Let's turn the
pref on as soon as the tree re-opens.

Comment 30

[Asa Dotzler [:asa]]

OK. Thanks for the plan Scott. Setting flags accordingly.

Comment 31

[Andrew Schultz]

> I don't understand what this patch means vs. the pref which I would presume
> needs to be changed to turn the cached compose window on.

yes, the pref would need to get removed from unix.js as well. 

> Not worth holding 1.6a to have someone test this out on linux. Let's turn the
> pref on as soon as the tree re-opens.

I've tested it out on Linux and it works (although more testing certainly can't
be a bad thing).  I attached the patch in hopes that someone would test on
Windows, but that never happened.

Comment 32

[[e-mail]]

I've enabled the pref without the patch and see no obvious ill effects.

Comment 33

[Andrew Schultz]

FWIW: GTK still seems to have issues with recycled compose window (a bit worse
than in comment 12)

with gtk1 everything seems to work ok, but it still complains when I exit Mozilla:
> Gtk-CRITICAL **: file gtkwidget.c: line 1388 (gtk_widget_destroy): assertion 
> `GTK_IS_WIDGET (widget)' failed.
>
> Gtk-CRITICAL **: file gtkmain.c: line 582 (gtk_main_quit): assertion 
> `main_loops != NULL' failed.

with gtk2, when I close (or send mail from) the first compose window I open, it
leaves a zombie compose window (all inputs are greyed out and disabled).  I can
close it and then things continue normally.

Comment 34

[Asa Dotzler [:asa]]

if we've still got problems and this didn't get enabled for the bulk of the beta
cycle then it's too late for 1.6. Sorry. If this is going to make it into
mozilla then someone needs to champion it and get it in first thing in 1.7 alpha.

Comment 35

[Scott MacGregor]

Andrew,just so I am clear. Your patch fixes the GTK1 and GTK2 issues that you
just described? 

Or do we need your patch + some more code changes to eliminate the GTK errors?

Comment 36

[Andrew Schultz]

with GTK1, the only evidence of a problem is the message in the console when you
exit.  There are no other symptoms that bad behavior that I can see.
The attached patch does not fix the messages, or the zombie window in GTK2, so
additional code is needed.

But even if the GTK1/2 problems are severe enough to prevent re-enabling cached
window composeby default, then it would still be nice to land the patch (which
only removes the focus hack) so that linux people can enable cached window
compose via the pref themselves and so that the GTK1/2 issues can get more
visibility.

Comment 37

[Andrew Schultz]

Comment on attachment 124676 [details] [diff] [review]
patch

looks like sspitzer isn't going to review this

Comment 38

[[e-mail]]

Comment on attachment 124676 [details] [diff] [review]
patch

Sorry, I can't review this, I only have a gtk1 build. Apart from the widget
assertion I see no ill-effects from recycling the compose window.

Comment 39

[Andrew Schultz]

Attachment: re-enable cached compose window

With bug 301082 fixed, I see no adverse effects (no zombie window or focus
problems) with cached compose window enabled (tested both gtk1 and gtk2
builds).

My previous patch is probably still a good thing, but it's not very exciting
and is no longer needed.

Comment 40

[Scott MacGregor]

Comment on attachment 189997 [details] [diff] [review]
re-enable cached compose window

Let's get this checked in. We've waited long enough :)

Let me know if you need me to drive it in.

Comment 41

[Jean-Francois Ducarroz]

Comment on attachment 189997 [details] [diff] [review]
re-enable cached compose window

this is a good news. R=ducarroz (just in case somebody request it)

Comment 42

[Andrew Schultz]

Comment on attachment 189997 [details] [diff] [review]
re-enable cached compose window

need to get this in so it can get tested by the masses

Comment 43

[Scott MacGregor]

Fix checked in! Woo Hoo! Thanks for the patch.