487174 - Modify nsXULStub to launch from GRE folder on WinCE

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Because of LoadLibrary behavior on WinCE we can't load DLLs from outside the process folder or several other restrictive locations.

Some background can be found here: https://wiki.mozilla.org/Mobile/WinCE_LoadLibrary_SearchPath_Bug_Notes

We intend to try a modified version of Option #1 from the wiki page:
* Launch fennec.exe from wherever it is installed (phase 1)
* The stub will find the GRE folder
* The stub will copy itself to the GRE folder
* The stub will launch the copy using the -app parameter, which will point back to the actual install location (phase 2)
* The second launch will load the XPCOM Glue and use the application.ini to launch Fennec

Using this approach allows Fennec to use XPCOM Glue and icon resources loaded into fennec.exe, working around some process issues with the WinCE task manager.

Yes, this is a PITA

Comment 1

[Benjamin Smedberg]

Can you clarify some things from the wiki:

* The XPCOM glue should be explicitly loading all the dependencies of xul.dll (using LoadLibrary and a full path name). Is this not sufficient? Or if we load nspr4.dll using LoadLibrary, does WinCE not use that copy when resolving implicit dependencies later?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

As far as I can confirm on Windows Mobile 6 Emulator, LoadLibraryExW with 0 for dwFlags can load xulrunner DLLs and Fennec seems work well at least with release build.

But unfortunately with debug build (i.e. --enable-debug option), loading xul.dll fails due to "
not enough storage is available to complete this operation" error.
I guess the error causes by limitation of 32MB on WinCE application because xul.dll with debug build is 30MB itself.
See 
http://msdn.microsoft.com/en-us/library/ms836325.aspx#advmemmgmt_topic3

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch

I tested with this patch.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #2)

> But unfortunately with debug build (i.e. --enable-debug option), loading
> xul.dll fails due to "
> not enough storage is available to complete this operation" error.

After resetting emulator, this error has gone.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 371597 [details] [diff] [review]
A patch


> static void
> ReadDependentCB(const char *aDependentLib)
> {
>-    
>+    wchar_t wideDependentLib[MAX_PATH];
>+    MultiByteToWideChar(CP_ACP, 0, aDependentLib, -1, wideDependentLib, MAX_PATH);
>+
>     HINSTANCE h =
>-        LoadLibraryExW(NS_ConvertUTF8toUTF16(aDependentLib).get(), NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
>-    if (!h)
>+#ifdef WINCE
>+        LoadLibraryExW(wideDependentLib, NULL, 0);
>+#else
>+        LoadLibraryExW(wideDependentLib, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
>+#endif
>+    if (!h) {
>+        wprintf(L"Error loading %s %s\n", wideDependentLib, lpMsgBuf);

Oops, sorry. lpMsgBuf does not exist in the path.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #2)
> As far as I can confirm on Windows Mobile 6 Emulator, LoadLibraryExW with 0 for
> dwFlags can load xulrunner DLLs and Fennec seems work well at least with
> release build.

Where is fennec.exe located when you launch? Is fennec.exe in the same folder as xulrunner.exe?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #6)

> Where is fennec.exe located when you launch? Is fennec.exe in the same folder
> as xulrunner.exe?

No. fennec.exe is in the parent folder of xulrunner.exe. Like this:

bin\fennec.exe
bin\xulrunner\xulrunner.exe
bin\xulrunner\xul.dll 
...

Comment 8

[John Wolfe (:wolfe)]

Attachment: sligth revision on patch

Fixes compiler error from a missing lpMsgBuf in ReadDependentCB() function.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #8)

> Fixes compiler error from a missing lpMsgBuf in ReadDependentCB() function.

Thank you, John. The patch works fine on your environment? If so, Bug 482741 and Bug 482285 also are fixed by this patch, I think.

Comment 10

[John Wolfe (:wolfe)]

The latest patch works just fine on my WinMobile device - A HTC Touch Pro.

I applied this patch, compiled a release build, made a install CAB, and ran the install CAB on my device after removing all other fennec binaries from both the device and the associated microSD storage card.

Fennec started up just fine - so I think this latest patch works great.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #1)
> Can you clarify some things from the wiki:
> 
> * The XPCOM glue should be explicitly loading all the dependencies of xul.dll
> (using LoadLibrary and a full path name). Is this not sufficient? Or if we load
> nspr4.dll using LoadLibrary, does WinCE not use that copy when resolving
> implicit dependencies later?

Benjamin - It appears to be sufficient. The patch from Hiroyuki Ikezoe, tweaked by Wolfe, appears to get XPCOMGlue/WinCE working just like other platforms. That is the intent of this bug, so that's the patch we'd like to go with. Setting r?

Comment 12

[Benjamin Smedberg]

Comment on attachment 371964 [details] [diff] [review]
sligth revision on patch

>diff --git a/xpcom/glue/standalone/nsGlueLinkingWin.cpp b/xpcom/glue/standalone/nsGlueLinkingWin.cpp

This patch is too big... you should be able to use symbolic constant defines:

#ifdef WINCE
#define MOZ_LOADLIBRARY_FLAGS 0
#else
#define MOZ_LOADLIBRARY_FLAGS LOAD_WITH_ALTERED_SEARCH_PATH
#endif

> static void
> ReadDependentCB(const char *aDependentLib)
> {
>-    
>+    wchar_t wideDependentLib[MAX_PATH];
>+    MultiByteToWideChar(CP_ACP, 0, aDependentLib, -1, wideDependentLib, MAX_PATH);
>+
>     HINSTANCE h =
>-        LoadLibraryExW(NS_ConvertUTF8toUTF16(aDependentLib).get(), NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
>-    if (!h)
>+#ifdef WINCE
>+        LoadLibraryExW(wideDependentLib, NULL, 0);
>+#else
>+        LoadLibraryExW(wideDependentLib, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
>+#endif

What was the motivation for changing from UTF8 to the native codepage here? This value originally comes from http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsXPCOMGlue.h#103 which doesn't have a character set notation, and is passed from nsXULStub.cpp. I think that on Windows/WINCE it should be UTF8, not native, but we should verify this.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #12)
> > static void
> > ReadDependentCB(const char *aDependentLib)
> > {
> >-    
> >+    wchar_t wideDependentLib[MAX_PATH];
> >+    MultiByteToWideChar(CP_ACP, 0, aDependentLib, -1, wideDependentLib, MAX_PATH);
> >+
> >     HINSTANCE h =
> >-        LoadLibraryExW(NS_ConvertUTF8toUTF16(aDependentLib).get(), NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
> >-    if (!h)
> >+#ifdef WINCE
> >+        LoadLibraryExW(wideDependentLib, NULL, 0);
> >+#else
> >+        LoadLibraryExW(wideDependentLib, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
> >+#endif
> 
> What was the motivation for changing from UTF8 to the native codepage here?
> This value originally comes from
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsXPCOMGlue.h#103
> which doesn't have a character set notation, and is passed from nsXULStub.cpp.
> I think that on Windows/WINCE it should be UTF8, not native, but we should
> verify this.

This is a trace that I had been trying to load libraries and I am not sure the conversion is now needed since I have no machine for building here. I will confirm it is needed day after tomorrow.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: update patch

define MOZ_LOADLIBRARY_FLAGS.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #12)

> > static void
> > ReadDependentCB(const char *aDependentLib)
> > {
> >-    
> >+    wchar_t wideDependentLib[MAX_PATH];
> >+    MultiByteToWideChar(CP_ACP, 0, aDependentLib, -1, wideDependentLib, MAX_PATH);
> >+
> >     HINSTANCE h =
> >-        LoadLibraryExW(NS_ConvertUTF8toUTF16(aDependentLib).get(), NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
> >-    if (!h)
> >+#ifdef WINCE
> >+        LoadLibraryExW(wideDependentLib, NULL, 0);
> >+#else
> >+        LoadLibraryExW(wideDependentLib, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
> >+#endif
> 
> What was the motivation for changing from UTF8 to the native codepage here?
> This value originally comes from
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsXPCOMGlue.h#103
> which doesn't have a character set notation, and is passed from nsXULStub.cpp.
> I think that on Windows/WINCE it should be UTF8, not native, but we should
> verify this.

I forgot to an important thing that the argument source of ReadDependentCB is already converted to multi-byte code at http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsGlueLinkingWin.cpp#156 so we need to reconvert it to UTF-16.

Comment 16

[Benjamin Smedberg]

Comment on attachment 372339 [details] [diff] [review]
update patch

Please file a followup on the CP_ACP bugs: I believe we should be using UTF8 throughout this code and should not have any "native codepage" char*s.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #16)
> (From update of attachment 372339 [details] [diff] [review])
> Please file a followup on the CP_ACP bugs: I believe we should be using UTF8
> throughout this code and should not have any "native codepage" char*s.

If I understand things correctly, this includes code in nsXULStub and nsGlueLinkingWin.cpp - and any other code that uses const char buffers created in nsWindowsWMain.cpp

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Filed bug 488157

Comment 19

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

pushed http://hg.mozilla.org/mozilla-central/rev/6032ec47e686

Comment 20

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

Comment on attachment 372339 [details] [diff] [review]
update patch

this resolves  Bug 492948 which is a blocker

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 372339 [details] [diff] [review]
update patch

a191=beltzner

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cde6f902bccb