Closed Bug 477727 Opened 11 years ago Closed 11 years ago

WinCE/ARM patches for native Firefox on Windows CE 6

Categories

(Core :: General, defect)

ARM
Windows CE
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 1.0a1-wm+ ---

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Keywords: fixed1.9.1)

Attachments

(11 files, 4 obsolete files)

12.39 KB, patch
crowderbt
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
1.17 KB, patch
dougt
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
1.40 KB, patch
dcamp
: review+
pavlov
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
2.40 KB, patch
pavlov
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
1.10 KB, patch
pavlov
: review+
pavlov
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
3.41 KB, patch
ted
: review+
Details | Diff | Splinter Review
1.44 KB, patch
pavlov
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
1.29 KB, patch
pavlov
: review+
pavlov
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
2.22 KB, patch
vlad
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
7.13 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
7.13 KB, patch
dougt
: review+
Details | Diff | Splinter Review
I have a bunch of patches here that probably don't deserve their own bugs; I'm going to attach all of them to this bug.

These are all geared towards getting Firefox to run on a Windows CE 6 device, though many of these are generic bugfixes for memory corruption and the like.
Rework wince_shunt's environment map handling; the current implementation was pretty complex and had a number of memory buffer overrun bugs (leading to heap corruption errors with debug BSP builds).
GetViewportOrgEx isn't universally available, and generally isn't needed since SetViewportOrgEx returns the old viewport in the last arg.  Also fixes a bug in the FrameRect impl.

Note: this patch touches one bit of cross-platform code, removing the GetViewportOrgEx call in nsWindowsNativeDrawing.  Should be safe.
WinCE probably doesn't support FILE_FLAG_DELETE_ON_CLOSE; let's just try to delete the file before trying to create the lock.  If the lock is held, the delete will just fail, as will the open.
The xptcall Windows CE stubs were bouncing through a method on the class before going into the stub; this causes problems if the method impl has any code in it other than a straight tail call into the stub.  In at least debug builds, MSVC was emitting some additional code around the call, causing the stack to get all screwed up and all args beyond the 3rd to be corrupted.

The xptcall asm code had commented-out decorated symbol names for the right C++ impl; anyone know if there were any problems with it?  It seems to be working fine here, but I'm not sure why it was commented out originally.
CopySingleFile was missing some { }'s, causing a MoveFileW to get unconditionally executed.  (This was causing some profile startup problems.)
Mainly adding the right -ENTRY (mainWCRTStartup instead of wmainCRTStartup like on desktop).  Also makes migration and the shell service conditional on not WINCE -- migration requires libreg which doesn't compile on WinCE, and the shell service probably just needs a WinCE-specific impl.
Some code needs to be conditional on being compiled for Windows Mobile; this adds a --disable-windows-mobile-components configure option (defaults to off [i.e., windows mobile enabled] to avoid breaking current build configs).  WINCE_WINDOWS_MOBILE then becomes available both in code and in makefiles.

The WINDOWS_MOBILE pieces are linking libxul with cellcore, and a bunch of softkbd-related stuff in nsWindow.cpp (which is a separate patch).

This also adds the correct -ENTRY arg for the js shell and xpcshell.
Didn't track down the underlying cause here, but for some reason we don't allocate enough space here, and cause heap corruption in the final free(cl).  Allocating a few extra bytes fixes this.
This pulls out some code in nsWindow that's Windows Mobile specific; should be no functional changes on Windows Mobile, basically just skips over a bunch of it for Windows CE.  Also does the final blit from image surface painting using StretchDIBits directly instead of going through Cairo.
CoCreateGuid is only present if full DCOM support is compiled in; we don't really need it that badly, so just use the OS randomness function instead to avoid that dependency on WinCE/WinMo.
Attachment #361421 - Flags: superreview+
Attachment #361421 - Flags: review+
Attachment #361421 - Flags: approval1.9.1?
Attachment #361429 - Flags: superreview+
Attachment #361429 - Flags: review+
Attachment #361429 - Flags: approval1.9.1?
Attachment #361419 - Flags: superreview+
Attachment #361419 - Flags: review?(dcamp)
Attachment #361416 - Flags: review?(crowder)
Some more info about the xptcall patch in comment #5:

With debug (no optimizations), the following is emitted for each stub:

?Stub44@nsXPTCStubBase@@UAAIXZ (public: virtual unsigned int __cdecl nsXPTCStubB
ase::Stub44(void)):
  00000000: E1A0C00D mov         r12, sp
  00000004: E92D0001 stmdb       sp!, {r0}
  00000008: E92D5000 stmdb       sp!, {r12, lr}
  0000000C: E24DD008 sub         sp, sp, #8
$M18832:
  00000010: EB000000 bl          00000018
  00000014: E58D0004 str         r0, [sp, #4]
  00000018: E59D3004 ldr         r3, [sp, #4]
  0000001C: E58D3000 str         r3, [sp]
  00000020: E59D0000 ldr         r0, [sp]
  00000024: E28DD008 add         sp, sp, #8
  00000028: E89DA000 ldmia       sp, {sp, pc}

With -O1:

?Stub44@nsXPTCStubBase@@UAAIXZ (public: virtual unsigned int __cdecl nsXPTCStubBase::Stub44(
void)):
  00000000: EA000000 b           00000008

The no-opt code messes with the stack, so any args that aren't in r0/r1/r2/r3 won't be found by the stub code that it branches to.
Comment on attachment 361420 [details] [diff] [review]
05 - emit xptcall stub impls directly, instead of bouncing through a method [CHECKED IN]

you should just remove the #if 0 code, whenever this gets checked in
Attachment #361419 - Flags: review?(dcamp) → review+
Attachment #361417 - Flags: superreview+
Attachment #361417 - Flags: review+
Attachment #361417 - Flags: approval1.9.1?
Attachment #361419 - Flags: approval1.9.1?
wolfe, please look at comment #5 and #11.
Attachment #361418 - Flags: review+
Comment on attachment 361420 [details] [diff] [review]
05 - emit xptcall stub impls directly, instead of bouncing through a method [CHECKED IN]

I tested this and it appears to be working just fine in Fennec.
Comment on attachment 361416 [details] [diff] [review]
01 - rework mozce_shunt's environment map handling [CHECKED IN]

+  int len = strlen(envstr);
+
+  char *key = (char*) malloc(len+1);
+  strcpy(key, envstr);

strdup() instead?  Maybe I'm missing something...



+      int k = WideCharToMultiByte( CP_ACP, 0, pIn, envlen, key, 255, NULL, NULL );

Fix the wonky spacing, since you're here already?  There's another untouched call nearby with weird spacing too, if you feel like fixing it.



unsigned short* mozce_GetEnvironmentCL()

Should this return a wchar_t ?  All of the math done inside the routine seems to think so.


r+ with those, or at least your thoughts on 'em.
Attachment #361416 - Flags: review?(crowder) → review+
Updated patch #2; we need to get the orig viewport in gfxWindowsNativeDrawing before calling SetViewportOrgEx.
Attachment #361417 - Attachment is obsolete: true
Attachment #361585 - Flags: superreview+
Attachment #361585 - Flags: review+
Attachment #361417 - Flags: approval1.9.1?
Attachment #361420 - Flags: review+
Comment on attachment 361427 [details] [diff] [review]
09 - bandaid some memory corruption in nsWindowsRestart [CHECKED IN]

please file a followup bug!
Attachment #361427 - Flags: review+
Attachment #361416 - Attachment description: 01 - rework mozce_shunt's environment map handling → 01 - rework mozce_shunt's environment map handling [CHECKED IN]
Attachment #361585 - Attachment description: 02 - remove calls to GetViewportOrgEx (v2) → 02 - remove calls to GetViewportOrgEx (v2) [CHECKED IN]
Attachment #361418 - Attachment description: 03 - remove FLAG_DELETE_ON_CLOSE in ProfileLock → 03 - remove FLAG_DELETE_ON_CLOSE in ProfileLock [CHECKED IN]
Attachment #361419 - Attachment description: 04 - fix unaligned access in nsUrlClassifierDBService → 04 - fix unaligned access in nsUrlClassifierDBService [CHECKED IN]
Attachment #361420 - Attachment description: 05 - emit xptcall stub impls directly, instead of bouncing through a method → 05 - emit xptcall stub impls directly, instead of bouncing through a method [CHECKED IN]
Attachment #361421 - Attachment description: 06 - add missing { } around WINCE bits in nsLocalFileWin → 06 - add missing { } around WINCE bits in nsLocalFileWin [CHECKED IN]
Attachment #361427 - Attachment description: 09 - bandaid some memory corruption in nsWindowsRestart → 09 - bandaid some memory corruption in nsWindowsRestart [CHECKED IN]
Attachment #361429 - Attachment description: 11 - remove CoCreateGuid usage on WinCE → 11 - remove CoCreateGuid usage on WinCE [CHECKED IN]
Updated patch #7.  The shell service was easy to fix, so this just fixes that and disables profile migration.
Attachment #361423 - Attachment is obsolete: true
Attachment #361658 - Flags: review?(gavin.sharp) → review+
tracking-fennec: --- → 1.0a1-wm+
Comment on attachment 361425 [details] [diff] [review]
08 - Add WINCE_WINDOWS_MOBILE define [CHECKED IN]

Ted, this look ok for adding a new define/makefile flag, WINCE_WINDOWS_MOBILE?  It should default to 1/true if the target is wince, unless --disable-windows-mobile-components is given.
Attachment #361425 - Flags: review?(ted.mielczarek)
Updated version of patch #10; just merges changes with current nsWindow.cpp.  Should result in no functional changes for Windows Mobile; for Windows CE, just disables some softkb/IME stuff.

Also does a direct StretchDIBits for the final blit when using image surfaces for rendering, instead of going through cairo; no need to do all the Cairo work here.
Attachment #361428 - Attachment is obsolete: true
Attachment #361894 - Flags: review?(doug.turner)
Attachment #361658 - Attachment description: 07 - browser components update (fix shell service, disable migration) → 07 - browser components update (fix shell service, disable migration) [CHECKED IN]
Status: NEW → ASSIGNED
Comment on attachment 361425 [details] [diff] [review]
08 - Add WINCE_WINDOWS_MOBILE define [CHECKED IN]

This looks fine. As discussed on IRC, I'd prefer if we could fail in configure with a useful error message if you don't have the windows mobile bits in your SDK, or at least fail somewhere in the code that's going to break with a useful error message.
Attachment #361425 - Flags: review?(ted.mielczarek) → review+
I found the #defines for VK_TSOFT1 and friends hiding in winuserm.h, which doesn't get included by anything unless you explicitly ask for it.  So even fewer WINMO-specific things, just the soft kb pieces.
Attachment #361894 - Attachment is obsolete: true
Attachment #362059 - Flags: review?(doug.turner)
Attachment #361894 - Flags: review?(doug.turner)
Comment on attachment 362059 [details] [diff] [review]
10 - nsWindow widget code separation for WinCE/WinMo (v3) [CHECKED IN]

where is WINCE_HAVE_SOFTKB defined?

Also, this can not land until the patch which sets up WINCE_WINDOWS_MOBILE has landed.
Attachment #362059 - Flags: review?(doug.turner) → review+
Attachment #361425 - Attachment description: 08 - Add WINCE_WINDOWS_MOBILE define → 08 - Add WINCE_WINDOWS_MOBILE define [CHECKED IN]
Attachment #362059 - Attachment description: 10 - nsWindow widget code separation for WinCE/WinMo (v3) → 10 - nsWindow widget code separation for WinCE/WinMo (v3) [CHECKED IN]
WINCE_HAVE_SOFTKB is at the start of nsWindow.cpp, conditional on WINCE_WINDOWS_MOBILE.  I suspect I'll ahve to make the windows CE keyboard input thing work somehow at some point, but it'll need to be slightly different from the CE code.

Everything here's checked in; thanks reviewers!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
vlad, when the software keyboard stuff lands, bug 482739 can land as well.
Blocks: 482739
Comment on attachment 361416 [details] [diff] [review]
01 - rework mozce_shunt's environment map handling [CHECKED IN]

a191=beltzner
Attachment #361416 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 361418 [details] [diff] [review]
03 - remove FLAG_DELETE_ON_CLOSE in ProfileLock [CHECKED IN]

a191=beltzner
Attachment #361418 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 361419 [details] [diff] [review]
04 - fix unaligned access in nsUrlClassifierDBService [CHECKED IN]

a191=beltzner
Attachment #361419 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 361420 [details] [diff] [review]
05 - emit xptcall stub impls directly, instead of bouncing through a method [CHECKED IN]

a191=beltzner
Attachment #361420 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 361421 [details] [diff] [review]
06 - add missing { } around WINCE bits in nsLocalFileWin [CHECKED IN]

a191=beltzner
Attachment #361421 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 361427 [details] [diff] [review]
09 - bandaid some memory corruption in nsWindowsRestart [CHECKED IN]

a191=beltzner
Attachment #361427 - Flags: approval1.9.1? → approval1.9.1+
Attachment #361429 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 361429 [details] [diff] [review]
11 - remove CoCreateGuid usage on WinCE [CHECKED IN]

a191=beltzner
Comment on attachment 361585 [details] [diff] [review]
02 - remove calls to GetViewportOrgEx (v2) [CHECKED IN]

a191=beltzner
Attachment #361585 - Flags: approval1.9.1? → approval1.9.1+
Attachment #361658 - Flags: approval1.9.1?
Attachment #362059 - Flags: approval1.9.1?
Attachment #361658 - Flags: approval1.9.1? → approval1.9.1+
Attachment #362059 - Flags: approval1.9.1? → approval1.9.1+
Depends on: 490813
You need to log in before you can comment on or make changes to this bug.