1128588 - nsDependentCString copy constructor is broken

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1100398 I tried to implement the nsStringInputStream() copy constructor by simply using the nsDependentCString() copy constructor.  This worked in most of my tests, but then failed in the fetch data URL test.

In that case the nsStringInputStream was created with NS_NewCInputStream() using an nsAutoCString value.  Since the data was short, it was most certainly getting a stack based buffer.

In this case it appears the nsDependentCString copy constructor does not recognize that it needs to actually copy the data.  I suppose you could argue that this is correct for a "dependent" class, but it surprised me.

I fixed the nsStringInputStream by changing to use Assign() instead of the copy constructor.  Should we make the copy constructor do the same?  Or should we just delete the copy constructor?

Comment 1

[Benjamin Smedberg]

I don't understand. A dependent string is inherently tied to the lifetime of the data its wrapping. It sounds like you don't actually want a dependent string here, but just a nsString, which is an owning string type.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I didn't pick the nsDependentCSubstring type.  It was existing in nsStringInputStream.

Boris convinced me in IRC that I really just want Assign().

Comment 3

[Boris Zbarsky [:bzbarsky]]

So I do think there's a problem here.

Right now nsStringInputStream uses nsDependentCSubstring for its storage and sometimes uses Assign() on it (to implement setData/adoptData).  Which produces a nsDependentCSubstring that owns its data, and then copy-constructing another nsDependentCSubstring from it is just not going to be happy at all.

So why is nsStringInputStream using nsDependentCSubstring?

It needs a string type that can do the following:

1)  Have a no-arg public ctor, so you can have it as a member sanely.
2)  Able to either own or not own its buffer.
3)  Able to have a non-null-terminated buffer.

I don't believe we have such a string type right now.  nsDependentCSubstring kinda fails #2, in that it _can_ do it, but it doesn't claim to and if you do owning you get issues with the copy ctor.  nsCSubstring fails #1, I believe.  nsCString fails #3.

Am I just missing something?

Comment 4

[Benjamin Smedberg]

#2 doesn't make sense to me. Assuming we're not talking about literal strings, what kind of situation ends up being safe with a string that is either owning or non-owning? Is there some external ownership or context that makes this safe?

Reading the code, I'm guessing nsStringInputStream::ShareData, but that also feels inherently unsafe...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Of course it's unsafe.  It relies on the caller to make sure the data outlives the stream.

But yes, that is precisely the situation.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The string code is somewhat confused about dynamic vs. static typing.  The way I'd like to look at is is that the ownership model is dynamically typed (which is mostly the reality today, except that there are a bunch of types that default to different ownership models floating around and confusing people) but that there is static typing that guarantees null-termination (and, unlike the current code, actually guarantees it).

I'd really like to move the various actual methods on nsDependentString up to nsString, so that you can rebind a non-owned buffer on any nsString, and make nsDependentString, etc., really be only a special named constructor (or, even better, constructor function like Substring() with no named type at all) for nsString.

That's quite a bit of work, though, and not a priority for me.