472874 - Remove obsolete trim on minimize code

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[PSC]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/525.19 (KHTML, like Gecko) Chrome/1.0.154.36 Safari/525.19
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2

Windows 7 build 7000 beta
Windows Live Messenger 9.0 (14.0.8050.1202)

State:
1) Windows Live messenger running and minimized.
2) Open Firefox to any webpage.
3) Minimize FireFox and Live messenger will pop to the open state on the desktop.

Sort of odd but happens every time.

Other software installed:
Office 2007
Visual Studio 2008
SQL Server 2005

Dell XPS M1530 laptop. 

Reproducible: Always

Steps to Reproduce:
1) Windows Live messenger running and minimized.
2) Open Firefox to any webpage.
3) Minimize FireFox and Live messenger will pop to the open state on the desktop.
Actual Results:  
Windows Live messenger will consistenly pop open.  Firefox is the only application that does this.  

Expected Results:  
I would expect Firefox to not open live messenger.

Pretty irritating bug. I've had to stop using Firefox until this issue is resolved. :/

Comment 1

[Tyler Downer [He/Him]]

Does it happen in Safe Mode? Could be a Win 7 bug though.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

confirming. It looks buggy. Not sure where the bug is but it does not happen with IE+MSN, only Fx+MSN.

Comment 3

[PSC]

Yes it does happen in safe mode.  Only happens with FX+MSN.  IE, and Chrome do not cause this behavior.

Comment 4

[Nicolas Redfern]

Bug happens on 64 bit version of Win7 as well.

Comment 5

[James Ross]

What about if you create (via about:config) a boolean preference called "config.trim_on_minimize" and set it to true, then restart Firefox; does it still happen?

Comment 6

[Alex Dobie]

I can confirm that the config.trim_on_minimize trick above fixes this issue on Firefox 3.1b2 and Thunderbird 3.0b1

Comment 7

[Hernán Pablo Alvarez]

I confirm that the "config.trim_on_minimize" setting fix this bug on Windows 7 build 7068 and Firefox 3.0.8

Comment 8

[Oscar]

I confirm that the "config.trim_on_minimize" setting fix this bug on Windows 7
build 7100 and Firefox 3.5.b4

Comment 9

[James Ross]

My guess here is that the code I did for bug 300689 is finding a window it shouldn't; it's probably finding the window used to show the Messenger "status" overlay on the taskbar, since focusing that causes the main Messenger window to open.

My code looked for the next visible, non-minimised window and focused that.

Anyone that wants to try restricting it further - or just commenting out the code my patch added to check that is the cause - is welcome to try (I don't have a working Firefox dev environment).

Comment 10

[James Ross]

A quick exploration with Spy++ reveals with the Messenger status window is disabled, so perhaps that could be the extra restriction needed here. It'd need appropriate testing with programs with modal dialogs open and such like.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

We're not shipping with support for Windows 7, as it's not a final release yet, but I would indeed love to see this fixed. I'd also support shipping it in a stability update if we can't get it done in time.

Jim: can you attempt the fix that James is talking about in comment 10?

Comment 12

[Jim Mathies [:jimm]]

With a little testing, we could consider enabling trim_on_minimize by default which would disable the focus functionality. (I'm not seeing any performance issues on restore on Vista or win7.) Maybe the issue bug 76831 meant to address has since been addressed in other places? The whole scenario feels a little hackish to begin with.

Comment 13

[Jim Mathies [:jimm]]

This is looking more and more like the kind of code we can finally pull because we don't support 95, 98, and 2K anymore. I'm testing now with a single window, 8 open tabs on win7 with trim_on_minimize set to true. The messenger problem isn't present and restoring the window / working set seem to be just fine. Anyone else care to do some testing, looking for window redraw issues after extended periods of minimization?

Comment 14

[PSC]

I'll do some more testing here.  I'm using Windows 7 RC build 7100 now though.

Comment 15

[Alex Dobie]

For what it's worth, I've been using Firefox 3.1b2 to 3.5b4 and Thunderbird 3.0b1 to 3.0b2 with the trim_on_minimize setting enabled on various builds on Windows 7 since January and haven't noticed any redraw issues whatsoever.

Comment 16

[Jim Mathies [:jimm]]

One interesting thing I found, we've been piling pref check code into the gTrimOnMinimize == 2 if block without realizing it. (At least it seems that way, I don't think render mode and keyboard layout prefs should be tied to trim on minimize settings.) I wonder if that's causing other bugs.

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#1320

Comment 17

[Jim Mathies [:jimm]]

(In reply to comment #16)
> One interesting thing I found, we've been piling pref check code into the
> gTrimOnMinimize == 2 if block without realizing it. (At least it seems that
> way, I don't think render mode and keyboard layout prefs should be tied to trim
> on minimize settings.) I wonder if that's causing other bugs.
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#1320

Ahh, never mind, we actually use the value of gTrimOnMinimize as an indicator of initial startup!

Comment 18

[Jim Mathies [:jimm]]

Attachment: m-c trim_on_minimize patch v.1

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 377271 [details] [diff] [review]
m-c trim_on_minimize patch v.1

This doesn't seem to be causing any problems in testing. It'll be interesting to see if we get any feedback once this lands on trunk.

I'm working up the one off for 1.9.1 as well, should have that here in a bit.

Comment 20

[Jim Mathies [:jimm]]

Attachment: 1.9.1 activate patch v.1

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 377485 [details] [diff] [review]
1.9.1 activate patch v.1

A simple patch that adds a filter for disabled windows. Tested on win7, gets rid of the problem.

Comment 22

[James Ross]

Comment on attachment 377485 [details] [diff] [review]
1.9.1 activate patch v.1

Did you try the function BOOL IsWindowEnabled(HWND)?

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #22)
> (From update of attachment 377485 [details] [diff] [review])
> Did you try the function BOOL IsWindowEnabled(HWND)?

Yeah I imagine that would work as well. :) I started out getting as much info as possible, I suppose I could dumb it down to just that call.

Comment 24

[Jim Mathies [:jimm]]

Attachment: [checked in: fa212420de96] 1.9.1 activate patch v.1

That works too.

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Stuart, do we still need trim on minimize?  I seem to remember it having significant memory usage effects.

Comment 26

[[e-mail]]

(In reply to comment #13)
> This is looking more and more like the kind of code we can finally pull because
> we don't support 95, 98, and 2K anymore.
2K got dropped already? Or were you thinking of Me?

Comment 27

[Jim Mathies [:jimm]]

(In reply to comment #26)
> (In reply to comment #13)
> > This is looking more and more like the kind of code we can finally pull because
> > we don't support 95, 98, and 2K anymore.
> 2K got dropped already? Or were you thinking of Me?

1.9.2 is xpsp2 and up I believe. There were a couple threads on the newsgroups about. I think it was on planning.

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Correct, though I think we said that we wouldn't go out of our way to break 2K.  If we can remove this code and not break basic functionality on 2K, sounds good to me.

Comment 29

[Luke Iliffe (Harlequin99)]

The incidence of live messenger popping up when shiretoko is minimised has reduced from happening every time. However for me on Win 7 RC (7100) it is still happening occasionally. I haven't been able to find a way of reproducing it reliably.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1pre) Gecko/20090523 Shiretoko/3.5pre ID:20090523044232

Comment 30

[Ere Maijala (slow)]

Comment on attachment 377271 [details] [diff] [review]
m-c trim_on_minimize patch v.1

I'm quite hesitant on this. Trimming never was a problem for everyone so we don't have enough data to say it isn't needed anymore. I'm still seeing heavy disk activity before anything happpens e.g. after resuming from hibernation, which kind of indicates that we might still be suffering from the same issue.

Comment 31

[Stuart Parmenter]

(In reply to comment #25)
> Stuart, do we still need trim on minimize?  I seem to remember it having
> significant memory usage effects.

All it does is swaps out the process, so next time you restore it, it can take a while to reload everything.

Comment 32

[Jim Mathies [:jimm]]

(In reply to comment #30)
> (From update of attachment 377271 [details] [diff] [review])
> I'm quite hesitant on this. Trimming never was a problem for everyone so we
> don't have enough data to say it isn't needed anymore. I'm still seeing heavy
> disk activity before anything happpens e.g. after resuming from hibernation,
> which kind of indicates that we might still be suffering from the same issue.

Hibernation restores the os and session from disk, so that's probably not us. Fx does do some work on hibernation events, but that shouldn't play into minimizing a window. 

Since the people who have tested this here don't see any issues, why don't we try disabling trim on minimize by default, and land the messenger fix as well, on trunk? This way we have an easy work around and can use trunk to test if trim is still needed? If by the time we get to releasing betas of fx.next no one has raised an issue, we can land the original patch.

Comment 33

[Ere Maijala (slow)]

I didn't mean the resume phase where the working set is restored from disk, but what happens after that when the system becomes usable. When other apps become responsive in a fair amount of time, Firefox sometimes take significantly more time and seems to swap in heavily. This is because, as far as I know, Windows trims working sets before hibernating to minimize the amount of data it needs to write to the hibernation file. This trimming is the same thing we avoid doing upon minimizing with the hack you want to remove. 

I'm still not quite convinced we wouldn't suffer from the old problems if the hack is removed. Please convince me that we don't or that it's more important to remove it to fix the problem here. Please note also that if people test trimming with systems with enough (free) memory, the problem might not appear at all or be so visible.

Comment 34

[Ere Maijala (slow)]

Comment on attachment 377271 [details] [diff] [review]
m-c trim_on_minimize patch v.1

Canceling review pending more feedback.

Comment 35

[wraithdu]

I recently experienced this bug with Win7 RC1, FF 3.5 Final, and a launcher program called Executor (http://www.executor.dk/).  I have executor set to be activated via hotkey, and its window disappears when not focused.  When FF is minimized with no other desktop windows present, the executor window is focused and appears.  The config.trim_on_minimize hack fixes the problem.

Comment 36

[u88484]

Jim, didn't you mean Resolved fixed instead of WONTFIX?

Comment 37

[Jim Mathies [:jimm]]

(In reply to comment #36)
> Jim, didn't you mean Resolved fixed instead of WONTFIX?

It's a mixture, the original bug was all about removing the the trim on minimize code which never landed. (See comment 30) An updated 1.9.1 patch and a trunk version that fix the focus problems on win7 are now in bug 499816.

Comment 38

[James Ross]

The *original* bug was the minimise activation code not working (which is not fixed, but bug 499816 appears to be taking up that). :P

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #38)
> The *original* bug was the minimise activation code not working (which is not
> fixed, but bug 499816 appears to be taking up that). :P

Yeah I think I was the one who changed the title here. 1.9.1 is fixed right now with the landing of " [checked in: fa212420de96] 1.9.1 activate patch v.1 ". Bug 499816 updates that to remove some paren and has the trunk fix.