Convert windows nsOSHelperAppService to use nsIWindowsRegKey

RESOLVED FIXED

Status

Core Graveyard
File Handling
RESOLVED FIXED
13 years ago
a year ago

People

(Reporter: Biesinger, Assigned: Jungshik Shin)

Tracking

Trunk
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

Currently, nsOSHelperAppService contains various codepaths that use wide/ansi
registry functions depending on windows version. the code could be simplified by
using http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsIWindowsRegKey.idl#42.
(Assignee)

Comment 1

13 years ago
I thought I han done this, but cbie did it. Because I'm pretty sure I wrote a
similar wide/narrow API switching for Windows registry handling (perhaps in
mailnews),  there must be  other parts that need to be changed to take advantage
of  nsIWindowsRegKey...

Assignee: file-handling → jshin1987

Comment 2

13 years ago
There are definitely quite a few hits for RegQueryValueEx in LXR.
(Assignee)

Comment 3

13 years ago
Yeah, some of them are listed in bug 250255. I've just made a patch for this bug
and am building ff for testing.

Status: NEW → ASSIGNED
(Assignee)

Comment 4

13 years ago
Created attachment 184661 [details] [diff] [review]
patch

this patch may be too difficult to read. If so, let me know and I'll upload a
patch made with '-u7 -w -p', but it seems like it's no easier to read than
this. Perhaps, the whole patched file would be easier. Btw, I kept most of
error handlings of the original code which are a little inconsistent.
(Assignee)

Updated

13 years ago
Attachment #184661 - Flags: superreview?(darin)
Attachment #184661 - Flags: review?(cbiesinger)

Comment 5

13 years ago
Comment on attachment 184661 [details] [diff] [review]
patch

>Index: uriloader/exthandler/win/nsOSHelperAppService.cpp

>+PRBool nsOSHelperAppService::sIsNT = PR_FALSE;
> 
> nsOSHelperAppService::nsOSHelperAppService() : nsExternalHelperAppService()
> {
>   OSVERSIONINFO osversion;
>   ::ZeroMemory(&osversion, sizeof(OSVERSIONINFO));
>   osversion.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> 
>   if (!GetVersionEx(&osversion)) {
>     // If the call failed, better be safe and assume *W functions don't work
>+    sIsNT = PR_FALSE;
>   }
>   else {
>+    sIsNT = (osversion.dwPlatformId == VER_PLATFORM_WIN32_NT);
>   }

Now that sIsNT is a static member, shouldn't we only initialize it
once?  Or, perhaps this is fine since nsOSHelperAppService is a 
singleton.  How about a comment in the code to that effect?


>+  nsCAutoString mimeDatabaseKey ("MIME\\Database\\Content Type\\");

nit: maybe use .AssignLiteral, and maybe build this value using nsAutoString
to avoid conversion below.


>+  regKey->Close();

the regKey closes automatically when it goes out of scope.  this method
call is not really needed here.


>   nsCAutoString cmdString(aScheme);
>   cmdString.AppendLiteral("\\shell\\open\\command");
>+  nsresult rv = regKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
>+                             NS_ConvertASCIItoUTF16(cmdString),

might as well start with nsAutoString here.


>+  rv = regKey->ReadStringValue(EmptyString(), _retval); 
>+  regKey->Close();

again, no need for explicit call to nsIWindowsRegKey::close.  same
thing elsewhere in this file.


>+  nsCAutoString type;
>+  rv = regKey->ReadBinaryValue(NS_LITERAL_STRING("Content Type"), type);

This seems wrong to me.  "Content Type" is of type REG_SZ, so you need
to use ReadStringValue (right?) or else you may get a result that is
UTF-16 encoded here, yet stored in a nsCAutoString.

I think you should avoid ReadBinaryValue on values that are of type REG_SZ.


>+    if (lastCommaPos != kNotFound)
>+      handlerFilePath = Substring(handlerCommand, rundllSegmentLength, 
>+                                  lastCommaPos - rundllSegmentLength);
>+    else
>+      handlerFilePath = Substring(handlerCommand, rundllSegmentLength, 
>+                                  handlerCommand.Length() - rundllSegmentLength);

nit: these two cases are identical except for the third parameter.
sometimes it is better to store that third parameter as a temporary
variable, and then call Substring only once.  it helps reduce codesize.


>+    DWORD required = ::ExpandEnvironmentStringsW(handlerFilePath.get(),
>+                                                 NULL, 0);
>+    nsAutoArrayPtr<WCHAR> destination(new WCHAR[required]);

this works.  you could also use:

  nsAutoString temp;
  temp.SetLength(required);
  if (temp.Length() != required)
    return NS_ERROR_OUT_OF_MEMORY;
  ::ExpandEnvironmentStringsW(handlerFilePath.get(), temp.BeginWriting(),
			      required);
  handlerFilePath = temp;

only advantage here is that you might be able to share the buffer
allocated by the SetLength call when assigning into handlerFilePath.

same comment applies to the ANSI version.


>+  nsILocalFileWin* lfw = nsnull;
>+  rv = CallQueryInterface(lf, &lfw);
>+  if (NS_SUCCEEDED(rv) && lfw) {

since you don't do anything with a failed rv, you might as well just write
this like so:

  nsILocalFileWin* lfw = nsnull;
  CallQueryInterface(lf, &lfw);
  if (lfw) {
    ...


>+    if (NS_FAILED(regKey->ReadBinaryValue(NS_LITERAL_STRING("Content Type"), 
>+                  typeToUse))) {

use ReadStringValue.
Attachment #184661 - Flags: superreview?(darin) → superreview-
Comment on attachment 184661 [details] [diff] [review]
patch

+  mimeDatabaseKey += aMimeType;


wonder if some of the arguments in this file should be changed to
nsAString/nsACString

+  if (pos > 0) // we have a comma separated list of languages...
+    // truncate everything after the first comma (including the comma)
+    aFileExtension.Truncate(pos); 

hm, I generally prefer {} for multiline if bodies, even if they are a single
statement

also, I know you just copied this, but I think s/languages/types/ would be more
accurate :-)

+  if ( NS_FAILED(rv) )

can you use the standard style here? (if (NS_FAILED(rv)))
(several times)

also what darin said


+    DWORD required = ::ExpandEnvironmentStringsW(handlerFilePath.get(),
+						  NULL, 0);
+    nsAutoArrayPtr<WCHAR> destination(new WCHAR[required]);

this seems wrong, quoting the ExpandEnvironmentStrings doc:
"f the destination buffer is too small to hold the expanded string, the return
value is the required buffer size, in characters."

in other words, seems like you need to add 1, for the terminating null
character.

Additionally:
"nSize
    [in] Maximum number of characters that can be stored in the buffer pointed
to by the lpDst parameter. When using ANSI strings, the buffer size should be
the string length, plus terminating null character, plus one. When using
Unicode strings, the buffer size should be the string length plus the
terminating null character."

So in the ANSI case you seem to need to add 2, not 1.

(although I notice the original code got this wrong as well, sigh)

+  if (!mimeInfo)
+    // we failed to find a mime type.
+    return nsnull;

that comment seems wrong? isn't the if hit only in case of OOM?

+  (void) regKey->Close();

why the (void)? no other place in this file uses it.
Attachment #184661 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 7

13 years ago
Created attachment 185967 [details] [diff] [review]
patch update 

all the review comments are addressed.
Attachment #184661 - Attachment is obsolete: true
Attachment #185967 - Flags: superreview?(darin)
Attachment #185967 - Flags: review?(cbiesinger)
Comment on attachment 185967 [details] [diff] [review]
patch update 

    ::ExpandEnvironmentStringsW(handlerFilePath.get(), dest.BeginWriting(),
				required);


Hm, my reading of MSDN is that you should pass the amount of storable
characters here (i.e. excluding the null byte)... although, I may be wrong, I
think the doc is a bit ambiguous here...


You are misusing the string API here, too. the SetLength argument is the amount
of characters, excluding the null byte; and having .Length() be larger than the
actual stored string is asking for trouble...

(similar in the ansi case)
Attachment #185967 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 9

13 years ago
(In reply to comment #8)
> (From update of attachment 185967 [details] [diff] [review] [edit])
>     ::ExpandEnvironmentStringsW(handlerFilePath.get(), dest.BeginWriting(),
> 				required);
> 
> Hm, my reading of MSDN is that you should pass the amount of storable
> characters here (i.e. excluding the null byte)... although, I may be wrong, I
> think the doc is a bit ambiguous here...

It's indeed ambiguous, but the example code at 
http://msdn.microsoft.com/library/en-us/sysinfo/base/getting_system_information.asp

seems to indicate the opposite of your reading although it's still unclear.
However, we don't have to worry about one/two-off error here because the value
returned by the first call should give us the correct buffer size required for
the second call (see 'Return value' section of the MSDN on the API). That is, my
first patch and the original code currently in the tree do the right thing (TM)
except that instead of passing 'NULL/nsnull', a zero-length string should be
passed (well, |Expand...String| seems to work fine with NULL, but the MSDN
doesn't say so explicitly and we'd better be on the safe side)  

> You are misusing the string API here, too. the SetLength argument is the amount
> of characters, excluding the null byte; and having .Length() be larger than the
> actual stored string is asking for trouble...

 Hmm, really?  Well, I'll just go back to nsAutoArrayBuffer to steer clear of
any potential trouble at the expense of a slight perf. loss. 

Comment 10

13 years ago
> > You are misusing the string API here, too. the SetLength argument is the 
> > amount of characters, excluding the null byte; and having .Length() be
> > larger than the actual stored string is asking for trouble...
> 
>  Hmm, really?  Well, I'll just go back to nsAutoArrayBuffer to steer clear of
> any potential trouble at the expense of a slight perf. loss. 

Hrm... it sounds like we should just call SetLength(len - 1) to account for the
null byte correctly.  Then, everything should be fine.  The string API is
designed to allow you to resize the string buffer and write to it.
(Assignee)

Comment 11

13 years ago
(In reply to comment #10)

> >  Hmm, really?  Well, I'll just go back to nsAutoArrayBuffer to steer clear of
> > any potential trouble at the expense of a slight perf. loss. 
> 
> Hrm... it sounds like we should just call SetLength(len - 1) to account for the
> null byte correctly.  Then, everything should be fine.  The string API is
> designed to allow you to resize the string buffer and write to it.

For 'wide' case, I'll do that because you seem to mind perf. loss, but for ANSI
case where I have little idea what Windows does with an extra byte in addition
to the terminating null, I'd rather use nsAutoArrayPtr to be safe. Btw, I
confirmed that 'required' returned by the first call to ::Expand..Strings is the
correct buffer size required for the second call (that is, |strlen + 1| for 'W'
API).  
(Assignee)

Updated

13 years ago
Attachment #185967 - Flags: superreview?(darin)
(Assignee)

Comment 12

13 years ago
Created attachment 186042 [details] [diff] [review]
patch update (with the correct buffer size)
Attachment #186042 - Flags: superreview?(darin)
Attachment #186042 - Flags: review?(cbiesinger)

Comment 13

13 years ago
Aren't ANSI strings supposed to have a single null byte?  Whereas wide strings
are supposed to have two null bytes?  I don't get why SetLength(len - 1)
couldn't be used in the ANSI and wide cases, but at any rate... it would
probably be better to use the same coding approach in both cases.  So, either
nsAutoArrayPtr for both or SetLength + BeginWriting for both.
darin: well,
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/expandenvironmentstrings.asp
explicitly says:
"When using ANSI strings, the buffer size should be the string length, plus
terminating null character, plus one."
Comment on attachment 186042 [details] [diff] [review]
patch update (with the correct buffer size)

r+, although I think I'd also prefer using the same style in both cases.
Attachment #186042 - Flags: review?(cbiesinger) → review+
(and sorry about my apparent misreading of the docs)
(Assignee)

Comment 17

13 years ago
(In reply to comment #13)
> Aren't ANSI strings supposed to have a single null byte?  Whereas wide strings
> are supposed to have two null bytes?  I don't get why SetLength(len - 1)
> couldn't be used in the ANSI and wide cases

The wide case is fine. I'm worried about the ANSI case because of the peculiar
requirement for two additional bytes as quoted by cbie in comment #14 and
comment #6. For some unknown reason, Windows needs two extra bytes so that we
can't |SetLength(required - 2)| because in that case we would give Windows one
fewer bytes than it needs. Neither does |SetLength(required - 1)| work  (cbie's
concern in comment #8) 

(In reply to comment #15)
> (From update of attachment 186042 [details] [diff] [review] [edit])
> r+, although I think I'd also prefer using the same style in both cases.

Thanks for r. I'll change the wide case to use nsAutoArrayPtr, too.

 (In reply to comment #16)
> (and sorry about my apparent misreading of the docs)

No problem. You raised an important point I would have missed otherwise.

Comment 18

13 years ago
Comment on attachment 186042 [details] [diff] [review]
patch update (with the correct buffer size)

sr=darin with both cases switched to nsAutoArrayPtr as discussed.
Attachment #186042 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 19

13 years ago
Comment on attachment 186042 [details] [diff] [review]
patch update (with the correct buffer size)

asking for a to trunk.
This patch 'translates' the existing code that invokes Windows APIs directly to
use our new interface. The risk of regressing anything is low.

As a added benefit of 'translation', it takes care of switching between 'A' and
'W' APIs so that on Unicode-capable platforms (Win2k/XP), it can deal with
characters outside the ANSI code page.
Attachment #186042 - Flags: approval1.8b3?

Updated

13 years ago
Attachment #186042 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 20

13 years ago
fix landed on the trunk 
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: helpwanted
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.