69468 - [xpcdom] xpconnect needs better support for null DOMStrings

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[John Bandhauer]

This bug is a continuation of the process of adding full support for DOMStrings
into xpconnect. We left off in bug 66610 with DOMString support working, but
without good support for null strings. There is a bunch of discussion back there
about that (and too many other things - hence the spinoff). Also see bug 67876
on the lowlevel string support needed. And bug 50602 discusses adding refcounted
string support.

Comment 1

[John Bandhauer]

adding shaver - he's currently hacking this stuff in xpconnect.

Comment 2

[John Bandhauer]

shaver has volunteered.

Note that (I think) jst *wants* something pretty iffy for the DOM use of this.
He wants to be able to to look at the AString result and detect that its 'null'
flag is set. But at the same time he wants the string to still contain the
literal "null" for cases where it is to be shown to the user.

Comment 3

[Brendan Eich [:brendan]]

Can someone cite w3c chapter and verse requiring null to be a string type value?
Or is it that some w3c-spec'd methods return either null or a valid string?  I
forget.

/be

Comment 4

[Johnny Stenback (:jst)]

The w3c specs say that functions that return strings can return null and w3c
specs also define how methods should behave if a string input argument is null
or not (yet in most cases we (the spec doesn't say this) also haveto carry over
value of the string that we got from doing the binding specific null to string
conversion, i.e. "null", "None", whatever).

Yet the w3c specs define strings in terms of corba idl as a sequence of unsigned
short, which according to corba IDL can *not* be null, ever, so go figure.

Comment 5

[Brendan Eich [:brendan]]

So is this just yet another unfiled erratum for the w3c to digest, or do we have
a compelling compatibility reason (DOM 0, 1, 2, IE, Opera, etc.) do handle two
types (null and string) in some cases?

/be

Comment 6

[Johnny Stenback (:jst)]

We do have compatibility reasons for doing this, IE deals with this "correctly",
and every DOM test-suite out there expects the implementation to deal with null
strings. I don't see IE changing their implementation and people do rely on it's
behavior, and this has been discussed in the DOM WG and it doesn't look like
it's changing in the spec.

Comment 7

[Scott Collins]

Attachment: fixes to buffer handles flags to help with NULL-ness

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So scc, can we put mFlags on nsBufferHandle, and not just the shared version? 
That would improve my quality of life.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This is going to happen on the xpcdom branch, so it's not critical for 0.9.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

So, scc, in 67876, you said:

> If you need more bits, or
> want flags for strings that aren't sharable, you now have the opportunity to
> do the right thing.

How does one go about adding the flags on strings that aren't sharable?  It
looks like only nsSharedBufferHandle<T> has the GetImplementationFlags method,
but I could be high.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Scott and I are discussing various solutions to this problem, but I've been
remiss in providing the document I promised, as a starting point to the
conversation, and he is going to be a-travelling for a few days.

So I'm going to implement support based on the current architecture (as checked
into the tree), and then revisit it when strings get generic nullness support. 
In addition, I hope to have a way to avoid double-or-worse wrapping of
JSStrings, which will require flags on all strings, and a way to tell which
external string type is associated with a given JSString.  That's _definitely_
on my list for 0.9.1, but I don't want to block the landing, so I'm pushing
ahead with the first phase, today.

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: null string and unwrapping support, plus tests

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The attached patch provides the ability to create null strings and test strings
for nullness, as well as code to avoid double-wrapping of the JS(C(JS)) form. 
And, just because it's you, some test code.

I don't actually use the null-string flag internally, but I provided it so that
people could use it to test their strings without going through XPConnect, which
is how the world will work when we get generic null-string support.  Maybe that
means I don't need nsIXPConnect::stringIsNull?

Avoiding the other flavour of double wrapping (C(JS(C)) requires a JS engine
tweak, for which I've filed bug 78100, to avoid needing to consult the hash
table every time.

I'm going to check this in on the xpcdom branch and remove the summary-flag once
I finish testing everything with a fully-current build, but I'll leave this bug
open as a place for review comments.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

For the love of...

This null stuff isn't going to work correctly at all, for the DOM case.  I
totally forgot about the WritableString magic.

It looks to me, from reading the code, that I'm going to have to provide a
WritableString implementation as well, which will be a pile of fun, and it looks
to me like nsAWritableString::SetNull is a wee bit, well, absent.  I should
really finish that document on string sharing so that scc can tell me what I
need to do to make this all not suck.  I think I'll go do that now.

Should I check this stuff in?  I'm thinking not, though the unwrapping stuff
would be nice.

Comment 15

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'm waiting on the null-and-flags scc-hacking that we decided on in our string
summit.  0.9.2, and dependent on that bug, when I figure out what it is.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Asa just told me that we shipped 0.9.2!  Who knew?

Comment 17

[carnold]

Johnny Stenbeck's message wrote:

We do have compatibility reasons for doing this, IE deals with 
this "correctly", and every DOM test-suite out there expects the 
implementation to deal with null strings. I don't see IE changing their 
implementation and people do rely on it's behavior, and this has been 
discussed in the DOM WG and it doesn't look like
it's changing in the spec.

MSXML does handle nulls strings when passed from C++, but I have not been able 
to be able to pass a null value into MSXML from JavaScript or Visual Basic 
since the variant to string coercion apparently throws an exception before the 
method is called.  

My feeling (as an outside observer) if both IE and Netscape have problems with 
null strings, the DOM WG would consider allowing empty strings as an 
alternative.

Comment 18

[chris hofmann]

->0.9.5 since we have run out of time

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think jst has a patch for this.  Unsetting milestone and handing over.

Comment 20

[Johnny Stenback (:jst)]

Attachment: Initial whack at fixing this

Comment 21

[Johnny Stenback (:jst)]

The patch I just attached makes an attempt at creating an implementation of
nsAString that is voidable so that the DOM code can communicate string voidness
to XPConnect to indicate string nullness. The voidable nsAString implementation
is a class that simply inherits from nsString and overrides the methods needed
for making the strings carry the voidness state correctly.

On a slightly unrelated note I'm thinking it's probably a good idea to make this
new string class inherit from nsAutoString in stead of nsString to reduce the
number of allocations that happen while executing code through XPConnect.
Allocating nsAutoStrings might seem silly, but I bet allocating the larger
nsAutoStrings and avoiding allocations for the string buffer (in a large number
of cases) is cheaper than allocating nsStrings and then also allocating the
string buffer for the string data. These are temporary objects that are
allocated and then deleted, so their size doesn't really matter.

Thoughts? Are we ok with creating a class in XPConnect that inherits from our
often changing nsString code?

Comment 22

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Comment 23

[John Bandhauer]

+    void SetIsVoid(PRBool aVoid)
+    {
+        mIsVoid = aVoid;
+
+        if (aVoid) {
+            Truncate();
+        }
+    }

could be more like...

        if (aVoid && !mIsVoid) {
            Truncate();
        }

        mIsVoid = aVoid;

Then you possibly avoid a truncate call and also avoid any possible wrong 
behavior from calling the base class's truncate on an object that is in a state 
where it will return PR_TRUE to IsVoid().

You can remove the line:
            static const NS_NAMED_LITERAL_STRING(sNullString, "null");

Otherwise it all looks good to me. As we discussed, I'd like the string guys to 
say if they like how you inherited from nsAutoString. And it wouuld be nice if 
shaver signed off on your changes to his xpcstring code.

Comment 24

[John Bandhauer]

Comment on attachment 54152 [details] [diff] [review]
Proposed fix.

sr=band (assuming you address the nits)

Comment 25

[Johnny Stenback (:jst)]

Attachment: Nits fixed.

Comment 26

[Johnny Stenback (:jst)]

Attachment: More nits fixed, and one allocated nsString was changed to nsAutoString

Comment 27

[Johnny Stenback (:jst)]

jband, the last patch changes one more nsString to nsAutoString, the string is
used for passing "undefined" and "" to native code, so the value will always fit
in an nsAutoString. Let me know if you don't like this change and I'll undo it.

Comment 28

[John Bandhauer]

Comment on attachment 54186 [details] [diff] [review]
More nits fixed, and one allocated nsString was changed to nsAutoString

sr=jband
The nit fixes look fine.
I just don't think it matters one way or the other on the added nsString change.
I set a breakpoint and I didn't see it get there much. Also,
note that the empty string does not cause nsString to allocate a buffer. So, it is only
for the "undefined" case that you are allocating the extra space of the nsAutoString's
internal buffer. Like I said, I don't think it matters one way or the other. You decide.

Comment 29

[jag (Peter Annema)]

Comment on attachment 54186 [details] [diff] [review]
More nits fixed, and one allocated nsString was changed to nsAutoString

r=jag.
The logic in your XPCVoidableString is close enough, though it will not transfer voidness when you're assigning a void XPCVoidableString into another XPCVoidableString.

Comment 30

[Johnny Stenback (:jst)]

Shaver, you wrote the XPCReadableJSStringWrapper code, could you have a look at
the XPCReadableJSStringWrapper changes (and the other changes too if you don't
mind) in the attached patch?

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, I'll give (overdue) review tomorrow, instead of Saturday morning cartoons.

Comment 32

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

You test for aString.IsEmpty() in NS_NewAtom, but not aString.IsVoid().  Is that
intentional?  Or is this change unrelated to null-string support?

Other that that quandry, r=shaver.  Looks great, thanks for finishing this off.

Comment 33

[Johnny Stenback (:jst)]

Shaver, the change to nsAtomTable is not directly related to string voidness, it
just fixes a crash when an atom is requested for an empty string that doesn't
have a string buffer.

Comment 34

[carnold]

I've been following bug 69468 and am glad to see that it appears to be coming 
to an end.  

When should the changes appear in the nightly builds?  

Will the nightly builds also include changes for methods (like nodeValue for 
Element,...) that have been returning empty strings so that they return null 
strings or do these patches only enable those other methods to be brought into 
conformance.  If so, would you like me to prepare a list of issues.

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

The nsAtomTable.IsEmpty causes an assert when SetCharsetLangGroup() does:

aCharSetInfo->mLangGroup = NS_NewAtom("");

Perhaps that should be "aCharSetInfo->mLangGroup = nsnull;"?
(nsFontMetricsGTK.cpp:2817)

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Actually, the code in SetCharsetLangGroup() probably doesn't need to set
mLangGroup at all, since we only enter the body if it's null, and that line of
code was if we failed the call to get a new mLangGroup.  Setting to nsnull may
relieve paranoia, though.

Before this change was checked in, did NS_NewAtom("") actually return an atom,
or nsnull?

Comment 37

[Johnny Stenback (:jst)]

Fix checked in on the trunk, leaving open in case we want this on the embedding
branch.

carnold, this fix should be in todays nightly, and some, but not all DOM null
string issues have been fixed in todays nightly.

Randell, nsAtomTable used to either crash or return a atom when passed an empty
string, depending on the string class. Asking for an atom for an empty string
seems wrong to me.

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

Well, the font code called it in at least two places with (char *) "" on purpose.
One in SetCharsetLangGroup(), the other in SetFontLangGroupInfo().  I have some
patches that I _think_ are correct, but I need to know what NS_NewAtom("")
returned before.

Comment 39

[Randell Jesup [:jesup] (needinfo me)]

Another unintentional NS_NewAtom("") from HTMLContentSink::AddAttributes(). 
nodeType is eHTMLTag_img.  Its parent is an eHTMLTag_td.  The attribute we were
trying to get the atom for was index 2 (of 5 attributes (0...4)).

Side comment: why does AddAttributes() do "k.Truncate(); k.Append(key);"? 
Wouldn't k.Assign(key) or equivalent make more sense?

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

BTW, that last one will segfault you - the code depends on getting an Atom back.

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

Since the checked-in fix causes a crash in the parser, adding 'crash' and upping
severity.

Comment 42

[R.K.Aa.]

crash filed as bug 106527

Comment 43

[Johnny Stenback (:jst)]

Attachment: New approach to dealing with atoms for empty strings

Comment 44

[Johnny Stenback (:jst)]

Comment on attachment 54960 [details] [diff] [review]
New approach to dealing with atoms for empty strings

David says r=dbaron

Comment 45

[Johnny Stenback (:jst)]

New atom table change checked in.

Comment 46

[Johnny Stenback (:jst)]

Pushing to 094 to get this off my 096 list.

Comment 47

[Judson Valeski]

going to have to minus this for 0.9.4.

Comment 48

[carnold]

I've just posted a report on the 20011103 nightly build against the
in-development W3C/NIST Level 1 test suite
http://lists.w3.org/Archives/Public/www-dom-ts/2001Nov/0011.html

The code behind DocumentFragment.nodeValue and Element.nodeValue still appears
to be returning "" instead of null as defined in the spec.

Comment 49

[Fabian Guisset]

See bug 6052. Basically in this bug we added the implementation of null strings,
but we need to convert the different DOM properties to use those null strings.
Not as easy as it appears though :-(

Comment 50

[Tobias B. Besemer [:BesTo] (QA)]

Can we set the target milestone to mozilla0.9.7 or above ?

Comment 51

[John Bandhauer]

I think this bug should be closed - the work to xpconnect it describes is done.
A new bug can be openned to specifically cover whatever DOM work remains.

Comment 52

[Tobias B. Besemer [:BesTo] (QA)]

John Bandhauer ([e-mail]):
I think I have not the right to close this bug.
Can you close this bug?

Comment 53

[Johnny Stenback (:jst)]

Closing, lets file separate bugs on the remaining DOM null string issues as they
araise.

Comment 54

[Phil Schwartau]

Marking Verified - 

Comment 55

[Tobias B. Besemer [:BesTo] (QA)]

thx :)