620888 - ReadParam(msg, iter, uint32) is extracted to ReadParam(msg, iter, size_t/intptr_t) on Windows x86

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Makoto Kato [:m_kato]]

ReadParam(msg, iter, uint32) is extraced to ReadParam(msg, iter, size_t/intptr_t) on Windows x86.  if uint32, we should not use size_t due to compatiblity of 32bit <-> 64bit IPC and reduction of IPC message size.


- Disassembled code on the latest nightly

0:044> x xul!*nsACString*::Read
66065a37 xul!IPC::ParamTraits<nsACString_internal>::Read (class IPC::Message *, void **, class nsACString_internal *)

0:044> u 66065a37
xul!IPC::ParamTraits<nsACString_internal>::Read:
66065a37 55              push    ebp
66065a38 8bec            mov     ebp,esp
66065a3a 51              push    ecx
66065a3b 56              push    esi
66065a3c 8b7508          mov     esi,dword ptr [ebp+8]
66065a3f 8d450b          lea     eax,[ebp+0Bh]
66065a42 50              push    eax
66065a43 ff750c          push    dword ptr [ebp+0Ch]
66065a46 83c604          add     esi,4
66065a49 56              push    esi
66065a4a e83026ebff      call    xul!Pickle::ReadBool (65f1807f)
66065a4f 84c0            test    al,al
66065a51 7449            je      xul!IPC::ParamTraits<nsACString_internal>::Read+0x65 (66065a9c)
66065a53 807d0b00        cmp     byte ptr [ebp+0Bh],0
66065a57 740e            je      xul!IPC::ParamTraits<nsACString_internal>::Read+0x30 (66065a67)
66065a59 6a01            push    1
66065a5b ff7510          push    dword ptr [ebp+10h]
66065a5e e86a26f3ff      call    xul!nsACString_internal::SetIsVoid (65f980cd)
66065a63 b001            mov     al,1
66065a65 eb37            jmp     xul!IPC::ParamTraits<nsACString_internal>::Read+0x67 (66065a9e)
66065a67 8d4508          lea     eax,[ebp+8]
66065a6a 50              push    eax
66065a6b ff750c          push    dword ptr [ebp+0Ch]
66065a6e 56              push    esi
66065a6f e82b25ebff      call    xul!Pickle::ReadIntPtr (65f17f9f)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ <-- HERE
66065a74 84c0            test    al,al
66065a76 7424            je      xul!IPC::ParamTraits<nsACString_internal>::Read+0x65 (66065a9c)
66065a78 ff7508          push    dword ptr [ebp+8]
66065a7b 8d45fc          lea     eax,[ebp-4]
66065a7e 50              push    eax
66065a7f ff750c          push    dword ptr [ebp+0Ch]
66065a82 56              push    esi
66065a83 e88d24ebff      call    xul!Pickle::ReadBytes (65f17f15)
66065a88 84c0            test    al,al
66065a8a 7410            je      xul!IPC::ParamTraits<nsACString_internal>::Read+0x65 (66065a9c)
66065a8c ff7508          push    dword ptr [ebp+8]
66065a8f 8b55fc          mov     edx,dword ptr [ebp-4]
66065a92 8b4d10          mov     ecx,dword ptr [ebp+10h]
66065a95 e83666aaff      call    xul!nsACString_internal::Assign (65b0c0d0)
66065a9a ebc7            jmp     xul!IPC::ParamTraits<nsACString_internal>::Read+0x2c (66065a63)
66065a9c 32c0            xor     al,al
66065a9e 5e              pop     esi
66065a9f c9              leave
66065aa0 c3              ret

Comment 1

[Josh Matthews [:jdm]]

I am having difficulty imagining a hybrid 32/64 bit IPC setup.  When we build, don't we build the browser and plugin-container in the same manner?

Comment 2

[Makoto Kato [:m_kato]]

(In reply to comment #1)
> I am having difficulty imagining a hybrid 32/64 bit IPC setup.  When we build,
> don't we build the browser and plugin-container in the same manner?

64bit Windows build env builds 64-bit binary only.  But IPC protocol should be considered that 64bit process talks with 32bit process.  Example, on Mac OS X 64 bit firefox can talk with 32-bit plugin-container.

I am analyzing and testing 32bit process and 64bit process built from same source code.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: fix v1

Comment 4

[Makoto Kato [:m_kato]]

Attachment: fix v1.1

Comment 5

[Josh Matthews [:jdm]]

>+#if !(defined(OS_MACOSX) || defined(OS_WIN) || (defined(CHROMIUM_MOZILLA_BUILD) && defined(OS_LINUX) defined(ARCH_CPU_64_BITS)))

This line is certainly not going to work as intened; look at the last two conditions.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 507805 [details] [diff] [review]
fix

We already overcame this obstacle with mixed-architecture mac OOPP.

  bool WriteIntPtr(intptr_t value) {
    // Always written as a 64-bit value since the size for this type can
    // differ between architectures.
    return WriteInt64(int64(value));
  }

// Always written as a 64-bit value since the size for this type can
// differ between architectures.
bool Pickle::ReadIntPtr(void** iter, intptr_t* result) const {

Comment 8

[Makoto Kato [:m_kato]]

On Mac OS X, (In reply to comment #7)
> Comment on attachment 507805 [details] [diff] [review]
> fix
> 
> We already overcame this obstacle with mixed-architecture mac OOPP.

Chris, Do you say that ReadParam(aMsg, aIter, PRUint32) should be extracted to ReadIntPtr()?

struct ParamTraits<size_t> is never defined on Mac OS X.  So this issue isn't on Mac OS X.  On Mac OS X both 32bit and 64bit is extracted it to ReadUInt32().

xul!IPC::ParamTraits<nsACString_internal>::Read() calls ReadParam(aMsg, aIter, PRUint32)) in http://mxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h?mark=228-229#228.  So on Mac OS X (that can use OOPP) extracts it to ParamTraits<PRUint32>::Read(m, iter, PRUint32).   But on Windows x86, it extracts to ParamTraits<void *>::Read(m, iter, void*)....

Comment 9

[Makoto Kato [:m_kato]]

On Windows x86, struct ParamTraits<uint32> uses ReadSize()

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, I misunderstood what you were trying to fix.

Yeah, saving a few bytes here sounds fine.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 507805 [details] [diff] [review]
fix

r=me assuming it's green on tryserver ;).  This code is very fragile.

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 507805 [details] [diff] [review]
fix

pass tests on try server

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/302723303f42