Closed
Bug 295202
Opened 19 years ago
Closed 19 years ago
Convert windows nsOSHelperAppService to use nsIWindowsRegKey
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Biesinger, Assigned: jshin1987)
References
()
Details
Attachments
(2 files, 1 obsolete file)
30.32 KB,
patch
|
Biesinger
:
review-
|
Details | Diff | Splinter Review |
30.61 KB,
patch
|
Biesinger
:
review+
darin.moz
:
superreview+
asa
:
approval1.8b3+
|
Details | Diff | Splinter Review |
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•19 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•19 years ago
|
||
There are definitely quite a few hits for RegQueryValueEx in LXR.
Assignee | ||
Comment 3•19 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•19 years ago
|
||
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•19 years ago
|
Attachment #184661 -
Flags: superreview?(darin)
Attachment #184661 -
Flags: review?(cbiesinger)
Comment 5•19 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-
Reporter | ||
Comment 6•19 years ago
|
||
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•19 years ago
|
||
all the review comments are addressed.
Attachment #184661 -
Attachment is obsolete: true
Attachment #185967 -
Flags: superreview?(darin)
Attachment #185967 -
Flags: review?(cbiesinger)
Reporter | ||
Comment 8•19 years ago
|
||
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)
Reporter | ||
Updated•19 years ago
|
Attachment #185967 -
Flags: review?(cbiesinger) → review-
Assignee | ||
Comment 9•19 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•19 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•19 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•19 years ago
|
Attachment #185967 -
Flags: superreview?(darin)
Assignee | ||
Comment 12•19 years ago
|
||
Attachment #186042 -
Flags: superreview?(darin)
Attachment #186042 -
Flags: review?(cbiesinger)
Comment 13•19 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.
Reporter | ||
Comment 14•19 years ago
|
||
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."
Reporter | ||
Comment 15•19 years ago
|
||
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+
Reporter | ||
Comment 16•19 years ago
|
||
(and sorry about my apparent misreading of the docs)
Assignee | ||
Comment 17•19 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•19 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•19 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•19 years ago
|
Attachment #186042 -
Flags: approval1.8b3? → approval1.8b3+
Assignee | ||
Comment 20•19 years ago
|
||
fix landed on the trunk
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•