Closed Bug 295202 Opened 19 years ago Closed 19 years ago

Convert windows nsOSHelperAppService to use nsIWindowsRegKey

Categories

(Core Graveyard :: File Handling, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: jshin1987)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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.
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
There are definitely quite a few hits for RegQueryValueEx in LXR.
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
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #184661 - Flags: superreview?(darin)
Attachment #184661 - Flags: review?(cbiesinger)
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-
Attached patch patch update Splinter Review
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-
(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. 
> > 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.
(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).  
Attachment #185967 - Flags: superreview?(darin)
Attachment #186042 - Flags: superreview?(darin)
Attachment #186042 - Flags: review?(cbiesinger)
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)
(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 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+
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?
Attachment #186042 - Flags: approval1.8b3? → approval1.8b3+
fix landed on the trunk 
Status: ASSIGNED → RESOLVED
Closed: 19 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.

Attachment

General

Creator:
Created:
Updated:
Size: