Closed Bug 492948 Opened 10 years ago Closed 10 years ago

xulrunner-stub.exe cannot discover GRE

Categories

(Core :: XPCOM, defect, P1, major)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: ynvich, Assigned: benjamin)

Details

(Keywords: fixed1.9.1, regression)

Attachments

(1 file, 6 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3) Gecko/20090329 Firefox/3.1b3
Build Identifier: 1.9.1 beta4

This is a regression introduced by bug 455381.

After commit 66c7e78d564e, we try to use NS_ConvertUTF8toUTF16 while working with Windows Registry to locate xpcom.dll. But NS_ConvertUTF8toUTF16 requires xpcom.dll to function.

Reproducible: Always
For me, runtime GRE discovery is an important feature. Nominating as blocker.
Flags: blocking1.9.1?
We should be using the CP_UTF8 code here!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: regression
Priority: -- → P1
somehow related to bug 488157 ?
Not exactly, but that kind of fix is what's needed: somebody's using NS_ConvertUTF8toUTF16 before initializing XPCOM, which won't work. Instead you have to use MultiByteToWideChar
Which line is still present in mozilla-central as well: http://hg.mozilla.org/mozilla-central/annotate/abfb6aa0e08a/xpcom/glue/nsGREGlue.cpp#l450
This looks like a leak:

>        return PR_TRUE;
> +      free(lowerBuf);
> +      free(upperBuf);
yes, definitely, thanks for catching it
(In reply to comment #8)
> Created an attachment (id=377423) [details]
> switches to MultiByteToWideChar in nsGREGlue.cpp

The patch works, but it fixes only part of the problem. This call is passing null pointers to wcscmp, which doesn't validate its parameters:

http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/a0622a816df8/xpcom/glue/nsVersionComparator.cpp#l301
Brad's patch with some enhancements restores GRE discovery.
Attachment #377525 - Flags: review?
Comment on attachment 377423 [details] [diff] [review]
switches to MultiByteToWideChar in nsGREGlue.cpp

>+    PRInt32 wideLen = MultiByteToWideChar(CP_ACP, 0, aBuffer, -1, 0, 0);

Actually it does not cause any problems in most cases, this should be CP_UTF8 instead.
yes, it should definitely be CP_UTF8 otherwise we may not allocate enough memory.
Attachment #377423 - Flags: review?(benjamin) → review+
Comment on attachment 377423 [details] [diff] [review]
switches to MultiByteToWideChar in nsGREGlue.cpp

r=me if you use CP_UTF8 consistently and avoid any mention of CP_ACP... although boy, this sure could use a helper function
Carrying forward r+ from attachment 377525 [details] [diff] [review]
Attachment #377525 - Attachment is obsolete: true
Attachment #377690 - Flags: review+
Attachment #377525 - Flags: review?
Keywords: checkin-needed
(In reply to comment #16)
> Created an attachment (id=377690) [details]
>  switches to MultiByteToWideChar in nsGREGlue.cpp v3
> 
> Carrying forward r+ from attachment 377525 [details] [diff] [review]

Attachment 377525 [details] [diff] doesn't have r+. (Also, "carrying forward" doesn't really work, because the reviewer matters.)

Please make sure to mark all obsolete patches as obsolete and assign this bug to the right person before adding checkin-needed.
Keywords: checkin-needed
(In reply to comment #17)
> > Carrying forward r+ from attachment 377525 [details] [diff] [review]
> 
> Attachment 377525 [details] [diff] doesn't have r+. (Also, "carrying forward" doesn't really
> work, because the reviewer matters.)
> 
> Please make sure to mark all obsolete patches as obsolete and assign this bug
> to the right person before adding checkin-needed.

I am sure I saw benjamin:r+ on attachment 377525 [details] [diff] [review] before attachment 377690 [details] [diff] [review] was filed. The flag may have been cleared by obsoleting the former.

Brad, could you check v3 and obsolete v1 if appropriate?
(In reply to comment #18)
> I am sure I saw benjamin:r+ on attachment 377525 [details] [diff] [review] before attachment 377690 [details] [diff] [review] was
> filed. The flag may have been cleared by obsoleting the former.

After checking the bug history, I am not so sure. Sorry.
After seeing bug 492225, this patch needs additional code to ensure null termination. Please make a helper function like AllocConvertUTF16toUTF8 which does the correct allocation/termination.
Attachment #377690 - Flags: review+ → review-
The difference between the patch on bug 492225 and this patch is that this patch is passing -1 for the length of the input string and the patch on bug 492225 is passing the length of the string in chars.  Passing -1 as the length accounts for the null char correctly.
Attachment #377690 - Flags: review-
Attachment #377690 - Attachment is obsolete: true
I'd still really like a helper function to abstract away the double-call logic
Assignee: nobody → benjamin
Attached patch Patch being tested, rev. 1 (obsolete) — Splinter Review
Here's what I'm working. It compiles, but I haven't tried to use it yet.
Attachment #377423 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #378416 - Flags: review?(bugmail)
Attachment #378416 - Attachment is obsolete: true
Attachment #378666 - Flags: review?(bugmail)
Attachment #378416 - Flags: review?(bugmail)
Attached patch rev. 2, with more context (obsolete) — Splinter Review
Whiteboard: [has patch, needs review blassey]
Comment on attachment 378667 [details] [diff] [review]
rev. 2, with more context

r=me, just two things to fix up and a "take it or leave it" comment


>       for (; ok && props < propsEnd; ++props) {
>         pathlen = sizeof(pathbuf);
> 
this will set pathlen to (MAXPATHLEN + 1) * 2, where it should be MAXPATHLEN +1


>-           !CopyWithEnvExpansion(aBuffer, pathbuf, aBufLen, pathtype))) {
>+           !CopyWithEnvExpansion(buffer, pathbuf, aBufLen, pathtype))) {

aBufLen is the wrong length, it should be pathlen


>-    if (ok)
>+    if (ok) {
>+      WideCharToMultiByte(CP_UTF8, 0, buffer, -1, aBuffer, aBufLen, NULL, NULL);
>       return PR_TRUE;
>+    }

I wonder if we should check the return value of WideCharToMultiByte but I'm not sure what the right behavior should be
Attachment #378667 - Flags: review+
Attachment #378666 - Attachment is obsolete: true
Attachment #378666 - Flags: review?(bugmail)
Whiteboard: [has patch, needs review blassey] → [has patch]
Attachment #378667 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/fef8eb29d1e5

I can't push this to 1.9.1 tonight, but I'm happy for others to do so or I'll do it tomorrow morning.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch] → needs 1.9.1 landing
Keywords: fixed1.9.1
(In reply to comment #27)
> Created an attachment (id=378678) [details]
> Nits fixed, rev. 2.1

Sorry, but error is not gone after patching with rev. 2.1
Gennadiy, could you be more specific? What are your steps to reproduce? I specifically tested this with a registered GRE and the stub and it was found correctly.
(In reply to comment #31)
> Gennadiy, could you be more specific? What are your steps to reproduce? I
> specifically tested this with a registered GRE and the stub and it was found
> correctly.

I rebuilt the program after applying the patch rev 2.1 and my application crashed with error (null pointer). with my patch it works fine
Non virtual system WinXP SP3
Gennadiy: please indicate the exact steps you're using to test.
(In reply to comment #33)
> Gennadiy: please indicate the exact steps you're using to test.

1. apply patch
2. build xulrunner
3. register it (xulrunner --register-global)
4. build my application
5. try to launch my application -> error report from windows
can you get a stack trace from that crash?
(In reply to comment #35)
> can you get a stack trace from that crash?

 	appl.exe!wcscmp(const wchar_t * src=0x0012f614, const wchar_t * dst=0x00000000)  Line 53 + 0x4 bytes	C
>	appl.exe!GRE_GetPathFromRegKey(HKEY__ * aRegKey=0x000007c8, const GREVersionRange * versions=0x00000000, unsigned int versionsLength=0x00000001, const GREProperty * properties=0x00364238, unsigned int propertiesLength=0x00000002, char * aBuffer=0x0012fdb4, unsigned int aBufLen=0x00000104)  Line 802 + 0x46 bytes	C++
 	appl.exe!GRE_GetGREPathWithProperties(const GREVersionRange * versions=0x0012fa0c, unsigned int versionsLength=0x00000001, const GREProperty * properties=0x004152e4, unsigned int propertiesLength=0x00000001, char * aBuffer=0x0012fdb4, unsigned int aBufLen=0x00000104)  Line 463 + 0x1e bytes	C++
 	appl.exe!NS_internal_main(int argc=0x00000001, char * * argv=0x00367e60)  Line 313 + 0x1c bytes	C++
 	appl.exe!wmain(int argc=0x00000000, wchar_t * * argv=0x00364c60)  Line 112	C++
 	appl.exe!__tmainCRTStartup()  Line 327 + 0x12 bytes	C
 	kernel32.dll!7c817077() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]
(In reply to comment #35)
> can you get a stack trace from that crash?

Sorry, I take my words back. The patch isn't applied as needed.
It works well.
Verified with the Windows XR nightly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.