[xpcdom] xpconnect needs better support for null DOMStrings

VERIFIED FIXED in mozilla0.9.4



17 years ago
16 years ago


(Reporter: John Bandhauer, Assigned: jst)



Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [HAVE FIX])


(7 attachments)



17 years ago
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.


17 years ago
Depends on: 67876


17 years ago
Target Milestone: --- → mozilla0.9

Comment 1

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

Comment 2

17 years ago
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.
Assignee: jband → shaver
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


Comment 4

17 years ago
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.
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?


Comment 6

17 years ago
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

17 years ago
Created attachment 30523 [details] [diff] [review]
fixes to buffer handles flags to help with NULL-ness
So scc, can we put mFlags on nsBufferHandle, and not just the shared version? 
That would improve my quality of life.

This is going to happen on the xpcdom branch, so it's not critical for 0.9.
Target Milestone: mozilla0.9 → mozilla0.9.1
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.
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.
Summary: xpconnect needs better support for null DOMStrings → [xpcdom] xpconnect needs better support for null DOMStrings
Created attachment 32570 [details] [diff] [review]
null string and unwrapping support, plus tests
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.
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.
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.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Asa just told me that we shipped 0.9.2!  Who knew?
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 17

16 years ago
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 


16 years ago
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Comment 18

16 years ago
->0.9.5 since we have run out of time
Target Milestone: mozilla0.9.4 → mozilla0.9.5
I think jst has a patch for this.  Unsetting milestone and handing over.
Assignee: shaver → jst
Target Milestone: mozilla0.9.5 → ---

Comment 20

16 years ago
Created attachment 53723 [details] [diff] [review]
Initial whack at fixing this

Comment 21

16 years ago
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?
Target Milestone: --- → mozilla0.9.6

Comment 22

16 years ago
Created attachment 54152 [details] [diff] [review]
Proposed fix.

Comment 23

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

could be more like...

        if (aVoid && !mIsVoid) {

        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

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

sr=band (assuming you address the nits)
Attachment #54152 - Flags: superreview+

Comment 25

16 years ago
Created attachment 54184 [details] [diff] [review]
Nits fixed.


16 years ago
Attachment #54184 - Flags: superreview+

Comment 26

16 years ago
Created attachment 54186 [details] [diff] [review]
More nits fixed, and one allocated nsString was changed to nsAutoString

Comment 27

16 years ago
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.
Whiteboard: [HAVE FIX]

Comment 28

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

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.
Attachment #54186 - Flags: superreview+


16 years ago
Blocks: 105405

Comment 29

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

The logic in your XPCVoidableString is close enough, though it will not transfer voidness when you're assigning a void XPCVoidableString into another XPCVoidableString.
Attachment #54186 - Flags: review+

Comment 30

16 years ago
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?
Yeah, I'll give (overdue) review tomorrow, instead of Saturday morning cartoons.
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

16 years ago
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

16 years ago
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.
The nsAtomTable.IsEmpty causes an assert when SetCharsetLangGroup() does:

aCharSetInfo->mLangGroup = NS_NewAtom("");

Perhaps that should be "aCharSetInfo->mLangGroup = nsnull;"?
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

16 years ago
Fix checked in on the trunk, leaving open in case we want this on the embedding

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.
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.
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?
BTW, that last one will segfault you - the code depends on getting an Atom back.
Since the checked-in fix causes a crash in the parser, adding 'crash' and upping
Severity: normal → critical
Keywords: crash

Comment 42

16 years ago
crash filed as bug 106527

Comment 43

16 years ago
Created attachment 54960 [details] [diff] [review]
New approach to dealing with atoms for empty strings

Comment 44

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

David says r=dbaron
Attachment #54960 - Flags: review+

Comment 45

16 years ago
New atom table change checked in.
Keywords: crash


16 years ago
Keywords: edt0.9.4

Comment 46

16 years ago
Pushing to 094 to get this off my 096 list.
Target Milestone: mozilla0.9.6 → mozilla0.9.4


16 years ago
Keywords: edt0.9.4 → edt0.9.4-

Comment 47

16 years ago
going to have to minus this for 0.9.4.

Comment 48

16 years ago
I've just posted a report on the 20011103 nightly build against the
in-development W3C/NIST Level 1 test suite

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

Comment 49

16 years ago
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 :-(
Can we set the target milestone to mozilla0.9.7 or above ?

Comment 51

16 years ago
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.
John Bandhauer (jband@netscape.com):
I think I have not the right to close this bug.
Can you close this bug?

Comment 53

16 years ago
Closing, lets file separate bugs on the remaining DOM null string issues as they
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 54

16 years ago
Marking Verified - 
thx :)


16 years ago
Keywords: topembed
You need to log in before you can comment on or make changes to this bug.