136756 - Strings requires unfrozen nsCRT class

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

Strings requiring unfrozen the nsCRT class is a problem for components, plugins
and embedders that want to work in or with multiple version of mozilla and not
have to reimplement the string code.  The idea is to have these clients link in
the string code so that they can have a "snapshot" of our string implementation.
 This allows the freedom to change the string classes in mozilla without worring
about backwards compatibly (assuming that no interfaces changes - string
interfaces are (or will be) frozen, right!).  Basically, the clients get a copy
of the concrete implementation of the string interfaces.

Having string require this unfrozen class will cause problems when nsCRT
changes.  Any change could break compatiblity with built clients.

Comment 1

[Doug Turner (:dougt)]

*** Bug 136755 has been marked as a duplicate of this bug. ***

Comment 2

[Doug Turner (:dougt)]

reconsider please.

Comment 3

[Doug Turner (:dougt)]

Attachment: Removes nsCRT v.1

Straightfoward patch which copies the nsCRT functionality into the string code.


There are a couple of issues with this patch with may be minor.  The first
where should we put the strlen(const PRUnichar* s) function.  I put it in
nsStr.cpp and extern'ed it where it was needed.

The second issues is why do we use nsCRT::ToUpper and nsCRT::ToLower instead of
the toupper and tolower?  In this patch, I just converted callers.  If this is
a problem, we can copy the nsCRT:: functions into this string code.

Comment 4

[Scott Collins]

hmmm, |nsCRT| should be deprecated and replaced.  No outside caller should be
allowed, let alone encouraged to use this API.  Strings (hopefully temporary)
use of it won't break compatibility if we don't freeze this API, becuase no use
of it occurs in |inline|s and therefor, the dependency is not exposed.

I strongly recommend we do not allow outside callers of the largely unnecessary
|nsCRT|.

Comment 5

[Doug Turner (:dougt)]

scc, you missed the point.  If I want to develop an xpcom component or plugin, I
can't use strings since it currently links to xpcom for nsCRT.  Since nsCRT is
not frozen, anyone that uses strings before we fix this bug will be broken when
we do fix this bug!

Comment 6

[Scott Collins]

uh... no.  Since the strings methods they use will be in the Mozilla/Netscape
binary, and as I said, no use of the |nsCRT| routines appears in any inline,
they _won't_ be broken by a later change to Strings internal dependencies. 
Maybe if this changed the structure of a string in their code, or changed the
composition of an inline.  But it doesn't.  So this does not break callers.

Comment 7

[Doug Turner (:dougt)]

strings and glue are being linked INTO a application as static libraries.  This
is so that they may use the string concrete classes.  Since the application is
taking a "snapshot" of the string library and embedding it into their code, they
are protected against change in the mainline trunk.  Now, to do this, we need to
make sure that all of the dependencies on XPCOM in both glue and strings are
totally frozen.  The only dependency is nsCRT.

Below is the window import listing of nsTestSample.exe (xpcom/sample).  

With the above change:

bash-2.05a$ dumpbin /imports *.exe
Microsoft (R) COFF Binary File Dumper Version 6.00.8168
Copyright (C) Microsoft Corp 1992-1998. All rights reserved.


Dump of file nsTestSample.exe

File Type: EXECUTABLE IMAGE

  Section contains the following imports:

    xpcom.dll
                40B0A0 Import Address Table
                40BFA0 Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                 840  NS_GetMemoryManager
                 85F  NS_ShutdownXPCOM
                 85D  NS_RegisterXPCOMExitRoutine
                 842  NS_InitXPCOM2

Without the change:

bash-2.05a$ dumpbin /imports *.exe
Microsoft (R) COFF Binary File Dumper Version 6.00.8168
Copyright (C) Microsoft Corp 1992-1998. All rights reserved.


Dump of file nsTestSample.exe

File Type: EXECUTABLE IMAGE

  Section contains the following imports:

    xpcom.dll
                42F45C Import Address Table
                42F238 Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                 874  ?strncmp@nsCRT@@SAHPBG0I@Z
                 870  ?strlen@nsCRT@@SAIPBG@Z
                 7F8  ?ToUpper@nsCRT@@SADD@Z
                 523  ?HashCode@nsCRT@@SAIPBGPAI@Z
                 522  ?HashCode@nsCRT@@SAIPBDPAI@Z
                 8AA  NS_ShutdownXPCOM
                 7ED  ?ToLower@nsCRT@@SADD@Z
                 88D  NS_InitXPCOM2

Comment 8

[Scott Collins]

Comment on attachment 82247 [details] [diff] [review]
Removes nsCRT  v.1

the |Compare1To1| fix is not symmetric: why not fix very positive numbers as
well?  You should be using |nsCharTraits<PRUnichar>::compare| in |Compare2To2|.
 In fact, in |Compare1To1| as well.

Ow!  An |extern| function declaration.	Evil.  You should probably include
nsCharTraits.h and use |nsCharTraits<PRUnichar>::length|.  It's really wrong to
write this function again.

It's too bad you had to copy the hash function.  There're to many copies of
this stuff already, and it denies guarantees of hash compatibility.  I don't
see any way around it though, without pulling the hash functions out into their
own library that everyone could use...	and that's way beyond the pale of this
bug.

Comment 9

[Scott Collins]

In case my grammar was ambiguous (and it was to me on re-reading), I meant:

  in |Compare1To1|  --use--> |nsCharTraits<char>::compare|
  in |Compare2To2|  --use--> |nsCharTraits<PRUnichar>::compare|

Comment 10

[Scott Collins]

I discussed this with dougt.  I'm going to update his patch with the suggestions
made by me and jag (since doug is pressed for time) as noted in the bug.  I will
attach to the bug as an update of his patch.  We'll go on from there.  Jag and I
both strongly support getting a good fix in for this bug asap.

Comment 11

[Scott Collins]

noting the bugs for which I am the driver

Comment 12

[Scott Collins]

Attachment: update of dougt's patch

I'm happy to debate whether I should have changed |strlen(some_char_ptr)| to
|nsCharTraits<char>::length(some_char_ptr)|.  I did it for symmetry, since I
_had_ to do it for all the |PRUnichar| cases.  I could be talked out of it,
though I predict it makes exactly the same code and it may lower the overall
surprise factor.

I didn't include jag's suggested fix for |nsCharTraits<PRUnichar>::compare|
because I don't understand why he included hand-rolled code.  He and I should
discuss this, then he can further update this patch, which builds and runs on
my machines-of-the-moment (MacOS X, Linux)

Comment 13

[Doug Turner (:dougt)]

This does removed the nsCRT dependency and does not create any new dependencies
on XPCOM.  Great work scc.

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 83074 [details] [diff] [review]
update of dougt's patch

nit: spacing off in the comment.  Maybe intentional?:

>       // The following cases are rare and survivable caller errors.
>       //  Two null pointers are equal, but any string, even 0 length
>       //  is greater than a null pointer.  It might not really matter,
>       //  but we pick something reasonable anyway.

Comment 15

[jag (Peter Annema)]

If you have for example PRUnichar(255) and PRUnichar(257), in a little endian
encoding those become |0xFF 0x00| and |0x01 0x01|. memcmp won't take the high
byte into account when comparing these two values and will thus say that
PRUnichar(255) is larger than PRUnichar(257). That's why I'm handrolling that loop.

Comment 16

[Brendan Eich [:brendan]]

diff -u please!  :-)

/be

Comment 17

[Scott Collins]

I hated the spacing, but that's what emacs gave me and wouldn't let me fix; back
to vi.  My next patch will be -u.  Jag's going to add the |#include "nsCRT.h"|s
required on windows and Linux, I'll update for Mac.

Comment 18

[Doug Turner (:dougt)]

thou shall not blame emacs.  send me your .emacs and I shall hack.

Comment 19

[jag (Peter Annema)]

Attachment: Updated to include ::compare fix and nsCRT.h includes

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 83109 [details] [diff] [review]
Updated to include ::compare fix and nsCRT.h includes

thanks for finding all of these.  r=dougt

Comment 21

[Doug Turner (:dougt)]

scc, are you happy with jag's last changes?  Happy enough to sr' them so that I 
can land these changes on the trunk?

Comment 22

[Scott Collins]

his last patch is missing linux and mac changes to include nsCRT.h in files that
need it (now that they don't get it from string).  I'm adding the linux and mac
(OS X/mach-o) includes now.  We'll need to get a Mac buddy to build on classic
or carbon to find the real mac specific fixes.  I (for some reason I'm still
trying to understand) can't build non-mach-o builds on my Mac.

Comment 23

[Scott Collins]

Attachment: more inserted |#include "nsCRT.h"|s; and more to go

there's more to go here, but I have to stop for now.  Attaching my work so far,
so that someone else can continue from here.

Comment 24

[Doug Turner (:dougt)]

Attachment: builds on linux, windows, mac classic, solaris

extra files that were touched:

mozilla/extensions/xmlextras/tests/TestXMLExtras.cpp
mozilla/extensions/p3p/src/nsP3PService.cpp
mozilla/extensions/interfaceinfo/src/iixprivate.h
mozilla/extensions/transformiix/source/base/Double.cpp

mozilla/xpfe/components/filepicker/src/nsFileView.cpp
mozilla/xpfe/components/xremote/src/XRemoteService.cpp

mozilla/gfx/src/mac/nsUnicodeMappingUtil.cpp
mozilla/gfx/src/mac/nsPrintOptionsX.cpp
mozilla/widget/src/mac/nsClipboard.cpp
mozilla/widget/src/mac/nsMacEventHandler.cpp
mozilla/widget/src/mac/nsMacWindow.cpp
mozilla/widget/src/mac/nsMenuX.cpp
mozilla/widget/src/mac/nsMimeMapper.cpp
mozilla/widget/src/mac/nsSound.cpp
mozilla/widget/src/mac/nsTextWidget.cpp
mozilla/embedding/browser/powerplant/source/CAppFileLocationProvider.cpp
mozilla/embedding/browser/powerplant/source/CBrowserApp.cp
mozilla/embedding/browser/powerplant/source/CBrowserShell.cpp
mozilla/embedding/browser/powerplant/source/CProfileManager.cpp
mozilla/embedding/browser/powerplant/source/CTextInputEventHandling.cpp
mozilla/xpfe/appshell/src/nsMacMIMEDataSource.cpp

Comment 25

[jag (Peter Annema)]

Hrm, and someone will have to do the same for the commercial builds...

Comment 26

[jag (Peter Annema)]

Comment on attachment 83594 [details] [diff] [review]
builds on linux, windows, mac classic, solaris

r=/sr=jag

Comment 27

[Doug Turner (:dougt)]

commerical is fine.  I got it covered.

waiting on scc for a r=.

Comment 28

[Scott Collins]

Comment on attachment 83594 [details] [diff] [review]
builds on linux, windows, mac classic, solaris

r=scc; hope you caught them all or it'll be a long night :-)

Comment 29

[Daniel (Leaf) Nunes]

um, doug?
+
+#ifndef nsCharTraits_h___
+#include "nsCharTratis.h"
+#endif

Is that a typo, or do we intentionally mispell nsCharTraits.h?

Comment 30

[Doug Turner (:dougt)]

leaf: good catch.  I will fix it up when the tree opens.

Comment 31

[timeless]

i already fixed that on trunk

Comment 32

[chris hofmann]

Comment on attachment 83594 [details] [diff] [review]
builds on linux, windows, mac classic, solaris

a=chofmann,dbaron,valeski for checkin to the 1.0 branch.

Comment 33

[Doug Turner (:dougt)]

Attachment: last mod to be checked into branch

Comment 34

[Doug Turner (:dougt)]

Checked into trunk and branch

Comment 35

[Cyril Elkaim]

I think there is a bug in DOMParser.cpp from this. In the ParseFromString method
in line :
  nsresult rv = ConvertWStringToStream(str, nsCRT::strlen(str),
getter_AddRefs(stream), &contentLength);
str is a PRUnichar * and strlen accept char*
Anyway I have a core dump each time I use this method from JS

Comment 36

[Doug Turner (:dougt)]

Cyril, 
File a seperate bug, attach a test case, and reference this bug.  closing again.