684506 - Nightly is using old file manager window

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Peter Henkel [:Terepin]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110903 Firefox/9.0a1
Build ID: 20110903030832

Steps to reproduce:

Tried to download/upload a file.


Actual results:

Old file manager window appeared.


Expected results:

Current file manage window should opened, just like before.

Comment 1

[Logan Rosen [:Logan]]

Confirming on Windows 7 64-bit with the new 20110903 build.

Comment 2

[Alice0775 White]

Regression wiondow(cached m-c hourly),
Works:
http://hg.mozilla.org/mozilla-central/rev/7d3d1c2c75f8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110901 Firefox/9.0a1 ID:20110901004809
Fails:
http://hg.mozilla.org/mozilla-central/rev/ce43a8644bc0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110902 Firefox/9.0a1 ID:20110902030838
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7d3d1c2c75f8&tochange=ce43a8644bc0

Comment 3

[Rumos Mok]

Windows 7 32-bit
Nightly 20110903
Changeset: 76462:472716252ea3

Confirmed. Old-style file manager windows is shown.

Comment 5

[Brian R. Bondy [:bbondy]]

As soon as you specify a hook via OFN_ENABLEHOOK this happens to the look.

Apparently Windows will not allow you to use a hook procedure via OFN_ENABLEHOOK with the new look.  This is because a lot of legacy hook code actually reorganized the file dialog's content and the new look would have broken.  Why they didn't just add another flag I'm not sure. 

We can simply NOT do a hook in Vista and up since the problem in Bug 295540 is only on XP anyway.  This would solve the problem.   

I just found out that this problem has actually been in Nightly for a while now but only in multi file selections because of Bug Bug 660833. I guess we can't get that fix for Vista and up until we move the new common file dialogs code.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Fixed Windows silently using old dialogs because of hook procedure.

As mentioned in Comment 5:
- This patch will have no side effects for the recent Bug 295540.
- This patch will cause the fix in Bug 660833 to only work in XP and 2000, but if we want the new looking dialogs we have no choice.  It will be fixed again once we start using the Common File Dialogs for Vista/7. 

I think it's better to have the search bar and new nav bar than to have the fix in Bug 660833 for multi selections.

Comment 8

[remixedcat]

how did this even happen? it was fine before. What change really motivated this?

Comment 9

[Brian R. Bondy [:bbondy]]

As per Comment 5 there was a fix for Windows XP which required us to pass an extra flag.  Windows Vista and 7 don't handle this flag properly in the new dialogs so it was falling back to the old dialog silently.  It was not noticed during development because the bug that introduced the flag was tested and done for Windows XP which is the same as the file picker dialog that you see now.

Comment 10

[ｘｘｘ]

Attachment: Description

Comment 11

[Peter Henkel [:Terepin]]

Serious question: How did this get uploaded?

Comment 12

[remixedcat]

(In reply to Peter Henkel [:Terepin] from comment #11)
> Serious question: How did this get uploaded?

my points exactly Terepin!!!

Comment 13

[Brian R. Bondy [:bbondy]]

Hi Peter & remixedcat.  

The answer was given in Comment 9 and Comment 5. 

Nightly builds are a wild place, they may or may not work.  
Nightly builds are the place where testing happens, so regressions are common and expected.

Firefox Aurora or Firefox Beta will have much less regressions and still allow you to try early features and fixes.  
They will give you the more stable experience that you are expecting.

Thanks to your report thought the regression was avoided for future builds within 4 hours of being reported.  Once the patch has been reviewed it will be pushed to Nightly.

Thanks again for your support.

Comment 14

[Peter Henkel [:Terepin]]

(In reply to Brian R. Bondy [:bbondy] from comment #13)
> Hi Peter & remixedcat.  
> 
> The answer was given in Comment 9 and Comment 5. 
> 
> Nightly builds are a wild place, they may or may not work.  
> Nightly builds are the place where testing happens, so regressions are
> common and expected.
> 
> Firefox Aurora or Firefox Beta will have much less regressions and still
> allow you to try early features and fixes.  
> They will give you the more stable experience that you are expecting.
> 
> Thanks to your report thought the regression was avoided for future builds
> within 4 hours of being reported.  Once the patch has been reviewed it will
> be pushed to Nightly.
> 
> Thanks again for your support.

I was talking about malware.

Comment 15

[Brian R. Bondy [:bbondy]]

Ah, in that case Comment 13 minus Peter :)

Comment 18

[Brian R. Bondy [:bbondy]]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4325bfb17f6f

Comment 19

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2b84c0ca82b6

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/2b84c0ca82b6

Comment 21

[Jim Jeffery not reading bug-mail 1/2/11]

This patch appears to still have major issues with the DLM. 

STR:
1.  Install a build with this patch 
2.  Open the Download Manager (ctrl+j) or via Menu 
3.  Right-click any file in the DM and select 'Open Folder Location' , browser crashes

There is no crash when using the old style manager, but its the wrong manager for Vista/Win7.  Why are we messing with 'custom' DM and not using the Native API.  Its worked for yrs until someone decided to 'fix' an ancient bug that no one was complaining about to beging with, so why not back out the whole thing and WONTFIX bug 295540 ?  For those with dual monitors, I rather imagine at this late date they have learned to just 'work-around' the problem as reported.

I don't have crash-reports as I'm testing with hourly builds.

Win7 64bit using m-c 32bit builds.

Regression range:
20110908135749 817c2b9dc11d crashes 
20110908132721 a73f7a3b85a5 Works, no crash when opening folder from DM

Comment 22

[Jim Jeffery not reading bug-mail 1/2/11]

[quote]There is no crash when using the old style manager, but its the wrong manager for Vista/Win7.  Why are we messing with 'custom' DM and not using the Native API[/quote]

Wrong terminology,  should have said 'File picker'...

Comment 23

[Alice0775 White]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #21)
> This patch appears to still have major issues with the DLM. 
> 
> STR:
> 1.  Install a build with this patch 
> 2.  Open the Download Manager (ctrl+j) or via Menu 
> 3.  Right-click any file in the DM and select 'Open Folder Location' ,
> browser crashes
> 
> There is no crash when using the old style manager, but its the wrong
> manager for Vista/Win7.  Why are we messing with 'custom' DM and not using
> the Native API.  Its worked for yrs until someone decided to 'fix' an
> ancient bug that no one was complaining about to beging with, so why not
> back out the whole thing and WONTFIX bug 295540 ?  For those with dual
> monitors, I rather imagine at this late date they have learned to just
> 'work-around' the problem as reported.
> 
> I don't have crash-reports as I'm testing with hourly builds.
> 
> Win7 64bit using m-c 32bit builds.
> 
> Regression range:
> 20110908135749 817c2b9dc11d crashes 
> 20110908132721 a73f7a3b85a5 Works, no crash when opening folder from DM

This crash is triggered by
0907fce1e6fc	Brian R. Bondy — Bug 670068 - Open Containing Folder spawns multiple instances of explorer.exe. r=jimm

Comment 24

[Jim Jeffery not reading bug-mail 1/2/11]

Thanks Alice0775..

Comment 25

[Jim Mathies [:jimm]]

I haven't received a nightly update yet to test. Can someone who can reproduce this file a blocking on bug 670068 and post a crash signature plus the STR?

Comment 26

[Ekanan Ketunuti]

(In reply to Jim Mathies [:jimm] from comment #25)
> I haven't received a nightly update yet to test. Can someone who can
> reproduce this file a blocking on bug 670068 and post a crash signature plus
> the STR?

stack bp-e8f8e37f-9a3c-4e61-aed0-a3c592110909

STR: follow comment 21

Comment 27

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Jim Mathies [:jimm] from comment #25)
> I haven't received a nightly update yet to test. Can someone who can
> reproduce this file a blocking on bug 670068 and post a crash signature plus
> the STR?

https://crash-stats.mozilla.com/report/index/bp-54b73685-d4a7-49b4-8caf-e3d362110909

ref comment 26, that must be an hourly build your using, hourlys do not contain symbols to give an accurate stack-dump.

STR in comment 21

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=685847

Comment 28

[Brian R. Bondy [:bbondy]]

> There is no crash when using the old style manager, but its the wrong manager for Vista/Win7.

I'm using Win7 and it seems like the correct file picker now on the Nightly build.

For the crash which I can reproduce with the nightly build, unrelated to this ticket, I will handle in Bug 670068

Comment 29

[Brian R. Bondy [:bbondy]]

> I will handle in Bug 670068

... in Bug 685847 since it was just posted.

Comment 30

[remixedcat]

Now getting proper file picker here as well. Nice.

Comment 32

[rservus]

Hi Brian, I've reported Bug 692469 and notified that my bug is duplicate of current one. Can you tell me when firefox stable version will get fixed (I mean ff7). It is very important, because we need multiple file upload dialog with win7 features (search, image preview, remember last state, etc). 

I'm a bit disappointed when see "target milestone: mozilla9" for this bug resolution. Does it means we have to wait till version 9 is out?!!

Comment 33

[Brian R. Bondy [:bbondy]]

Yes it will be in version 9, which is a couple months away in December.

Comment 35

[Mark Banner (:standard8)]

(In reply to Brian R. Bondy [:bbondy] from comment #33)
> Yes it will be in version 9, which is a couple months away in December.

Given this is a regression in 7, and the patch (afaict) is just adding some windows version checks, couldn't this be pushed forward to be fixed in version 8? Or do you see potential issues with it that need more testing?

Comment 36

[Brian R. Bondy [:bbondy]]

Attachment: Request for reverting 2 changesets in Firefox Beta

Requesting beta+
 
This bug is a problem with the multi file picker using the older Windows file picker.
The problem is a regression that was introduced by the fix in bug 660833.
The problem was introduced and not noticed because Windows silently uses it whenever you use a hook on the filepicker dialog.

The problem is already fixed in Aurora and Nightly.
The problem is currently present in FF7 and Beta.
The regression first hit FF7 and was not in FF6.

Risk of introducing further regressions: None since we would be just backing out the most recent 2 changes in the file picker code of FF7.

2 changesets:
http://hg.mozilla.org/releases/mozilla-beta/rev/08a813a47709
http://hg.mozilla.org/releases/mozilla-beta/rev/84533f5e86bf

NOTE:****DO NOT use the patch as the code has changed too much to use it, instead backout the 2 changesets mentioned above.****

Comment 39

[Mark Banner (:standard8)]

Christian did the backouts just before b4:

http://hg.mozilla.org/releases/mozilla-beta/rev/e3fb31311b62
http://hg.mozilla.org/releases/mozilla-beta/rev/75616c2e1ddf

Comment 40

[Mark Banner (:standard8)]

Onno, I'm resetting the flags to fixed for 8 as I assume you accidentally re-set them.

Comment 41

[Onno Ekker [:nONoNonO UTC+1]]

Yes, thanks Mark. Sorry for doing so.