Constructing an nsTDependentString from an nsTAString makes no sense

RESOLVED FIXED

Status

()

Core
String
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 489961 [details] [diff] [review]
Fix consumers
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #489961 - Flags: review?(dwitte)

Comment 2

7 years ago
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.
Attachment #489961 - Flags: review?(dwitte) → review+
(Assignee)

Comment 3

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

7 years ago
You could use Substring(), no?
(Assignee)

Comment 5

7 years ago
That only returns an nsTDependentSubstring. But it might be enough.
(Assignee)

Updated

7 years ago
Depends on: 610267
(Assignee)

Comment 6

7 years ago
Comment on attachment 489961 [details] [diff] [review]
Fix consumers

Pushed changeset aabde52014d5 to mozilla-central.
So is this fixed?  What's left to do here?
Whiteboard: not-ready-for-cedar
(Assignee)

Comment 8

7 years ago
Created attachment 522622 [details] [diff] [review]
Remove bogus constructor
Attachment #522622 - Flags: review?(dbaron)
(Assignee)

Updated

7 years ago
No longer depends on: 610267
Comment on attachment 522622 [details] [diff] [review]
Remove bogus constructor

r=dbaron
Attachment #522622 - Flags: review?(dbaron) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 525865 [details] [diff] [review]
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.
Attachment #525865 - Flags: review?(dwitte)

Comment 11

6 years ago
Comment on attachment 525865 [details] [diff] [review]
More fixes

I could review but for lack of time :)

This seems right up warhammer's alley!
Attachment #525865 - Flags: review?(dwitte) → review?(jones.chris.g)
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.
Attachment #525865 - Flags: review?(jones.chris.g) → review?(dbaron)
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
Attachment #525865 - Flags: review?(dbaron) → review+
(Assignee)

Comment 14

6 years ago
(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.
(Assignee)

Comment 15

6 years ago
Created attachment 527509 [details] [diff] [review]
Extra fixes
Attachment #527509 - Flags: review?(dbaron)
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.)
Attachment #527509 - Flags: review?(dbaron) → review-
(Assignee)

Comment 17

6 years ago
(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...
(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.
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.
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
(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.
(Assignee)

Comment 22

6 years ago
Created attachment 529318 [details] [diff] [review]
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.
Attachment #525865 - Attachment is obsolete: true
Attachment #527509 - Attachment is obsolete: true
Attachment #529318 - Flags: review?(dbaron)
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.
Attachment #529318 - Flags: review?(dbaron) → review+
(Assignee)

Comment 24

6 years ago
Comment on attachment 529318 [details] [diff] [review]
Combined fixes

As requested by dbaron just for the cookie changes.
Attachment #529318 - Flags: review?(jones.chris.g)
Attachment #529318 - Flags: review?(dwitte)
(Assignee)

Comment 25

6 years ago
Created attachment 535423 [details] [diff] [review]
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.
Attachment #489961 - Attachment is obsolete: true
Attachment #522622 - Attachment is obsolete: true
Attachment #529318 - Attachment is obsolete: true
Attachment #529318 - Flags: review?(jones.chris.g)
Attachment #529318 - Flags: review?(dwitte)
(Assignee)

Comment 26

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

Looks like I forgot to rerequest cookie service review.
Attachment #535423 - Flags: review?(dwitte)

Comment 27

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

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

r=dwitte
Attachment #535423 - Flags: review?(dwitte) → review+
(Assignee)

Comment 28

6 years ago
Pushed changeset 6f8b4709a508 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: not-ready-for-cedar
(Assignee)

Comment 29

6 years ago
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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 624738
You need to log in before you can comment on or make changes to this bug.