Closed Bug 436531 Opened 12 years ago Closed 12 years ago

asmXPTC_InvokeByIndex Bogus Return Value in WinMobile builds

Categories

(Core :: XPCOM, defect, critical)

ARM
Windows CE
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: wolfe, Assigned: wolfe)

References

Details

(Keywords: mobile)

Attachments

(1 file, 8 obsolete files)

When calling XPTC_InvokeByIndex on WinMobile builds, the return value of the function is a random, bogus value.

The function prototype for XPTC_InvokeByIndex is:

extern "C" nsresult
asmXPTC_InvokeByIndex( nsISupports* that,
                       PRUint32 methodIndex,
                       PRUint32 paramCount,
                       nsXPTCVariant* params,
                       PRUint32 pfn_CopyToStack,
                       PRUint32 pfn_CountWords);  


This nsresult random return value will mess up anyone who is examining the function's return code.  

All arguments passed into the function appear to be handled correctly.

Patch for this bug will also update the XPCOM/Proxy/Tests to check for this (and other) potential error.
Attached patch WinCE XPCOM Patch (obsolete) — Splinter Review
Revises XPCOM/proxy/tests to be more complete, fixes asmXPTC_InvokeByIndex return code
Assignee: nobody → wolfe
Status: NEW → ASSIGNED
This is a revised patch for the WinCE XPCOM return code value being incorrect.

This patch adds more duplicate output via printf and LOG(), allowing a casual tester to see immediate feedback about the XPCOM proxy functionality.
Attachment #323091 - Attachment is obsolete: true
Attached patch Oops - Added a needed IDL file (obsolete) — Splinter Review
HG does not take added files unless you actually do an "hg add" -- my oops.

Here is the revised patch file, which should compile XPCOM/proxy/tests executables correctly.
Attachment #323132 - Attachment is obsolete: true
I pulled out the large Proxy Test class into its own file - then end up including that CPP file back into proxytests.cpp because of automated build Makefile struggles.  This latest patch has all files added into the mix - with the result compiling the <OBJDIR>/XPCOM/proxy/tests directory.
Attachment #323366 - Attachment is obsolete: true
Just removed two unnecessary commented out lines of assembly code which would have cluttered up the ASM file.  As per request of DougT
Attachment #323381 - Attachment is obsolete: true
Attachment #324695 - Flags: review?(bsmedberg)
Attachment #324695 - Flags: review?(bsmedberg) → review?(benjamin)
Comment on attachment 324695 [details] [diff] [review]
Removed two commented out assembly code lines

>diff -r 41ccf537b240 build/wince/shunt/build/vs8/mozce_shunt_static.vcproj

I have not reviewed this file, as I don't know what it's for.

>diff -r 41ccf537b240 xpcom/proxy/tests/Makefile.in

>+CPP_UNIT_TESTS += proxytests.cpp \
>+		proxy-create-threadsafety.cpp \
>+		$(NULL)
>+XPIDLSRCS	= nsITestProxy.idl \
>+                  nsITestProxyOrig.idl

please indent both of these as
CPP_UNIT_TESTS += \
  file1.cpp \
  file2.cpp \
  $(NULL)

with no tabs
Attachment #324695 - Flags: review?(benjamin) → review+
Same as Benjamin's reviewed file, but with different indentation for Makefile.in, and removed mozce_shunt_static.vcproj and nsProxyEvent.cpp file changes.

The mozce_shunt_static.vcproj file change and the nsProxyEvent.cpp file change should not have been included in the patch to be reviewed. 

Thanks to Benjamin for catching those un-needed patch hunks!
Attachment #324695 - Attachment is obsolete: true
Attachment #330876 - Flags: review?(benjamin)
Attachment #330876 - Flags: review?(benjamin) → review+
Added keyword checkin-needed
Keywords: checkin-needed
Added Checkin-needed flag.
Blocks: 432792
Pushed as 17135:2e3d61018e3d.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a2
This was backed out due to build failures.  The Makefile was broken (trailing spaces after some \'s, missing $(NULL) in XPIDLSRCS), and a header file was missing.

Could you post a fixed patch please?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Fixed patch file, including bugs listed and missing header/source files.

Compiles on my pulled-this-morning (7-Sep-08) mozilla-central.  I apologize for creating the extra work for you, Reed and Dave.
Attachment #330876 - Attachment is obsolete: true
Attachment #337352 - Flags: review?(wolfe)
Comment on attachment 337352 [details] [diff] [review]
Fixed Patch File - Compiles against my just-pulled mozilla-central repository

Sent back to Benjamin for re-review.  Nothing new in this patch, just missing files and Makefile.in fixes (I have no idea why/where the previous patch came from in my compiling system).

-- John Wolfe (wolfe@lobo.us)
Attachment #337352 - Flags: review?(wolfe) → review?(bsmedberg)
You want to request review from ":bs" (or benjamin@smedbergs) instead of that address (he only uses it for watching).
Comment on attachment 337352 [details] [diff] [review]
Fixed Patch File - Compiles against my just-pulled mozilla-central repository

Thanks to Gavin for pointing out that Benjamin uses that gmail account for watching bugs, and should be requested via a different email.
Attachment #337352 - Flags: review?(bsmedberg) → review?(benjamin)
Attachment #337352 - Flags: review?(benjamin) → review+
Added checkin-needed flag.
Keywords: checkin-needed
(In reply to comment #16)
> Added checkin-needed flag.

Multiple failurse to apply: needs unbitrotting.
Keywords: checkin-needed
Attached patch v.8 patch (obsolete) — Splinter Review
Same as Attachment 337352 [details] [diff], but adjusts UUIDs so each are unique in XPCOM Proxy Tests.

Not for review yet, want to make sure this does not mess up the Win32 build.

Also, for some reason, GetProxyForObject is returning NULL proxy objects under WinMobile 6 XPCOM Proxy Test (running on HTC Touch Diamond USA).
Attachment #337352 - Attachment is obsolete: true
just so it is obvious what I am doing - the patch is the same as what was review by bsmedberg.  wolfe updated it and changed the uuids as requested.  I looked over it, applied it to my build and it works on windows ce and win 32.
Assignee: wolfe → nobody
Component: XPConnect → XPCOM
QA Contact: xpconnect → xpcom
Assignee: nobody → wolfe
Target Milestone: mozilla1.9.1a2 → mozilla1.9.1b2
Comment on attachment 342100 [details] [diff] [review]
v.8 patch

this patch doesn't work on non-windows machines.  Also, it is missing the new files from the previous patch.
Attachment #342100 - Attachment is obsolete: true
Attachment #342100 - Flags: review-
wince arm changes checked in:

dougt@mozilla.com
Wed Oct 08 20:58:40 2008 -0700	7e5d581bc86d	wolfe — Bug 436531 - asmXPTC_InvokeByIndex Bogus Return Value in WinMobile builds. r=bsmedberg/dougt

Wolfe, please create a separate bug for just the tests cases.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.