70083 - [API] work on |GetXXXFragment|

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect, P1)

Description

[Scott Collins]

The current |GetXXXFragment| routines API and requirements force sub-optimal
behavior for multi-fragment strings.  I have an ancient patch that shows what I
want; will attach anon.

Comment 1

[Scott Collins]

Targeting mozilla0.9.1, but this is one of the bugs I would like to pull in to
0.9 if I have time.

Comment 2

[Scott Collins]

Correcting this problem is the actual solution to bug #74709.

Comment 3

[Scott Collins]

*** Bug 74709 has been marked as a duplicate of this bug. ***

Comment 4

[Scott Collins]

re-targeting milestones, starting from a clean slate

Comment 5

[Scott Collins]

All planned string API fixes need to be in by mozilla1.0.

Comment 6

[Scott Collins]

Attachment: the new fragment machinery, this patch also address bug #70082

Comment 7

[Scott Collins]

The patch above is the real deal.  It addresses the concerns in this bug as well
as bug #70082.  This fixes the /Get(Read|Writ)ableFragment/ machinery.  The
machinery is now slightly more complicated for sub-class authors, but eliminates
all limitations (other than size-of-memory) on the complexity of concatenations,
substrings, iteration over multi-fragment strings, and combinations thereof. 
I've been working on this patch for some time now, bouncing ideas and
implementations off of jag and others.  It's ready for more formal review now,
though I'm still testing it.

Comment 8

[Scott Collins]

Still waiting on reviews from jag, saari, and waterson (who has tentatively 
rubber-stamped this change, I guess)

Comment 9

[jag (Peter Annema)]

Yeah, sorry about that. It looks good, but since you mentioned that this didn't
work yet as expected (you were hanging somewhere?), I was hoping to find an
updated patch soonish after you attached this one. I guess I'll go finish my
review.

Comment 10

[jag (Peter Annema)]

Index: public/nsSharableString.h
===================================================================
RCS file: /cvsroot/mozilla/string/public/nsSharableString.h,v
retrieving revision 1.5
diff -u -2 -r1.5 nsSharableString.h
--- public/nsSharableString.h   2001/06/29 12:46:51     1.5
+++ public/nsSharableString.h   2001/07/29 09:36:00
@@ -47,15 +47,13 @@
     public:
       typedef nsSharableString  self_type;
-      typedef PRUnichar         char_type;
-      typedef nsAString         string_type;

@@ -78,14 +77,14 @@
       typedef nsSharableCString self_type;
       typedef char              char_type;
-      typedef nsACString        string_type;

I guess you could remove the typedef for char_type here too then.

-----

+void
+nsASingleFragmentString::GetNextFragmentByIndex( const_fragment_type&
aResultFragment, int /*aIndex*/ ) const
+  {
+    aResultFragment.mEnd = aResultFragment.mStart;
+    aResultFragment.mStart = 0;
+  }

Call FaultNextFragment(aResultFragment);

This is actually causing problems with normalize_forward (crashing because
|first| is expected to point to the same thing as |last| but ends up pointing at
|0| when it faults).

-----

+nsDependentConcatenation::IsDependentOn( const abstract_string_type& aString )
const
   {
     return mStrings[0]->IsDependentOn(aString) ||
mStrings[1]->IsDependentOn(aString);
   }

|kFirstString| and |kLastString| instead of |0| and |1| resp.

-----

More in a bit.

Comment 11

[Scott Collins]

...and in fact that problem with |normalize_forward| was exactly the problem
that was stopping me.  I told you that you might discover it, and that it would
probably be obvious.  :-)  Making the corrections and testing them; a new patch
anon.

Comment 12

[jag (Peter Annema)]

So just trying to run mozilla, I run into an old problem :-)

nsXPIDLString foo; foo.get();

That get() doesn't RecalculateBoundaries any longer. It used to end up calling
|GetSharedBufferHandle|, now it goes through |BeginReading|.

Comment 13

[jag (Peter Annema)]

Well, I found the normalize_forward() problem's cause by reading, but I didn't
realize it was the cause, nor the extent of the problem, till I tried to run
Mozilla and ended up in the debugger :-)

Comment 14

[jag (Peter Annema)]

So overriding GetFragmentByIndex on nsXPIDLString and doing something similar to
GetSharedBufferHandler gets me further, now to an assertion that we're asking
for an index other than 0 on a single fragment string. Looking at that now (but
maybe I caused it with the previously mentioned "fix"?)

Comment 15

[jag (Peter Annema)]

Hrm. For some reasong it thinks an nsAutoString has two fragments.

Could this be because moving an iterator to the end of the first fragment ends
up causing FaultNextFragment to be called, thus giving the impression of a
second fragment?

Comment 16

[jag (Peter Annema)]

Yep, that's exactly it. Urgh. scc, HEEEEELP!

Comment 17

[jag (Peter Annema)]

Yay! A normalize_backward in nsDependentSubstring fixed that. Next I'm just
hanging somewhere after mozilla couldn't find some file. Pulling and building
clobber, maybe that'll clear this up.

Comment 18

[Scott Collins]

Attachment: (replaces patch above) ...everything we've learned so far

Comment 19

[Scott Collins]

with the current patch, everything runs great right up to the point something
funny happens in the parser that looks like |theParser| in |nsParser::Tokenize|
is bad, though I haven't yet found the connection to strings.  The bad tokenizer
then passes what looks like uninitialized iterators into the string machinery,
and everything starts to break.  I haven't yet found the connection that makes
this easy to debug, so I've been stepping through the places where tokenizers
are created and requested, near this code.

Comment 20

[Scott Collins]

Attachment: (replaces all patches above) ...and fixes |End(Read|Writ)ing|

Comment 21

[Scott Collins]

Attachment: source file containing tests

Comment 22

[Paul Chen]

r=pchen for jag's plan for this work

Comment 23

[jag (Peter Annema)]

Attachment: Update of above patch

Comment 24

[jag (Peter Annema)]

Attachment: Attempt at extracting nsASingleFragmentString change from above patch

Comment 25

[Scott Collins]

Comment on attachment 54473 [details] [diff] [review]
Attempt at extracting nsASingleFragmentString change from above patch

looks good, sr=scc

Comment 26

[jag (Peter Annema)]

Attachment: Initial patch updated to trunk

Comment 27

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 28

[timeless]

per comment 5 marking 1.0. beyond that what's the status of this bug? dbaron 
mentioned it in bug 121841 which is a live instance of runaway recursion ala 
bug 74709 although it's unclear whether it's related.

Comment 29

[scottputterman]

adding adt1.0.0+ for api changes only.  please get drivers approval and then
check into the 1.0 branch.

Comment 30

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

Comment on attachment 59091 [details] [diff] [review]
Initial patch updated to trunk

So this patch removes GetReadableFragment and GetWritableFragment from
nsAString, but it doesn't remove nsSharableString::GetWritableFragment.     
Does it add code somewhere else to fix bug 114438?  If not, this suggests that
we might need to fix bug 114140, rather than just leave things as-is (which I
was thinking would be acceptable, although not ideal).

Comment 31

[jag (Peter Annema)]

Sorry, let me clarify this. I'm going to attach a patch RSN (tm) which only adds
the new APIs with empty implementations, and doesn't remove the old ones just
yet (per drivers request).

Comment 32

[Scott Collins]

OK, here's our answer on how to get binary compatible stubs in to do this stuff
by Thursday (thanks to shaver for the saving the day with the #ifdef advice).

  Stub in the needed API calls, all empty
  #ifndef MOZILLA_STRICT_STRING_API
    structures and methods that will break binary compatibily
    for a plugin built now, against our library with the working fixes
    i.e., 1.0+string_fixes

People who are willing to recompile for 1.0+ need not define.  People who want
their binary to work with 1.0 and 1.0+ define it and live with restricted string
functionality _within_ their code.  That is, they can't use |+| to concatenate
strings, for instance, and a few other things.  Exact list will be added as a
comment to the bug.

This is the safest and quickest path.  This is the path we are following. 
Patches to follow.

Comment 33

[Scott Collins]

noting that I'm driving this, and that really, this is jag's bug and has been
for a while

Comment 34

[Scott Collins]

This is the decision: to maintain binary compatibility, and to get to 1.0, we
have decided not to fix this bug.

Information on how to code around to be presented, in a later comment.

Comment 35

[jag (Peter Annema)]

So it's all pretty simple, really:

You can still use concatenation of strings within your code, as long as you
don't do anything that casts away the concatenated string's type. The most
common form of this is passing a concatenation into a function as a |const
nsA{C}String&| parameter. If you do need to hand a concatenation to a function,
store it in an intermediate string for now, e.g. |nsString d(a+b+c);| and pass
in |d|.

Comment 36

[Asa Dotzler [:asa]]

off the list then.

Comment 37

[Jaime Rodriguez, Jr.]

Removing adt1.0.0+, as this has been marked as WONTFIX

Comment 38

[Mark Bitterling]

If this bug is WONTFIX, then what is the status of bug 74709 which was marked as
a duplicate of this one?

I can understand why the WONTFIX decision was made for Mozilla 1.0, but does
that reasoning carry through for ALL future Mozilla releases.  It seems like
there are still issues here that need to be addressed at some point.

If this is WONTFIX, how does that affect bug 76334 and bug 125389 which depend
on this bug?