611503 - Constructing an nsTDependentString from an nsTAString makes no sense

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[[e-mail]]

nsTDependentString has a constructor that takes an nsTAString as a parameter. But an nsTAString isn't guaranteed to be null-terminated, so I don't see the point of this constructor.

Most consumers should be using TPromiseFlatString, except for nsCookieService, which should be using Substring, since it doesn't need a flat string.

nsCookieService is the only caller that passes in an unterminated string. The other callers all happen to already pass in null-terminated strings.

Comment 1

[[e-mail]]

Attachment: Fix consumers

Comment 2

[[e-mail]]

Comment on attachment 489961 [details] [diff] [review]
Fix consumers

>diff --git a/netwerk/cookie/nsCookieService.cpp b/netwerk/cookie/nsCookieService.cpp

>   // aHost may contain a leading dot; if so, strip it now.

Comment isn't especially correct anymore.

>-  nsDependentCString host(aHost);
>-  PRBool domain = !host.IsEmpty() && host.First() == '.';
>-  if (domain)
>-    host.Rebind(host.BeginReading() + 1, host.EndReading());
>+  PRBool domain = !aHost.IsEmpty() && aHost.First() == '.';

Make this a PRInt32, and maybe call it domainOffset?

r=dwitte.

Comment 3

[[e-mail]]

I also looked at the consumers of nsTDependentString(nsTString). There are three cases (WebGLContext::CompileShader, DOMWorkerErrorReporter, nsCSSValue::AppendToString) where this is just a no-op, so I guess it should be removed. But nsCookieService::SetCookieStringInternal has this weird loop where it calls SetCookieInternal which rebinds the nsDependentCString at each call. I guess this is a valid use of an nsTDependentString, but it looks odd. This would make it the only real consumer of nsTDependentString(nsTString).

Comment 4

[[e-mail]]

You could use Substring(), no?

Comment 5

[[e-mail]]

That only returns an nsTDependentSubstring. But it might be enough.

Comment 6

[[e-mail]]

Comment on attachment 489961 [details] [diff] [review]
Fix consumers

Pushed changeset aabde52014d5 to mozilla-central.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So is this fixed?  What's left to do here?

Comment 8

[[e-mail]]

Attachment: Remove bogus constructor

Comment 9

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

Comment on attachment 522622 [details] [diff] [review]
Remove bogus constructor

r=dbaron

Comment 10

[[e-mail]]

Attachment: More fixes

I came up with a bit of a cheesy fix for the other Cookie Service case. The review comments from the previous patch are included here because they didn't land with the patch. There are some other bogus uses of nsTDependentString, which I can get other reviews for if you don't feel you can review. The change from attachment 522622 [details] [diff] [review] is also included.

Comment 11

[[e-mail]]

Comment on attachment 525865 [details] [diff] [review]
More fixes

I could review but for lack of time :)

This seems right up warhammer's alley!

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 525865 [details] [diff] [review]
More fixes

I'm not a good reviewer for our string code.  Passing the baton.

Sorry for the lag.

Comment 13

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

Comment on attachment 525865 [details] [diff] [review]
More fixes

>-        const char *s = nsDependentCString(shader->Source()).get();
>+        nsPromiseFlatCString src(shader->Source());
>+        const char *s = src.get();

This is already flat; just do:
  const char *s = shader->Source().get();

>-  nsDependentCString cookieHeader(aCookieHeader);
>+  nsDependentCString cookieHeader(aCookieHeader.get(), aCookieHeader.Length());
>   while (SetCookieInternal(aHostURI, baseDomain, requireHostMatch,
>                               cookieStatus, cookieHeader, serverTime, aFromHttp));

This is the only caller of SetCookieInternal; just fix its parameter
type so you can pass aCookieHeader directly.

>     ResourceMapping resource = {
>-        nsDependentCString(aKey), uri
>+        nsCString(aKey), uri
>     };

If you can just write aKey here instead of nsCString(aKey), do so; otherwise
it's fine.


r=dbaron with that

Comment 14

[[e-mail]]

(In reply to comment #13)
>(From update of attachment 525865 [details] [diff] [review])
> >-        const char *s = nsDependentCString(shader->Source()).get();
> >+        nsPromiseFlatCString src(shader->Source());
> >+        const char *s = src.get();
> This is already flat; just do:
>   const char *s = shader->Source().get();
I honestly hadn't noticed that, since I'd just copied an earlier example from earlier in the same method! Should I fix that too while I'm there?

> >-  nsDependentCString cookieHeader(aCookieHeader);
> >+  nsDependentCString cookieHeader(aCookieHeader.get(), aCookieHeader.Length());
> >   while (SetCookieInternal(aHostURI, baseDomain, requireHostMatch,
> >                               cookieStatus, cookieHeader, serverTime, aFromHttp));
> This is the only caller of SetCookieInternal; just fix its parameter
> type so you can pass aCookieHeader directly.
SetCookieInternal does weird things to its header, and I'm not immediately sure that it's possible to do that. I'd have to look into it.

> >     ResourceMapping resource = {
> >-        nsDependentCString(aKey), uri
> >+        nsCString(aKey), uri
> >     };
> If you can just write aKey here instead of nsCString(aKey), do so; otherwise
> it's fine.
I did try that first, but it's an explicit constructor, so it doesn't work.

Comment 15

[[e-mail]]

Attachment: Extra fixes

Comment 16

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

Comment on attachment 527509 [details] [diff] [review]
Extra fixes

Code that constructs an nsDependentCString from an nsAString the way you do here in the cookie code is incorrect, since nsDependentCString requires null-termination and you don't have guaranteed null-termination.

Instead of adding a new NS_IsAscii, use the IsASCII in nsReadableUtils.h directly on the string.

(Also, it's a little unclear to me what's on top of the previous version of the patch and what's not.)

Comment 17

[[e-mail]]

(In reply to comment #16)
> (From update of attachment 527509 [details] [diff] [review])
> Code that constructs an nsDependentCString from an nsAString the way you do
> here in the cookie code is incorrect, since nsDependentCString requires
> null-termination and you don't have guaranteed null-termination.
I'm not sure what you mean here; there are only two uses of nsDependentCString in the patch, one existing one on char* aCookieHeader and one new one on nsCString aCookieString - note that's it not an nsAString.

> Instead of adding a new NS_IsAscii, use the IsASCII in nsReadableUtils.h
> directly on the string.
Ah, I never thought to look there. Thanks!

> (Also, it's a little unclear to me what's on top of the previous version of
> the patch and what's not.)
I thought that's what Bugzilla's interdiff was for...

Comment 18

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

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 527509 [details] [diff] [review])
> > Code that constructs an nsDependentCString from an nsAString the way you do
> > here in the cookie code is incorrect, since nsDependentCString requires
> > null-termination and you don't have guaranteed null-termination.
> I'm not sure what you mean here; there are only two uses of nsDependentCString
> in the patch, one existing one on char* aCookieHeader and one new one on
> nsCString aCookieString - note that's it not an nsAString.

Then just make the parameter type use nsCString instead of nsDependentCString, and don't construct anything.


> > (Also, it's a little unclear to me what's on top of the previous version of
> > the patch and what's not.)
> I thought that's what Bugzilla's interdiff was for...

Not if "Extra fixes" is a patch on *top* of "More fixes", which for the WebGL changes it looks like it is... either that, or you missed propagating a chunk in the WebGL changes.

Comment 19

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

The point being that if you want me to use interdiff (not bugzilla's, though), you should tell me which patches replace which earlier patches.  None of the patches in this bug have names that indicate that one is a replacement for another.

Comment 20

[[e-mail]]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (From update of attachment 527509 [details] [diff] [review])
> > > Code that constructs an nsDependentCString from an nsAString the way you do
> > > here in the cookie code is incorrect, since nsDependentCString requires
> > > null-termination and you don't have guaranteed null-termination.
> > I'm not sure what you mean here; there are only two uses of nsDependentCString
> > in the patch, one existing one on char* aCookieHeader and one new one on
> > nsCString aCookieString - note that's it not an nsAString.
> Then just make the parameter type use nsCString instead of nsDependentCString,
> and don't construct anything.
I can't do that because ParseAttributes calls Rebind on the dependent string.

> > > (Also, it's a little unclear to me what's on top of the previous version of
> > > the patch and what's not.)
> > I thought that's what Bugzilla's interdiff was for...
> Not if "Extra fixes" is a patch on *top* of "More fixes", which for the WebGL
> changes it looks like it is... either that, or you missed propagating a chunk
> in the WebGL changes.
Ah yes, I started work in one tree and finished in another and forgot to apply comment 13... unluckily for me.

Comment 21

[[e-mail]]

(In reply to comment #19)
> The point being that if you want me to use interdiff (not bugzilla's, though),
> you should tell me which patches replace which earlier patches.  None of the
> patches in this bug have names that indicate that one is a replacement for
> another.
I realise that I should have obsoleted the previous patch to make that clearer.

Comment 22

[[e-mail]]

Attachment: Combined fixes

OK, so this patch now definitely has all the fixes from the previous two patches and their review comments, noting that since ParseAttributes still needs to call Rebind I still create an nsDependentCString from an nsCString.

Comment 23

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

Comment on attachment 529318 [details] [diff] [review]
Combined fixes

r=dbaron on everything except the cookie changes, which I'm not comfortable reviewing.  If dwitte has reviewed those above, then you're fine; otherwise you should ask him to review them.

Comment 24

[[e-mail]]

Comment on attachment 529318 [details] [diff] [review]
Combined fixes

As requested by dbaron just for the cookie changes.

Comment 25

[[e-mail]]

Attachment: Updated for bitrot

So, there's a new fad for creating a dependent string from an abstract string, because it turns out that StringTail and 1-arg Substring don't let you create a terminated string from a terminated string. To fix those I thought the easiest way would be to allow nsDependentString to rebind and be constructed from the tail of an existing flat string, so that's what I've done here. I also added overrides for StringTail and Substring in case someone finds them useful.

Comment 26

[[e-mail]]

Comment on attachment 535423 [details] [diff] [review]
Updated for bitrot

Looks like I forgot to rerequest cookie service review.

Comment 27

[[e-mail]]

Comment on attachment 535423 [details] [diff] [review]
Updated for bitrot

Review of attachment 535423 [details] [diff] [review]:
-----------------------------------------------------------------

r=dwitte

Comment 28

[[e-mail]]

Pushed changeset 6f8b4709a508 to mozilla-central.

Comment 29

[[e-mail]]

Backout changeset f2a2adaaacba because Android also had some lame string handling (nsDependentString(NS_LTIERAL_STRING("Droid Sans")) anyone?)

Re-landed with Android fixes as changeset f1eef5e80caf.