264980 - [MASv1.8a2++] Installer crashes when using Alt-N key on 'Select Components' step of 'Custom' install

Status: RESOLVED FIXED

(SeaMonkey :: Installer, defect)

Description

[Serge Gautherie (:sgautherie)]

Discovered while testing (at least some) nightlies for bug 263653:

This seems to have regressed between v1.7.3 and _before_ v1.8a4 releases;
that would need narrowing :-/

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.3) Gecko/20040910] (release) (W98SE)

Works fine.

(and at least the "end" of v1.8a4 trunk before)
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a4) Gecko/20040927] (release) (W98SE)
and current trunk up to Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5)
Gecko/20041018

1. Start (Windows full installer) install...
2. Select "Custom"
3. On "Select Components" step, press Alt-N (to go to Next page)

result: crash !
{{
SETUP a causé une défaillance de protection générale
dans le module USER.EXE à 0003:00009e46.
Registres :
EAX=c003322f CS=17b7 EIP=00009e46 EFLGS=00000202
EBX=000216bf SS=4327 ESP=000081e8 EBP=0069820c
ECX=00010000 DS=16bf ESI=3b380064 FS=35a7
EDX=8160647c ES=016f EDI=000264bc GS=0000
Octets à CS : EIP :
67 3b 46 20 75 59 8e c0 26 83 3e 68 00 00 75 4f 
État de la pile :
000264bc 3b380064 064616bf 00001767 00050b1c 01110403 3b380064 000264bc 00690008
062b8264 0005176f 3b380064 000264bc 82d04327 00060000 37d30000 
}}

Odd, because Alt-N works perfectly on previous install steps...

This could be related to bug 261857;
yet, mouse clicking Next button works fine for me: it's only a keyboard issue.

Comment 1

[Andrew Schultz]

Serge: not much changed in the windows installer since 1.7.  just testing alpha
releases would probably be sufficient.

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> Serge: not much changed in the windows installer since 1.7.  just testing alpha
> releases would probably be sufficient.

I could reduce the regression time frame to a 2-week period :-|
(no much nighlies available on http://archive.mozilla.org/pub/mozilla/nightly/
for that long time frame)

Works:

2004-06-25-09-trunk

2004-06-26-09-trunk
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a2) Gecko/20040626

Crashes:

2004-07-12-15-trunk
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a2) Gecko/20040712

2004-07-18-09-trunk
2004-08-19-10-trunk

Comment 3

[Andrew Schultz]

that narrows it down pretty well:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fxpinstall%2Fwizard%2Fwindows&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-06-26+08%3A00%3A00&maxdate=2004-07-12+15%3A00%3A00&cvsroot=%2Fcvsroot

Comment 4

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Sean, this looks like you (see check-in link above). And change your CVS account
address to one that works!

Comment 5

[Serge Gautherie (:sgautherie)]

'blocking1.8a5=?': (installer) crash (with regression timeframe), with (mouse)
workaround...

Comment 6

[Asa Dotzler [:asa]]

not gonna make it. we should get this first thing in a6.

Comment 7

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041122] (release) (W98SE)

Bug still there. (of course)

Asa commented "we should get this first thing in a6."...

Comment 8

[Asa Dotzler [:asa]]

sean or dveditz, any chance one of you can work up a fix for alpha6?

Comment 9

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a6) Gecko/20050104] (nightly) (W98SE)

(Bug still there.)

Comment 10

[Asa Dotzler [:asa]]

We need to get this fixed, but it's not a topcrash and we'd like to get the tree
open for beta sooner than later. If a patch happens today, we might still be
able to squeeze it in. 

Comment 11

[Mats Palmgren (inactive)]

Sean, are you working on this?

Comment 12

[Brian Ryner (not reading)]

It's crashing on the call to CallWindowProc at the end of NewListBoxWndProc
(dialogs.c line 1335), but inside about 7 frames of user32.dll.  I'm not sure
what to make of that.

Comment 13

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050212] (nightly) (W98SE)

(Bug still there.)

Comment 14

[Hermann Schwab]

(In reply to comment #3)
> that narrows it down pretty well:
> 
>
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fxpinstall%2Fwizard%2Fwindows&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-06-26+08%3A00%3A00&maxdate=2004-07-12+15%3A00%3A00&cvsroot=%2Fcvsroot

I´ve been looking around a bit, and found in the resource script only one dialog
having 2 styles, is that o.k., or was it forgotten to delete line 250 when line
251 was added?

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpinstall/wizard/windows/setuprsc&command=DIFF_FRAMESET&file=setuprsc.rc&rev1=1.61&rev2=1.62&root=/cvsroot
http://lxr.mozilla.org/mozilla/source/xpinstall/wizard/windows/setuprsc/setuprsc.rc#248

248 
249 DLG_MESSAGE DIALOG DISCARDABLE  0, 0, 236, 34
250 STYLE DS_MODALFRAME | WS_POPUP | WS_VISIBLE | WS_CAPTION
251 STYLE DS_MODALFRAME | WS_MINIMIZEBOX | WS_POPUP | WS_VISIBLE | WS_CAPTION |
WS_SYSMENU
252 CLASS "MozillaSetupDlg"
253 FONT 8, "MS Sans Serif"
254 BEGIN
255     CTEXT           "",IDC_MESSAGE,0,0,236,34,SS_CENTERIMAGE
256 END
257 

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <setuprsc.rc> [Checked in: Comment 22]

Per comment 14 suggestion.

I have no compiler: Could you compile/test/(super-)review this patch ? Thanks.

PS: Could you update your "unreferenced" '[e-mail]' on
<http://www.mozilla.org/hacking/reviewers.html> ? Thanks.

Comment 16

[Asa Dotzler [:asa]]

dan, when you get free from 1.0.1 can you help with reviews here?

Comment 17

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Comment on attachment 174245 [details] [diff] [review]
(Av1) <setuprsc.rc>
[Checked in: Comment 22]

r+sr=dveditz

Comment 18

[Sean Su]

Attachment: (Bv1) patch v1.0

This patch fixes the crash.

Not sure how the crash really happens, but an old window proc was being reused
inappropriately without being cleaned up before hand.  Might have been a memory
corruption of some type.

Added a new window proc for the different subclassing being done.  Also added a
mothod to unsubclass when exiting the respectible dialogs.

Cleaned up some various unreferenced vars too.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 174964 [details] [diff] [review]
(Bv1) patch v1.0

>Index: dialogs.c
>===================================================================
>RCS file: /cvsroot/mozilla/xpinstall/wizard/windows/setup/dialogs.c,v

nits only:

>   HWND  hwndLBFolders;
> 
>+  hwndLBFolders  = GetDlgItem(hDlg, 1121);

You could leave one space only before '=', (could be done elsewhere too, or
not)
and may be join these two lines into one.

Comment 20

[Sean Su]

Attachment: patch v1.1 [Checked in: Comment 37]

addressing the nit pick of extra spaces (compared to patch v1.0).

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #20)
> addressing the nit pick of extra spaces (compared to patch v1.0).

Good :-)
Yet, why not do the |h* = GetDlgItem()| merging part too, on the 3 place, like
{{
   HWND      hSTMessage = GetDlgItem(hDlg, IDC_MESSAGE); /* handle to the Static
Text message window */
}}
is ?

Comment 22

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050222
Build ID: 2005022205
There was a checkin today, 2005-02-22 01:15, but it didn´t fix the bug.
mozilla/ xpinstall/ wizard/ windows/ setuprsc/ setuprsc.rc
Is this file used by a tinderbox compile?
http://tinderbox.mozilla.org/bonsai/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpinstall/wizard/windows/setuprsc&command=DIFF_FRAMESET&file=setuprsc.rc&rev1=1.62&rev2=1.63&root=/cvsroot

I deinstalled, deleted the chrome folder, didn´t change the plugin folder
holding java for all my mozillas...
Did a download of yesterdays (unfixed) and todays (fixed?) exe-installer,
tested yesterdays, crashing on  Alt-N key on 'Select Components'  step of
'Custom' install, also on Alt-B key.
tested todays, same behaviour, crashing on Alt-B and Alt-N.

Installed using mouse.

Comment 23

[Sean Su]

(In reply to comment #22)
> There was a checkin today, 2005-02-22 01:15, but it didn´t fix the bug.
> mozilla/ xpinstall/ wizard/ windows/ setuprsc/ setuprsc.rc

The .rc file patch is needed, but is not what will fix the crash.  The patch
from comment #20 has not landed yet, and is what will fix the crash.  It is
awaiting r=/sr=

Comment 24

[Serge Gautherie (:sgautherie)]

Daniel, would you have time to review the v1.1 patch ?

Comment 25

[Hermann Schwab]

who removed 'blocking1.8b2+', and when was this removed?
This bug has a patch, why not fix it before it bitrots?

Comment 26

[Hermann Schwab]

(In reply to comment #25)
> who removed 'blocking1.8b2+', and when was this removed?

Seems the field 'blocking 1.8b2' was removed from Product: Mozilla Application
Suite and was replaced by field 'blocking-seamonkey1.0a', but without
transferring status, and without leaving trace in bug-activity.

blocking 1.8b2+ still is seen on 11 bugs core and 1 bug NSS,
blocking 1.8b2? is requested from 43 bugs core

Comment 27

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

It doesn't have to have blocking status to get fixed

Comment 28

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Comment on attachment 175059 [details] [diff] [review]
patch v1.1
[Checked in: Comment 37]

r/sr=dveditz
Does the forked Firefox/Thunderbird installer also need this fix?

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> (From update of attachment 175059 [details] [diff] [review] [edit])
> Does the forked Firefox/Thunderbird installer also need this fix?

Daniel, I guess it doesn't:

<http://lxr.mozilla.org/mozilla/find?string=%2Fsetup%2Fdialogs.c>
{{
/toolkit/mozapps/installer/windows/wizard/setup/dialogs.c
/xpinstall/wizard/os2/setup/dialogs.c
/xpinstall/wizard/windows/setup/dialogs.c
}}

<http://lxr.mozilla.org/mozilla/search?string=OldListBoxWndProc>
{{
OldListBoxWndProc

/toolkit/mozapps/installer/windows/wizard/setup/dialogs.c, line 80 -- static
WNDPROC OldListBoxWndProc;
/toolkit/mozapps/installer/windows/wizard/setup/dialogs.c, line 1217 --
return(CallWindowProc(OldListBoxWndProc, hWnd, uMsg, wParam, lParam));
/toolkit/mozapps/installer/windows/wizard/setup/dialogs.c, line 1269 --
OldListBoxWndProc = SubclassWindow(hwndLBComponents, (WNDPROC)NewListBoxWndProc);

/xpinstall/wizard/os2/setup/dialogs.c, line 50 -- static PFNWP OldListBoxWndProc;
/xpinstall/wizard/os2/setup/dialogs.c, line 1049 -- MRESULT mr =
(OldListBoxWndProc)(hWnd, msg, mp1, mp2);
/xpinstall/wizard/os2/setup/dialogs.c, line 1057 --
return(OldListBoxWndProc)(hWnd, msg, mp1, mp2);
/xpinstall/wizard/os2/setup/dialogs.c, line 1132 -- OldListBoxWndProc =
WinSubclassWindow(hwndLBComponents, (PFNWP)NewListBoxWndProc);
/xpinstall/wizard/os2/setup/dialogs.c, line 1274 -- OldListBoxWndProc =
WinSubclassWindow(hwndLBComponents, (PFNWP)NewListBoxWndProc);

/xpinstall/wizard/windows/setup/dialogs.c, line 54 -- static WNDPROC
OldListBoxWndProc;
/xpinstall/wizard/windows/setup/dialogs.c, line 453 --
return(CallWindowProc(OldListBoxWndProc, hWnd, uMsg, wParam, lParam));
/xpinstall/wizard/windows/setup/dialogs.c, line 473 -- OldListBoxWndProc =
SubclassWindow(hwndLBFolders, (WNDPROC)ListBoxBrowseWndProc);
/xpinstall/wizard/windows/setup/dialogs.c, line 1335 --
return(CallWindowProc(OldListBoxWndProc, hWnd, uMsg, wParam, lParam));
/xpinstall/wizard/windows/setup/dialogs.c, line 1412 -- OldListBoxWndProc =
SubclassWindow(hwndLBComponents, (WNDPROC)NewListBoxWndProc);
/xpinstall/wizard/windows/setup/dialogs.c, line 1553 -- OldListBoxWndProc =
SubclassWindow(hwndLBComponents, (WNDPROC)NewListBoxWndProc);
}}

***

Sean, I believe there are 2 possible further cleanups:

<http://lxr.mozilla.org/mozilla/search?string=wasMinimized>
{{
wasMinimized
/xpinstall/wizard/windows/setup/dialogs.c, line 2681 -- BOOL wasMinimized = FALSE;
/xpinstall/wizard/windows/setup/dialogs.c, line 2709 -- wasMinimized = TRUE;
/xpinstall/wizard/windows/setup/dialogs.c, line 2713 -- if(wasMinimized)
/xpinstall/wizard/windows/setup/dialogs.c, line 2715 -- wasMinimized = FALSE;
}}
The current associated code seems useless !?

{{
2673   HWND      hSTMessage = GetDlgItem(hDlg, IDC_MESSAGE); /* handle to the
Static Text message window */
}}
The var is unused ! I don't know if the function call must be kept or not ?

Comment 30

[Hermann Schwab]

(In reply to comment #27)
> It doesn't have to have blocking status to get fixed

What does it need to get fixed?
Exact date of regression was found about half a year ago, bug was found about 10
weeks ago, and a fix made a week later, and r/sr done a week later.
Now this crasher bug fix is waiting for a month to get checked in, what is needed?
Does somebody have to apply for approval?

Comment 31

[Serge Gautherie (:sgautherie)]

(In reply to comment #30)
> (In reply to comment #27)
> > It doesn't have to have blocking status to get fixed
> 
> What does it need to get fixed?

(SeanS, DanielV)
I would guess taking into account my answer and questions in comment 29 would be
the next step.

> Does somebody have to apply for approval?

(NeilR)
Maybe: I'm not sure how frozen (MAS ou "Core") this file is ?

Comment 32

[Hermann Schwab]

(In reply to comment #31)
> > What does it need to get fixed?
> 
> (SeanS, DanielV)
> I would guess taking into account my answer and questions in comment 29 would be
> the next step.

This bug is a crasher bug, probability to be seen very low,
as it can be seen only at custom installation.

> > Does somebody have to apply for approval?
> 
> (NeilR)
> Maybe: I'm not sure how frozen (MAS ou "Core") this file is ?

You´ve been guessing in comment 29 that Firefox/Toolkit doesn´t need this, links
supplied.
You found some more nits to fix, but that won´t help the crash.
As there are now changes for seamonkey going on I´ve got some fear this patch
will bitrot, if not applied soon.

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #32)
> This bug is a crasher bug, probability to be seen very low,
> as it can be seen only at custom installation.

Sure. (but I don't understand why you wanted to write that in reply to my
previous comment ?)

> You found some more nits to fix, but that won´t help the crash.

Sure. (these nits can be included to do one checkin only, or be dealt with in a
second time as they are not mandatory at first.)

> As there are now changes for seamonkey going on I´ve got some fear this patch
> will bitrot, if not applied soon.

I'm only the reporter here: it seems to be between Sean and Daniel, and possibly
Neil...

Comment 34

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 175059 [details] [diff] [review]
patch v1.1
[Checked in: Comment 37]

Based on the comments, it looks like we do want to request approval for this.

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 175059 [details] [diff] [review]
patch v1.1
[Checked in: Comment 37]

a=asa

Comment 36

[Serge Gautherie (:sgautherie)]

Sean, anyone: Checkin wanted !

Comment 37

[timeless]

Comment on attachment 175059 [details] [diff] [review]
patch v1.1
[Checked in: Comment 37]

2005-05-05 09:08:
mozilla/xpinstall/wizard/windows/setup/dialogs.c	1.95

Comment 38

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050506] (nightly) (W98SE)

R.Fixed

Comment 39

[Serge Gautherie (:sgautherie)]

(In reply to comment #29)
> 
> Sean, I believe there are 2 possible further cleanups:
> 
> <http://lxr.mozilla.org/mozilla/search?string=wasMinimized>
> {{
> wasMinimized
> /xpinstall/wizard/windows/setup/dialogs.c, line 2681 -- BOOL wasMinimized = FALSE;
> /xpinstall/wizard/windows/setup/dialogs.c, line 2709 -- wasMinimized = TRUE;
> /xpinstall/wizard/windows/setup/dialogs.c, line 2713 -- if(wasMinimized)
> /xpinstall/wizard/windows/setup/dialogs.c, line 2715 -- wasMinimized = FALSE;
> }}
> The current associated code seems useless !?

This variable was added by
{{
1.92	ssu%netscape.com	2004-07-07 17:47	 	fixing bug 47762 - Installer switches
focus to itself repeatedly. r=leaf, sr=dveditz, a=asa
}}
but I wonder if/how it works.

Sean, anyone: Could you answer my question(s) ? Thanks.

Comment 40

[Andrew Schultz]

Serge: |wasMinimized| does have an effect:

http://lxr.mozilla.org/mozilla/source/xpinstall/wizard/windows/setup/dialogs.c#2730
2730           if(wasMinimized)
2731           {
2732             wasMinimized = FALSE;
2733             ResizeAndSetString(hDlg, lParam);
2734           }

Comment 41

[Serge Gautherie (:sgautherie)]

(In reply to comment #29)
> {{
> 2673   HWND      hSTMessage = GetDlgItem(hDlg, IDC_MESSAGE); /* handle to the
> Static Text message window */
> }}
> The var is unused ! I don't know if the function call must be kept or not ?

The answer is "useless line": issue moved to bug 47762 comment 31.

Comment 42

[Serge Gautherie (:sgautherie)]

(In reply to comment #40)
> Serge: |wasMinimized| does have an effect:

{{ (from emails between Andrew and me)
> But I'm puzzled by
>
http://lxr.mozilla.org/mozilla/source/xpinstall/wizard/windows/setup/dialogs.c#2698
> 2698   BOOL      wasMinimized = FALSE;

Oh, ok.  [...] wasMinimized would need to be declared static (line ~58) to
persist between calls to DlgProcMessage.  But I don't know if that was Sean's
intent or not.
}}
(Now we understand each other, but still miss Sean's insight :-/)

I'll move this issue to bug 47762 too.