Last Comment Bug 684506 - Nightly is using old file manager window
: Nightly is using old file manager window
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: All Windows 7
: -- normal with 2 votes (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
:
Mentors:
: 684524 684556 684756 684795 689732 693038 694757 694853 (view as bug list)
Depends on:
Blocks: 295540
  Show dependency treegraph
 
Reported: 2011-09-03 08:37 PDT by Peter Henkel [:Terepin]
Modified: 2011-10-28 03:16 PDT (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Fixed Windows silently using old dialogs because of hook procedure. (3.55 KB, patch)
2011-09-03 13:10 PDT, Brian R. Bondy [:bbondy]
jmathies: review+
Details | Diff | Splinter Review
Description (896.00 KB, text/plain)
2011-09-05 09:59 PDT, xxx
no flags Details
Request for reverting 2 changesets in Firefox Beta (45 bytes, text/plain)
2011-10-10 16:00 PDT, Brian R. Bondy [:bbondy]
christian: approval‑mozilla‑beta+
Details

Description Peter Henkel [:Terepin] 2011-09-03 08:37:03 PDT
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] 2011-09-03 08:50:14 PDT
Confirming on Windows 7 64-bit with the new 20110903 build.
Comment 2 Alice0775 White 2011-09-03 09:04:57 PDT
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 2011-09-03 10:27:14 PDT
Windows 7 32-bit
Nightly 20110903
Changeset: 76462:472716252ea3

Confirmed. Old-style file manager windows is shown.
Comment 4 Alice0775 White 2011-09-03 12:04:33 PDT
*** Bug 684524 has been marked as a duplicate of this bug. ***
Comment 5 Brian R. Bondy [:bbondy] 2011-09-03 12:24:16 PDT
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] 2011-09-03 13:10:30 PDT
Created attachment 558094 [details] [diff] [review]
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 7 Alice0775 White 2011-09-03 21:50:49 PDT
*** Bug 684556 has been marked as a duplicate of this bug. ***
Comment 8 remixedcat 2011-09-03 22:03:41 PDT
how did this even happen? it was fine before. What change really motivated this?
Comment 9 Brian R. Bondy [:bbondy] 2011-09-03 22:07:11 PDT
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 xxx 2011-09-05 09:59:58 PDT
Created attachment 558304 [details]
Description
Comment 11 Peter Henkel [:Terepin] 2011-09-05 10:03:48 PDT
Serious question: How did this get uploaded?
Comment 12 remixedcat 2011-09-05 10:18:04 PDT
(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] 2011-09-05 11:08:08 PDT
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] 2011-09-05 11:12:59 PDT
(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] 2011-09-05 11:15:41 PDT
Ah, in that case Comment 13 minus Peter :)
Comment 16 (mostly gone) XtC4UaLL [:xtc4uall] 2011-09-05 16:18:00 PDT
*** Bug 684756 has been marked as a duplicate of this bug. ***
Comment 17 Tim (fmdeveloper) 2011-09-05 22:14:30 PDT
*** Bug 684795 has been marked as a duplicate of this bug. ***
Comment 18 Brian R. Bondy [:bbondy] 2011-09-07 09:39:56 PDT
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4325bfb17f6f
Comment 19 Brian R. Bondy [:bbondy] 2011-09-08 08:32:45 PDT
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2b84c0ca82b6
Comment 21 Jim Jeffery not reading bug-mail 1/2/11 2011-09-09 03:28:23 PDT
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 2011-09-09 03:32:40 PDT
[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 2011-09-09 03:48:58 PDT
(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 2011-09-09 03:57:27 PDT
Thanks Alice0775..
Comment 25 Jim Mathies [:jimm] 2011-09-09 05:21:23 PDT
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 2011-09-09 05:38:57 PDT
(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 2011-09-09 06:22:31 PDT
(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] 2011-09-09 06:23:00 PDT
> 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] 2011-09-09 06:25:50 PDT
> I will handle in Bug 670068

... in Bug 685847 since it was just posted.
Comment 30 remixedcat 2011-09-09 07:02:05 PDT
Now getting proper file picker here as well. Nice.
Comment 31 Brian R. Bondy [:bbondy] 2011-10-04 07:20:52 PDT
*** Bug 689732 has been marked as a duplicate of this bug. ***
Comment 32 rservus 2011-10-07 02:08:19 PDT
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] 2011-10-07 06:38:43 PDT
Yes it will be in version 9, which is a couple months away in December.
Comment 34 Matthias Versen [:Matti] 2011-10-08 06:36:59 PDT
*** Bug 693038 has been marked as a duplicate of this bug. ***
Comment 35 Mark Banner (:standard8) 2011-10-10 06:51:29 PDT
(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] 2011-10-10 16:00:46 PDT
Created attachment 566068 [details]
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 37 Matthias Versen [:Matti] 2011-10-15 05:47:03 PDT
*** Bug 694757 has been marked as a duplicate of this bug. ***
Comment 38 Matthias Versen [:Matti] 2011-10-16 07:58:35 PDT
*** Bug 694853 has been marked as a duplicate of this bug. ***
Comment 39 Mark Banner (:standard8) 2011-10-25 03:50:35 PDT
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) 2011-10-28 03:03:57 PDT
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] 2011-10-28 03:16:05 PDT
Yes, thanks Mark. Sorry for doing so.

Note You need to log in before you can comment on or make changes to this bug.