116976 - nsFrame::GetBidiProperty is bad for Big Endian machine

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Frank Tang]

when I debug MacOS X hebrew support with smontagu, we find out one major problem
with bidi code. The nsFrame::GetBidiProperty and SetBidiProperty pair is bad for
Big Endian machinese:

http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsFrame.cpp#4104
NS_IMETHODIMP nsFrame::GetBidiProperty(nsIPresContext* aPresContext,
                                  nsIAtom*        aPropertyName,
                                  void**          aPropertyValue,
                                  size_t          aSize) const
{
  if (!aPropertyValue || !aPropertyName) {
    return NS_ERROR_NULL_POINTER;
  }
  if ( (aSize < 1) || (aSize > sizeof(void*) ) ) {
    return NS_ERROR_ILLEGAL_VALUE;
  }

  nsCRT::zero(aPropertyValue, aSize);
  void* val = nsnull;

  nsCOMPtr<nsIPresShell> presShell;
  aPresContext->GetShell(getter_AddRefs(presShell) );

  if (presShell) {
    nsCOMPtr<nsIFrameManager> frameManager;
    presShell->GetFrameManager(getter_AddRefs(frameManager) );

    if (frameManager) {
      frameManager->GetFrameProperty( (nsIFrame*)this, aPropertyName, 0, &val);
      if (val) {
        nsCRT::memcpy(aPropertyValue, &val, aSize);
      }
    }
  }
  return NS_OK;
}

NS_IMETHODIMP nsFrame::SetBidiProperty(nsIPresContext* aPresContext,
                                  nsIAtom*        aPropertyName,
                                  void*           aPropertyValue) 
{
  nsresult rv = NS_ERROR_FAILURE;

  nsCOMPtr<nsIPresShell> shell;
  aPresContext->GetShell(getter_AddRefs(shell) );
  if (shell) {
    nsCOMPtr<nsIFrameManager> frameManager;
    shell->GetFrameManager(getter_AddRefs(frameManager) );
    if (frameManager) {
      rv = frameManager->SetFrameProperty( (nsIFrame*) this, aPropertyName,
aPropertyValue, nsnull);
    }
  }
  return rv;
}

What happen is when the aSize is < 4 and in the big endian machine. The
SetProperty will just copy size of (void*) to the hash table. But the
GetBidiProperty will copy the beginning aSize and return.
In the little endian machine, it is not a big deal. But if the return value is
0x00000001 and on big endian machine and ask for 1 byte return. The
GetBidiProperty will copy the 00 return instead of 0x01 return. Therefore, on
MacOS, the embedding level is always 0.

Comment 1

[Frank Tang]

I think this will always cause problem on other machines, such as AIX or
Solaris. As long as it is big endian machine. 

Comment 2

[Frank Tang]

reassign to self

Comment 3

[Frank Tang]

Attachment: patch, version 1. Maybe there are better way to solve this.

Comment 4

[Frank Tang]

ok. this patch compiled and run fine on MacOS X
compiled on Win and Linux. (not run time change for little endian macine)
mkaply can kataki, can you try this patch on AIX and big endian Solaris?

Comment 5

[Frank Tang]

Attachment: v1.1, same as v1.0 but fix CR LF problem in the patch

Comment 6

[Frank Tang]

mkply: can r= this one ?

Comment 7

[Shanjian Li]

Attachment: modified patch

Comment 8

[Frank Tang]

shanjian: my patch let compiler do the magic since it is the compiler which cast
the PRInt8 it void* eariler. 
Your patch probably will also work under big endian. What will happen to 64 bits
machine ? 

Comment 9

[Frank Tang]

shanjian send the following thorugh private email:
>Frank,
>
>I tried this on my HPUX box, and it does not compile. HPUX does not allow hard 
>cast from void* to PRUint8 and PRUint16. I modified the patch and attached to
bug >report to save a round of email.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 62884 [details] [diff] [review]
modified patch

The SetBidiProperty interface is broken.  It should take an aSize parameter and
a *pointer* to an integral type, and copy that many bytes from the given
address into whatever intermediate or internal buffer it needs (in this case, a
void* that's passed to SetFrameProperty).

As a quick fix, shanjian's patch is ok, but I'd rather see the interface
methods made symmetric.  There are only a few file's worth of a few calls per
file to SetBidiProperty.

/be

Comment 11

[Frank Tang]

I believe the following three patch should fix most mac os hewbrew problem

big endian issue in nsFrame.cpp
see latest patch in http://bugzilla.mozilla.org/show_bug.cgi?id=116976

big endian issue in nsBidiImp.cpp 
http://bugzilla.mozilla.org/attachment.cgi?id=62917&action=view

turn off hebrew reordering and do not report hint on mac gfx
http://bugzilla.mozilla.org/attachment.cgi?id=62920&action=view

Comment 12

[Simon Fraser [no longer active]]

What are the different BiDi properties that are being read and set here? Are they 
always 32 bits or less? Why not just get/set 32-bit values, rather than void*, 
and do the up/down cast in the calling code?

Comment 13

[Frank Tang]

it could be a nsIFrame*. It could be > 32 bits on a 64 bits machine.
Basically, I agree with you, I think we could always get a (void*) , not a 32
bits. and let the caller cast it down to what ever it shoudl be. 
Let's check in shanjian's patch and fix the api on a seperate bug. see bug 117751

Comment 14

[Frank Tang]

could we check in
http://bugzilla.mozilla.org/attachment.cgi?bugid=116976&action=enter for now and
fix the api issue in bug 117751?

Comment 15

[Frank Tang]

Comment on attachment 62884 [details] [diff] [review]
modified patch

r=ftang

Comment 16

[Frank Tang]

jdunn, can you try to compile on those big endian machines you have ?

Comment 17

[Frank Tang]

kataki- do you have big endian machine (solaris?)

Comment 18

[Frank Tang]

the patch compiled on Solaris w/o problem

Comment 19

[Roland Mainz]

I can test this on Solaris/SPARC if someone can explain me how I can verify this
...

BTW: Why are we using nsCRT::memcpy() instead of ANSI C memcpy() (this destroys
the optimisations some compiler/OSes can do when using their "built-in" memcpy()
versions) ?

Comment 20

[Frank Tang]

It compiled on HPUX

Comment 21

[Frank Tang]

>BTW: Why are we using nsCRT::memcpy() instead of ANSI C memcpy() (this destroys
>the optimisations some compiler/OSes can do when using their "built-in" memcpy()
>versions) ?

We should always use nsCRT if possible in xp code. If Compiler/OS can do with
their build-in memcpy() can we fix that in nsCRT::memcpy() ?

Comment 22

[Frank Tang]

>I can test this on Solaris/SPARC if someone can explain me how I can verify this

thanks, I already verify it on Solaris.
It should impact the correctness of Hebrew/Arabic display . Hard to explain.

Comment 23

[Roland Mainz]

ftang wrote:
> We should always use nsCRT if possible in xp code. If Compiler/OS can do with
> their build-in memcpy() can we fix that in nsCRT::memcpy() ?

Not really. The built-in compiler memcpy() requires to be |inline| because they
look for common optimisations (small fixed[1] size (inline long word block
copies), large fixed size (which calls specfic OS routine for large mem copies,
alignment optimisations etc. etc)

[1]=("fixed" = either constant fixed value or the compiler can figure out that
the variable may only contain a known value/expresson which can be used for
further optimisation).

I would vote to implement a PL_memcpy() which should be a macro to memcpy() on
platforms where memcpy() is not available (is there any ?) ...

Comment 24

[Brendan Eich [:brendan]]

I don't believe we need PL_strcpy, nsCRT::memcpy, etc. any longer.  The
rationale for these is almost lost in the mists of time (I dimly recall an HP-UX
memcpy bug from 1996).  jdunn, do you agree?

I see lots of Mozilla code using memcpy, strlen, etc. and porting as well as any
other code.  I see on reason to rub these hold-over "portability worry beads"
any longer.  Sorry I didn't say this earlier.

/be

Comment 25

[Jim Dunn]

I can't recall the memcpy issue from '96.
The lastest patch does look ok, however
you should get Shanmu to look at this on Tru64
(unless someone else has done a 64bit build?!?)
However, it looks ok to me.  Shanmu, can you
try to make sure the latest patch compiles on
Tru64?  I have no idea how to test it...

I also agree with Brendan maybe it is time we
go thru and look at removing nsCRT::memcpy's
but if we do, we should do that as a separate
issue since it is going to consume a bunch of 
time (I assume we will do performance analysis
on it as well)

Comment 26

[Frank Tang]

Attachment: updated patch ues ::memcpy directly

Comment 27

[Roland Mainz]

Comment on attachment 63501 [details] [diff] [review]
updated patch ues ::memcpy directly

r=[e-mail]

Comment 28

[Jim Dunn]

I am confused.  What is the significance of using 
::memcpy and NOT memcpy?

Comment 29

[Roland Mainz]

|::memcpy()| requests explicitly a "_mempy" symbol from the (flat) ANSI-C
namespace.
|memcpy| can be anything... function, macro... what you want (or what the
compiler can offer... :) ...

Comment 30

[Shanmugavelu Shanmuganathan (gone)]

The latest patch compiled on a Tru64 UNIX machine.

Comment 31

[Brendan Eich [:brendan]]

jdunn: agreed, we can leave existing nsCRT::memcpy usage, but I don't see any
reason to add new uses.

sr=[e-mail] contingent on successful Tru64 testing (code looks good
to me, but test platform coverage is key here).

/be

Comment 32

[Jim Dunn]

Ok, but for the record I don't want to keep ANY instances
of nsCRT::memcpy around.  Lets open another bug and use
that one to clean them up (I think that is the consensus.

Comment 33

[Brendan Eich [:brendan]]

Comment on attachment 63501 [details] [diff] [review]
updated patch ues ::memcpy directly

jdunn: sorry I was unclear, I meant we don't need to hassle with
nsCRT::memcpy=>memcpy in existing code touched by this or another bug that's
already on file, we can use the new bug ftang kindly filed to do the cleanup,
as you say.

ftang: thanks for filing that!

sr=[e-mail].

/be

Comment 34

[Brendan Eich [:brendan]]

jdunn: ::memcpy is pure C++ for "call the standard memcpy in the global
namespace, which must conform to the standard."  Calling memcpy (no ::) might
call some other method if we had a default namespace that was not the global or
standard one, or if someone added memcpy to the class whose method contains the
call, or whatever.

Since it would be evil, and prolly break lots of code, for anyone to name any
method memcpy other than the standard global function, this is more a
purity-of-essence or style issue.  It's ok with me to call memcpy either way,
but I defer to the C++ experts (cc'ing dbaron).

/be

Comment 35

[Brendan Eich [:brendan]]

Roland: ::memcpy does *not* explicitly request a _memcpy symbol from anywhere,
AFAIK.  I believe you are confusing linker implementation details on Unix-like
systems with the C++ standard.

Please correct me if I am wrong with something from the standard, and (better)
with real-world compilers that treat ::memcpy differently from memcpy (not that
macros expand even when their calls are preceded by punctuation such as ::).

/be

Comment 36

[Frank Tang]

let's keep the memcpy discussion on bug 118135 please.

Comment 37

[Frank Tang]

fixed and check in.

Comment 38

[Jerry]

according to the discuss of bug 118135
the fix can change ::memcpy to memcpy

Comment 39

[Jerry]

In my solaris platform, I apply this patch, it works .
but when I change the patch as below
#if IS_BIG_ENDIAN
       nsCRT::memcpy(aPropertyValue, ((char*)&val)+sizeof(void*) - aSize, 
aSize);
#else
       nsCRT::memcpy(aPropertyValue, &val, aSize);
#endif
it works too.

So I wonder should we change all nsCRT::memcpy to memcpy?
Is it useful? 

Comment 40

[Prabhat Hegde]

Patch works for me on Solaris.