1305422 - remove size_{forward,backward} and other similarly useless things from ns{Reading,Writing}Iterator

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Nathan Froyd [:froydnj]]

ns{Reading,Writing}Iterator are strange beasts insofar as they know how much farther iteration can proceed before there is no more string to iterate over.  This behavior is very unusual, and there are even comments that this behavior stems from when we had fragmented strings and iterators to match.

ns{Reading,Writing}Iterator are also less powerful versions of mozilla::RangedPtr, so we should try to move them over to the more modern alternative.  Getting rid of size_{forward,backward} is a good step in that direction.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - remove normalize* methods from ns{Reading,Writing}Iterator

They are never called.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - don't call size_forward on ns*String::iterators to check for SetLength failure

We have better ways of checking for SetLength failure nowadays, and even
if these SetLength calls did fail, the program would crash anyway.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - don't call size_forward in nsTStringComparator.cpp

Asking for size_forward on an iterator that you haven't started reading
from is just asking for the length of the original string, so use that
instead.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - don't call size_forward in nsIDNService.cpp

We don't need to bother with iterators here at all; we can just ask the
string for its data and length directly.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 5 - simplify Is8bit

The implementation of Is8bit, with its multiply-nested loops, dates from
the time when string iterators could be fragmented into multiple pieces.
We no longer have such iterators, so we can write Is8bit much more
straightforwardly, with the single loop you would expect.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 6a - add operator- support for ns{Reading,Writing}Iterator

This change is necessary so we can start using a pair of
iterators (current, end) and subtract them to figure out how far we have
left to go.  The current code just uses size_forward and size_backward
for this purpose, and it's quite an unusual iterator that knows how far
it can go until it's done.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 6 - pass explicit end iterators for CopyTo{Upper,Lower}Case

This change avoids calling size_forward on the iterator, which we are
trying to eliminate.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 7 - simplify nsXMLContentSerializer::SerializeAttr

The implementation of SerializeAttr, with its multiply-nested loops,
dates from the time when string iterators could be fragmented into
multiple pieces.  We no longer have such iterators, so we can write
SerializeAttr much more straightforwardly.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 8 - make nsSMILParserUtils::CheckForNegativeNumber more idiomatic

The surrounding code in this file and elsewhere uses pairs of RangedPtrs
for iterators.  While we're trying to modify nsString's iterators so
they're more like RangedPtr, it seems reasonable to go ahead and modify
this instance of nsString iterators to use RangedPtr directly for
conformity with the surrounding code.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 9 - remove {start,end} from ns{Reading,Writing}Iterator

These methods are now unused thanks to previous patches.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 10 - don't call size_forward in nsHTMLContentSerializer.cpp

See comment 0 for the rationale on why we want to remove size_forward calls.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 11 - don't call size_forward in nsXPCOMStrings.cpp

See comment 0 for the rationale on why we want to remove size_forward calls.

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 12 - don't call size_forward in {Body,Fetch}Util.cpp

See comment 0 for the rationale on why we want to remove size_forward calls.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: part 13 - don't call size_forward in nsXMLContentSerializer.cpp

See comment 0 for the rationale on why we want to remove size_forward calls.

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: part 14 - don't call size_forward in nsDefaultURIFixup.cpp

See comment 0 for the rationale on why we want to remove size_forward calls.

Comment 16

[Nathan Froyd [:froydnj]]

Attachment: part 15 - don't call size_forward in nsPKCS12Blob

nsCString::const_iterator and friends currently hold pointers to not
only the current iterator position, but also the start and end of the
string.  This functionality enables them to compute their position from
the start and the end of the string, respectively, but we have better
ways of accomplishing the same thing (e.g. mozilla::RangedPtr), and it's
a bit unusual for iterators to know their current position anyway.  So
let's make nsPKCS12Blob, when reading digests, hold pointers to both the
current iterator position and the end iterator position, and calculate
the remaining amount to read from those two pieces of information.

I apologize in advance for modifying this grotty code.  I think I could have
written this patch more cleanly, but then it would have been mixed up between
changes and cleanup, and I think it would have been harder to review.

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: part 16 - don't call size_backward in nsExceptionHandler.cpp

See comment 0 for the rationale on why we want to remove size_forward calls.

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: part 17 - remove size_{forward,backward} from ns{Reading,Writing}Iterator

Victory!

Comment 19

[Nathan Froyd [:froydnj]]

Attachment: part 18 - remove #if 0'd-out code from nsStringIterator.h

We don't support IBM VisualAge for OS/2 anymore, and we haven't needed
this code in a long time regardless.

Comment 20

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8794845 [details] [diff] [review]
part 15 - don't call size_forward in nsPKCS12Blob

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

I would actually prefer to remove the code that calls this entirely since it's completely unnecessary (NSS has this functionality built-in). I wanted to prove to myself that this would work, so I wound up just writing the patch. I'll attach it shortly.

Comment 21

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: part 15 - don't call size_forward in nsPKCS12Blob (code removal version)

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8794928 [details] [diff] [review]
part 15 - don't call size_forward in nsPKCS12Blob (code removal version)

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

Fantastic, thank you!

Comment 23

[Eric Rahm [:erahm]]

Comment on attachment 8794835 [details] [diff] [review]
part 6a - add operator- support for ns{Reading,Writing}Iterator

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

::: xpcom/string/nsStringIterator.h
@@ +131,5 @@
>    }
> +
> +  difference_type operator-(const self_type& aOther)
> +  {
> +    MOZ_ASSERT(mPosition >= aOther.mPosition);

I'm not sure we should be asserting here. We return |difference_type| (aka ptrdiff_t) which is signed, so that implies we allow for negative distances.

I *could* see an argument for asserting that the iterators are working on the same string, something like
MOZ_ASSERT(mStart == aOther.mStart && mEnd == aOther.mEnd) but that seems a bit gratuitous and I'm guessing we want to get rid of mStart and mEnd anyhow.

@@ +267,5 @@
>    }
> +
> +  difference_type operator-(const self_type& aOther)
> +  {
> +    MOZ_ASSERT(mPosition >= aOther.mPosition);

ditto

Comment 24

[Eric Rahm [:erahm]]

Comment on attachment 8794847 [details] [diff] [review]
part 17 - remove size_{forward,backward} from ns{Reading,Writing}Iterator

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

\o/

Comment 25

[Nathan Froyd [:froydnj]]

Attachment: part 16 - don't call size_backward in nsExceptionHandler.cpp

Didn't code this up right the first time, let's try again.

Comment 26

[Nathan Froyd [:froydnj]]

Attachment: part 6a - add operator- support for ns{Reading,Writing}Iterator

Version 2 of this patch adds some rationale for why we do what we do and
changes the result type of operator-.

Comment 27

[Eric Rahm [:erahm]]

Comment on attachment 8795018 [details] [diff] [review]
part 6a - add operator- support for ns{Reading,Writing}Iterator

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

Looks good, you can also get rid of the uint32_t casts in part 6 now I think.

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 8794996 [details] [diff] [review]
part 16 - don't call size_backward in nsExceptionHandler.cpp

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

I don't think we actually have a test for this, that might be useful. You could shoehorn it in here:
https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/toolkit/crashreporter/test/unit/test_crashreporter_crash.js#34

We have tests for bad chars in keys, but not for the escaping we do for values:
https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/toolkit/crashreporter/test/unit/test_crashreporter.js#47

Unrelated, but I wish I had just made the .extra file JSON when I originally wrote this code.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77a8d7d6a0fb
part 1 - remove normalize* methods from ns{Reading,Writing}Iterator; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/8313868b4334
part 2 - don't call size_forward on ns*String::iterators to check for SetLength failure; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e208dfe24c94
part 3 - don't call size_forward in nsTStringComparator.cpp; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b04e48b3e8
part 4 - don't call size_forward in nsIDNService.cpp; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c06cb170aa8
part 5 - simplify Is8bit; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b814ee26cd7
part 6a - add operator- support for ns{Reading,Writing}Iterator; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc21248ab12
part 6 - pass explicit end iterators for CopyTo{Upper,Lower}Case; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/caad2bb558f6
part 7 - simplify nsXMLContentSerializer::SerializeAttr; r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6e2e1c4f2a
part 8 - make nsSMILParserUtils::CheckForNegativeNumber more idiomatic; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/55be43ac96c9
part 9 - remove {start,end} from ns{Reading,Writing}Iterator; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b8ba9f7a51
part 10 - don't call size_forward in nsHTMLContentSerializer.cpp; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/088bfcd8f3cb
part 11 - don't call size_forward in nsXPCOMStrings.cpp; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/17a8607397e5
part 12 - don't call size_forward in {Body,Fetch}Util.cpp; r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/b26623101a0d
part 13 - don't call size_forward in nsXMLContentSerializer.cpp; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8f15fc02cfc
part 14 - don't call size_forward in nsDefaultURIFixup.cpp; r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/daf738c4a411
part 15 - don't call size_forward in nsPKCS12Blob; r=mgoodwin,nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad96d4c9d42
part 16 - don't call size_backward in nsExceptionHandler.cpp; r=ted.mielczarek
https://hg.mozilla.org/integration/mozilla-inbound/rev/f933f6215b9d
part 17 - remove size_{forward,backward} from ns{Reading,Writing}Iterator; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c685a95ab0
part 18 - remove #if 0'd-out code from nsStringIterator.h; r=erahm

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3caff20982f8
followup - use correct check for memory allocation failure in nsWindowsRegKey.cpp; r=bustage

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77a8d7d6a0fb
https://hg.mozilla.org/mozilla-central/rev/8313868b4334
https://hg.mozilla.org/mozilla-central/rev/e208dfe24c94
https://hg.mozilla.org/mozilla-central/rev/a5b04e48b3e8
https://hg.mozilla.org/mozilla-central/rev/1c06cb170aa8
https://hg.mozilla.org/mozilla-central/rev/9b814ee26cd7
https://hg.mozilla.org/mozilla-central/rev/9dc21248ab12
https://hg.mozilla.org/mozilla-central/rev/caad2bb558f6
https://hg.mozilla.org/mozilla-central/rev/8c6e2e1c4f2a
https://hg.mozilla.org/mozilla-central/rev/55be43ac96c9
https://hg.mozilla.org/mozilla-central/rev/32b8ba9f7a51
https://hg.mozilla.org/mozilla-central/rev/088bfcd8f3cb
https://hg.mozilla.org/mozilla-central/rev/17a8607397e5
https://hg.mozilla.org/mozilla-central/rev/b26623101a0d
https://hg.mozilla.org/mozilla-central/rev/e8f15fc02cfc
https://hg.mozilla.org/mozilla-central/rev/daf738c4a411
https://hg.mozilla.org/mozilla-central/rev/8ad96d4c9d42
https://hg.mozilla.org/mozilla-central/rev/f933f6215b9d
https://hg.mozilla.org/mozilla-central/rev/c8c685a95ab0
https://hg.mozilla.org/mozilla-central/rev/3caff20982f8