422784 - reduce narrow windows API calls in plugins

Status: RESOLVED DUPLICATE of bug 479104

(Core :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: directy from "other" patch on bug 418703

Comment 1

[Brian Crowder]

jst:  Review-ping?

Comment 2

[Bernard Alleysson]

Please see:
http://msdn.microsoft.com/en-us/library/ms647486(VS.85).aspx

LoadString "nBufferSize" is in TCHAR unit.

For instance, in the patch:

+  wchar_t szString[512];
+  LoadStringW(hInst, IDS_TITLE, szString, sizeof(szString));

sizeof(szString)/sizeof(szString[0]) should be used, or please use NS_ARRAY_LENGTH.

All use of LoadStringW should be carefully reviewed, as there are several of them in the patch.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 309231 [details] [diff] [review]
directy from "other" patch on bug 418703

Sorry for not getting to this sooner :(

- In TryToUseNPRuntimeJavaPlugIn():

+  if (ERROR_SUCCESS != ::RegOpenKeyExW(HKEY_LOCAL_MACHINE,
                                       keyName, 0, KEY_READ, &javaKey)) {

Fix next line indentation.

- Further down, same file:

-      char current_version[80];
+      wchar_t current_version[80];
       DWORD length = sizeof(current_version);

That needs to be sizeof(current_version)/sizeof(current_version[0]).

         DWORD pathlen = sizeof(path);

Same thing.

-        result = ::RegQueryValueEx(keyloc, "Plugins Directory", NULL, &type,
+        result = ::RegQueryValueExW(keyloc, L"Plugins Directory", NULL, &type,
                                    (LPBYTE)&path, &pathlen);

Fix next line indentation.

-    LONG result = ::RegOpenKeyEx(HKEY_LOCAL_MACHINE, curKey, 0, KEY_READ,
+    LONG result = ::RegOpenKeyExW(HKEY_LOCAL_MACHINE, curKey, 0, KEY_READ,
                                  &baseloc);

fix indentation.

     // Look for "BrowserJavaVersion"
-    if (ERROR_SUCCESS != ::RegQueryValueEx(baseloc, "BrowserJavaVersion", NULL,
+    if (ERROR_SUCCESS != ::RegQueryValueExW(baseloc, L"BrowserJavaVersion", NULL,
                                            NULL, (LPBYTE)&browserJavaVersion,
                                            &numChars))
here too.

       pathlen = sizeof(path);

sizeof(path)/sizeof(path[0]).

-      result = ::RegEnumKeyEx(baseloc, index, curKey, &numChars, NULL, NULL,
+      result = ::RegEnumKeyExW(baseloc, index, curKey, &numChars, NULL, NULL,
                               NULL, &modTime);

Fix indentation...

This patch is full of sizeof() issues, please go through all the places where you change the type of string variables and make sure the size isn't assumed to be the length. And while you're at it, fix as many indentation problems as you run into as well.

Once done, I can r+sr, but until then, r-

Comment 4

[Brian Crowder]

Attachment: updated revision

Comment 5

[Brian Crowder]

(In reply to comment #3)
> (From update of attachment 309231 [details] [diff] [review])
> Fix next line indentation.

I believe I have repaired all the indentation in this patch, please double-check my work.

> - Further down, same file:
> 
> -      char current_version[80];
> +      wchar_t current_version[80];
>        DWORD length = sizeof(current_version);
> 
> That needs to be sizeof(current_version)/sizeof(current_version[0]).

Actually, RegQueryValueEx's 6th parameter, according to MSDN, is "A pointer to a variable that specifies the size of the buffer pointed to by the lpData parameter, in bytes.", so this use of sizeof is correct.  I have fixed others, however.  Again, please double-check my work.

Comment 6

[Brian Crowder]

Comment on attachment 337898 [details] [diff] [review]
updated revision

I also removed a lot of meaningless twiddling from the previous patch (changes from lstrcmp to strcmp and back, and so on), so this patch should be markedly smaller and easier to review.

Comment 7

[Brian Crowder]

Bugzilla interdiff will probably fail you; the old patch was badly bitrotted.  I don't have a good way of giving you a better interdiff myself, either, I'm afraid.  I had to hand-rewrite a lot of this, because patch was barfing on the old patch, even with fuzz.

Comment 8

[Bernard Alleysson]

about the patch "update revision":

+    if(RegSetValueExW(hkey, wmime, 0,  REG_SZ, (LPBYTE) L"(none)", 7) != ERROR_SUCCESS)

Replace "7" by "7 * sizeof(WCHAR)".

Replace "MessageBox(0, "Error adding MIME type value", "Default Plugin", MB_OK);"
with "MessageBoxW(0, L"Error adding MIME type value", L"Default Plugin", MB_OK);" ?

Comment 9

[Bernard Alleysson]

>   DWORD dwType, keysize = 512;

Should be:

   DWORD dwType, keysize = 512 * sizeof(WCHAR);

Comment 10

[Brian Crowder]

Comment on attachment 337898 [details] [diff] [review]
updated revision

I'll post another patch with Bernard's finds fixed shortly.

Comment 11

[Brian Crowder]

Attachment: v3

Thanks, Bernard.

Comment 12

[Brian Crowder]

Attachment: v4

Woops, with comment #9 in, this time.

Comment 13

[Bernard Alleysson]

1)
>     DWORD numChars = _MAX_PATH;

Should be DWORD numChars = _MAX_PATH * sizeof(WCHAR);
It is used here
+    if (ERROR_SUCCESS != ::RegQueryValueExW(baseloc, L"BrowserJavaVersion", NULL,
+                                            NULL, (LPBYTE)&browserJavaVersion,
+                                            &numChars))

There are other declarations "DWORD numchars = _MAX_PATH" but some doesn't seem to be used ?

2) You changed only one occurrence of "MessageBox" to "MessageBoxW", there are other occurrences.

That's all I could find, I went through the patch twice.

Comment 14

[Brian Crowder]

Comment on attachment 337918 [details] [diff] [review]
v4

Thanks, one more spin coming up.

Comment 15

[Brian Crowder]

Attachment: v5

Thanks again, Bernard.

Comment 16

[Brian Crowder]

The v4 -> v5 interdiff works; I moved the numChars decls closer to where they're used, introduced a numBytes variable for this query, and then fixed the other MessageBox invocation.

Comment 17

[Brian Crowder]

jst:  Review-ping?

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

jst: review ping?

Comment 19

[Brian Crowder]

Actually, vlad posted a patch in another bug which jst is going to review, instead I think.  We should probably resolve this one INCOMPLETE?