243215 - gkwidget.dll reads control panel in bad way

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[Brant Gurganus]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 9.4; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 9.4; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

The application read from Current User Registry Key: ControlPanel. It should not
read this area of the registry directly for Control Panel information. Registry
keys change with each version of Windows. To guarantee compatibility, use the
correct API calls.

Thunderbird used QueryValue() to read HKEY_CURRENT_USER\Control Panel\Desktop.

Reproducible: Always
Steps to Reproduce:
1. Start Thunderbird as a subprocess of Microsoft's Application Verifier (free).

Actual Results:  
The issue mentioned.

Expected Results:  
The issue should be fixed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

See
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsLookAndFeel.cpp#300
and http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsLookAndFeel.cpp#338

What are the api calls for getting the information needed there?

Comment 2

[Brant Gurganus]

I am not a C programmer yet so I am not 100% sure on what information is being
retrieved, but here are some candidate replacements:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/system_information_functions.asp

From what I can make out of the C code, I think this is the best candidate:
SystemParametersInfo
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/systemparametersinfo.asp

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, that looks like it, indeed.

Comment 4

[[e-mail]]

Note that SPI_GETMENUSHOWDELAY is not defined by mingw or supported by Win95;
SPI_DRAGFULLWINDOWS is defined by mingw but requires Windows 95 Plus or later.

Comment 5

[Brodie]

If an item is not supported by the platform (it returns an error when getting 
it), then we should just use the default. i.e. SPI_DRAGFULLWINDOWS == true, 
SPI_GETMENUSHOWDELAY = 300ms (in existing code). 

I guess for mingw a bug should be filed against it in their bug system. I don't 
know much about mingw, how do you suggest it be worked around neil?

Comment 6

[[e-mail]]

Windows 95 does support menu show delay.
What it does not support is SPI_GETMENUSHOWDELAY.

Comment 7

[:Mook]

Attachment: Shot at patch (diff -w for review)

Notes:

- Unsure about legality of copying #defines (this one can be found either in
Microsoft PSDK or Wine)
- Can't find where eMetric_DragFullWindow is used in lxr (the only user - what
looks to be frame dragging code - is commented out) so that is untested.
- Old code left in for bulletproofing (uses the complete old codepath if
anything fails); should work for Win95.
- Not sure about coding convensions; sorry.

Comment 8

[:Mook]

Attachment: Appliable version of above (patch -u3); sorry for the bugspam.

Comment 9

[[e-mail]]

Comment on attachment 150025 [details] [diff] [review]
Shot at patch (diff -w for review)

Is there any mileage in changing the definitions of sSubmenuDelay and
sDragFullWindow so that we can pass their addresses to SystemParametersInfo?

Comment 10

[:Mook]

Comment on attachment 150025 [details] [diff] [review]
Shot at patch (diff -w for review)

Sorry, I just realized that asking somebody who doesn't run Windows to review
Windows only code isn't the brightest idea...

Neil: Since the whole thing is cached and run only once, that shouldn't matter
much right?  Besides, for sDragFullWindow, it just seems weird to let a BOOL
(even if typdefed to int) hold a third state (undefined) as well.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Mook, you want to request review from a specific person.. I recommend ere (ccing
him on the bug) or neil...

Comment 12

[:Mook]

Comment on attachment 150025 [details] [diff] [review]
Shot at patch (diff -w for review)

In that case, asking neil because he has already commented on the bug.	Thanks
for the heads up bz.

Comment 13

[[e-mail]]

Comment on attachment 150026 [details] [diff] [review]
Appliable version of above (patch -u3); sorry for the bugspam.

>-            if (result == ERROR_SUCCESS) {
>-              PRInt32 errorCode;
>-              nsString str; str.AssignWithConversion(value);
>-              sDragFullWindow = str.ToInteger(&errorCode);         
>+              if (result == ERROR_SUCCESS) {
>+                PRInt32 errorCode;
>+                nsString str; str.AssignWithConversion(value);
>+                sDragFullWindow = str.ToInteger(&errorCode);         
As you're reformatting this it would be nice if you could change this to
something simpler using strtoul...

Comment 14

[[e-mail]]

Comment on attachment 150026 [details] [diff] [review]
Appliable version of above (patch -u3); sorry for the bugspam.

>-          sSubmenuDelay = 300;
>+            sSubmenuDelay = 300;
Strange, it defaults to 400 on my system...

Comment 15

[:Mook]

Attachment: use strtoul() as per neil's comment #13

Only change was to replace the parsing of the strings to use strtoul() instead;
otherwise same as attachment 150026 [details] [diff] [review].

neil, re: comment #14:

It defaults to 400 here as well (XP); googling found WINE source that default
to 400 as well (with comment about NT 4 / 2000).  No idea where hyatt
originally got the 300 from (changed from 200 in rev. 1.19 appearently).

One question though: should there be a limit of some sort?  As far as I can
tell, the time is unsigned int in ms, so we can get a 49-day wait... Havn't
tested that case yet of course; but it should at least be no more broken than
before.

Comment 16

[[e-mail]]

Comment on attachment 150633 [details] [diff] [review]
use strtoul() as per neil's comment #13

>+                PRInt32 submenuDelay = strtoul(value, NULL, 0);
>+                if (submenuDelay != 0) {
>+                  sSubmenuDelay = submenuDelay;
>+                }
Thanks for looking at this. I did a bit of poking via TweakUI. It actually sets
the delay to 0 for Fast, and either 65534 or -2 for Slow, depending on the
platform. So not only isn't != 0 isn't a good test, it looks as if we should
use strtol, while sSubmenuDelay should actually be a WORD, or at least have its
upper bits masked off.

>+              if (result == ERROR_SUCCESS) {
>+                sDragFullWindow = strtoul(value, NULL, 0);
>+              }
This shouldn't be a problem. As it's a boolean, realistically it should only be
0 or 1 anyway.

Comment 17

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Updated last patch to comments.

Updated the patch from a while ago so it now works in the current builds.

I have two questions:
1. regarding the delay limit. Should we impose one (in the code I added one for 6.55 seconds, make this larder, smaller or maybe remove the check completely?)

2. Why is it not applicable to use the |submenuDelay != 0| check?

Comment 18

[[e-mail]]

(In reply to comment #17)
>I have two questions:
>1. regarding the delay limit. Should we impose one (in the code I added one for
>6.55 seconds, make this larder, smaller or maybe remove the check completely?)
In Windows 95 the delay limit is 65.535 seconds because the delay is stored in a WORD (confusingly it's actually written as an INT so Slow is -2), so I would limit the delay by masking it with 0xFFFF.

>2. Why is it not applicable to use the |submenuDelay != 0| check?
Because 0 is a valid submenuDelay. Only -1 is possibly invalid.

Comment 19

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #18)
> (In reply to comment #17)
> >I have two questions:
> >1. regarding the delay limit. Should we impose one (in the code I added one for
> >6.55 seconds, make this larder, smaller or maybe remove the check completely?)
> In Windows 95 the delay limit is 65.535 seconds because the delay is stored in
> a WORD (confusingly it's actually written as an INT so Slow is -2), so I would
> limit the delay by masking it with 0xFFFF.
> 

OK - I understand. So that would be check against -1. If not -1 allow the value to be stored and cancel off the upper bits with |submenuDelay & 0xFFFF| masking. Sound about right?

Also, thanks for the reply!

Comment 20

[Ryan Jones-Ward [:sciguyryan]]

Comment on attachment 241505 [details] [diff] [review]
Updated last patch to comments.

Marking obselete & removing review request.

Comment 21

[[e-mail]]

Comment on attachment 241505 [details] [diff] [review]
Updated last patch to comments.

>+                LONG submenuDelay = strtol(value, NULL, 0);
>+                // Enforce maximum delay  values. (6.55 seconds)
>+                if (submenuDelay > 6550) {
>+                  sSubmenuDelay = 6550;
>+                } else if (submenuDelay >= 0) {
>+                  sSubmenuDelay = submenuDelay;
>+                }
As you're masking the metric below I feel you only need to test for -1 here.
>=0 is definitely wrong on Windows 95 though, as -2 is used for 65534.

Comment 22

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #21)
> (From update of attachment 241505 [details] [diff] [review] [edit])
> >+                LONG submenuDelay = strtol(value, NULL, 0);
> >+                // Enforce maximum delay  values. (6.55 seconds)
> >+                if (submenuDelay > 6550) {
> >+                  sSubmenuDelay = 6550;
> >+                } else if (submenuDelay >= 0) {
> >+                  sSubmenuDelay = submenuDelay;
> >+                }
> As you're masking the metric below I feel you only need to test for -1 here.
> >=0 is definitely wrong on Windows 95 though, as -2 is used for 65534.
> 

Already got it fixed per your last comment. I also have one more question, do we need the |if (sDragFullWindow == -1)| and |if (sSubmenuDelay == -1)| checks because they will always return true anyway?

Comment 23

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Updated patch v2

Full update of the patch to Neil's comments.

* When |submenuDelay| isn't -1 its always valid. If its -1 then assume the Windows default 400.

Comment 24

[Ryan Jones-Ward [:sciguyryan]]

Attachment: patch v2.1

Comment 25

[[e-mail]]

(In reply to comment #22)
>I also have one more question, do we need the |if (sDragFullWindow == -1)| and
>|if (sSubmenuDelay == -1)| checks because they will always return true anyway?
These tests are because the variables in question are initialised to -1 as a sort of "unknown" value. The first time the metric is queried, we need to find the true value from the OS. Subsequently queries just return this saved value.

Comment 26

[[e-mail]]

Comment on attachment 241565 [details] [diff] [review]
patch v2.1

>           }
>-#endif
>-        }
>-        aMetric = sSubmenuDelay;
>         }
>+#endif
>+        // If -1 we fall back to 400 milliseconds,Windows default.
>+        aMetric = (sSubmenuDelay == -1) ? 400 : sSubmenuDelay;
So in other words you need to set sSubmenuDelay to 400 if it is still -1 just before the end of the if (sSubmenuDelay == -1) { block.

>-        aMetric = sDragFullWindow ? 1 : 0;
>-        }
>+        // If -1 we fall back to true Windows default.
>+        aMetric = (sDragFullWindow == -1) ? 1 : sDragFullWindow;
On the other hand I don't think you need this change at all.

Comment 27

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #26)
> (From update of attachment 241565 [details] [diff] [review] [edit])
> >           }
> >-#endif
> >-        }
> >-        aMetric = sSubmenuDelay;
> >         }
> >+#endif
> >+        // If -1 we fall back to 400 milliseconds,Windows default.
> >+        aMetric = (sSubmenuDelay == -1) ? 400 : sSubmenuDelay;
> So in other words you need to set sSubmenuDelay to 400 if it is still -1 just
> before the end of the if (sSubmenuDelay == -1) { block.

I actually did that for a good reason, because we have an |ifndef WINCE| block around the rest of the code for checking & setting the sSubmenuDelay variable. So if the |ifndef WINCE| doesn't evaluate too true then won't we get -1 passes as the end value which as you said is invalid. This is why I added the evaluation check right at the end to make sure -1 isn't passed from |ifndef WINCE| evaluation to false.

> 
> >-        aMetric = sDragFullWindow ? 1 : 0;
> >-        }
> >+        // If -1 we fall back to true Windows default.
> >+        aMetric = (sDragFullWindow == -1) ? 1 : sDragFullWindow;
> On the other hand I don't think you need this change at all.
> 

Thanks for the comment, I'll change that back.

Comment 28

[[e-mail]]

(In reply to comment #27)
>I actually did that for a good reason, because we have an |ifndef WINCE| block
>around the rest of the code for checking & setting the sSubmenuDelay variable.
In that case, unless Doug has any objections, I'd be tempted to write
#ifdef WINCE
aMetric = 400;
#else
// code as latest patch
#endif

Comment 29

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2.2

Firstly thanks for all the comments and help Neil!

This patch does everything discussed before and this time adds the early Neil's |#ifdef WINCE| suggestion for the quick 400 return.

Comment 30

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2.3

Fixed a stupid error on my part.

Comment 31

[[e-mail]]

Comment on attachment 241635 [details] [diff] [review]
Patch v2.3

>+        // 400ms is the current windows default.
>+          sSubmenuDelay = 400;
I would put this comment and assignment just inside the if (sSubmenuDelay == -1) block rather than the places they are now.

>+          	sSubmenuDelay = delay;
A tab crept on to this line. Please check the rest of your patch too!

Comment 32

[Ryan Jones-Ward [:sciguyryan]]

Attachment: No more tabs & wrong spaces

Spaces & tabs gone. Asking for a sr from the module owner.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There is no point in falling back to the registry if SystemParametersInfo fails. That should never happen on a version of Windows we actually support. Just remove the registry code and fall back to the default in that case.

Comment 34

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3

Updated to Robert's comments - removed the legacy code seeing as its apparently not needed.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You could simplify this a bit more. Write

static PRInt32 sSubmenuDelay = GetSystemParam(SPI_GETMENUSHOWDELAY, 400);

where GetSystemParam calls SystemParametersInfo and returns the provided default if that fails. You could reuse it for sDragFullWindow too.

Comment 36

[Ryan Jones-Ward [:sciguyryan]]

Comment on attachment 241648 [details] [diff] [review]
Patch v3

Will make this shorter still.

Comment 37

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v4

Shrunk down as far as possible.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually we're not done shrinking yet :-)

+PRInt32 GetSystemParam(long flag, PRInt32 def) {
+    BOOL succeeded;
+    DWORD value; 
+    succeeded = ::SystemParametersInfo(flag, 0, &value, 0);
+    return succeeded ? value : def;
+}

remove succeeded, just do "return ::SystemParametersInfo ..."

Also, make this function "static".

Also, you can move the #ifdef WINCE into GetSystemParam; just make it return "def" always instead of calling SystemParametersInfo (btw your patch would have broken WINCE builds I think, because it would have required SystemParametersInfo on WINCE)

+        aMetric = GetSystemParam(SPI_GETMENUSHOWDELAY, 400);
+#else
+        aMetric = 400;
 #endif
         }

You can properly indent this code.

Comment 39

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v4.1

Patch to Robert O'Callahan's comments.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 241756 [details] [diff] [review]
Patch v4.1

yay! perfect :-)

Comment 41

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/windows/nsLookAndFeel.cpp 	1.56

Comment 42

[jpl24]

+static PRInt32 GetSystemParam(long flag, PRInt32 def) {
+#ifndef WINCE
+    return def;
+#else
+    DWORD value; 
+    return SystemParametersInfo(flag, 0, &value, 0) ? value : def;
+#endif
+}

Shouldn't this be #ifdef instead of #ifndef, as SystemParametersInfo is not defined on WINCE?

Comment 43

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #42)
> Shouldn't this be #ifdef instead of #ifndef, as SystemParametersInfo is not
> defined on WINCE?

Good catch, I checked in that fix.