Last Comment Bug 611503 - Constructing an nsTDependentString from an nsTAString makes no sense
: Constructing an nsTDependentString from an nsTAString makes no sense
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
: 624738 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-11 15:56 PST by neil@parkwaycc.co.uk
Modified: 2011-11-06 13:20 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix consumers (3.56 KB, patch)
2010-11-11 15:59 PST, neil@parkwaycc.co.uk
dwitte: review+
Details | Diff | Review
Remove bogus constructor (984 bytes, patch)
2011-03-29 01:37 PDT, neil@parkwaycc.co.uk
dbaron: review+
Details | Diff | Review
More fixes (9.20 KB, patch)
2011-04-13 16:58 PDT, neil@parkwaycc.co.uk
dbaron: review+
Details | Diff | Review
Extra fixes (9.43 KB, patch)
2011-04-21 04:52 PDT, neil@parkwaycc.co.uk
dbaron: review-
Details | Diff | Review
Combined fixes (14.47 KB, patch)
2011-04-30 15:08 PDT, neil@parkwaycc.co.uk
dbaron: review+
Details | Diff | Review
Updated for bitrot (19.14 KB, patch)
2011-05-26 11:50 PDT, neil@parkwaycc.co.uk
dwitte: review+
Details | Diff | Review

Description neil@parkwaycc.co.uk 2010-11-11 15:56:36 PST
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 neil@parkwaycc.co.uk 2010-11-11 15:59:41 PST
Created attachment 489961 [details] [diff] [review]
Fix consumers
Comment 2 dwitte@gmail.com 2010-11-11 16:24:31 PST
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 neil@parkwaycc.co.uk 2010-11-12 13:15:05 PST
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 dwitte@gmail.com 2010-11-12 14:30:25 PST
You could use Substring(), no?
Comment 5 neil@parkwaycc.co.uk 2010-11-12 16:01:15 PST
That only returns an nsTDependentSubstring. But it might be enough.
Comment 6 neil@parkwaycc.co.uk 2011-03-25 04:47:15 PDT
Comment on attachment 489961 [details] [diff] [review]
Fix consumers

Pushed changeset aabde52014d5 to mozilla-central.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-28 09:47:10 PDT
So is this fixed?  What's left to do here?
Comment 8 neil@parkwaycc.co.uk 2011-03-29 01:37:49 PDT
Created attachment 522622 [details] [diff] [review]
Remove bogus constructor
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-29 19:18:59 PDT
Comment on attachment 522622 [details] [diff] [review]
Remove bogus constructor

r=dbaron
Comment 10 neil@parkwaycc.co.uk 2011-04-13 16:58:56 PDT
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.
Comment 11 dwitte@gmail.com 2011-04-14 10:38:02 PDT
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 2011-04-20 13:50:42 PDT
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-7 (review requests must explain patch) 2011-04-20 16:11:49 PDT
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 neil@parkwaycc.co.uk 2011-04-21 02:12:07 PDT
(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 neil@parkwaycc.co.uk 2011-04-21 04:52:59 PDT
Created attachment 527509 [details] [diff] [review]
Extra fixes
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-22 09:17:29 PDT
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 neil@parkwaycc.co.uk 2011-04-22 09:55:27 PDT
(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-7 (review requests must explain patch) 2011-04-22 12:13:15 PDT
(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-7 (review requests must explain patch) 2011-04-22 12:14:37 PDT
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 neil@parkwaycc.co.uk 2011-04-22 13:01:01 PDT
(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 neil@parkwaycc.co.uk 2011-04-22 13:06:40 PDT
(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 neil@parkwaycc.co.uk 2011-04-30 15:08:47 PDT
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.
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-05-19 10:33:11 PDT
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 neil@parkwaycc.co.uk 2011-05-24 05:05:53 PDT
Comment on attachment 529318 [details] [diff] [review]
Combined fixes

As requested by dbaron just for the cookie changes.
Comment 25 neil@parkwaycc.co.uk 2011-05-26 11:50:01 PDT
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.
Comment 26 neil@parkwaycc.co.uk 2011-07-14 04:09:56 PDT
Comment on attachment 535423 [details] [diff] [review]
Updated for bitrot

Looks like I forgot to rerequest cookie service review.
Comment 27 dwitte@gmail.com 2011-09-11 17:25:45 PDT
Comment on attachment 535423 [details] [diff] [review]
Updated for bitrot

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

r=dwitte
Comment 28 neil@parkwaycc.co.uk 2011-09-14 12:42:54 PDT
Pushed changeset 6f8b4709a508 to mozilla-central.
Comment 29 neil@parkwaycc.co.uk 2011-09-15 01:10:29 PDT
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.
Comment 30 neil@parkwaycc.co.uk 2011-11-06 13:20:57 PST
*** Bug 624738 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.