74709 - infinite loop: promise passed as base type confuses iterator

Status: RESOLVED DUPLICATE of bug 70083

(Core :: XPCOM, defect, P1)

Description

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

I think I might understand what's going on with the string infinite
loop I noticed on Menday.  (It may or may not be a new bug, since the code
may never have been tested until I tested it yesterday after fixing the
bustage caused by the removal of nsLiteralChar.)  This is my
understanding based on reading the code.  gdb isn't actually usable
enough for me to check this in the debugger.

But anyway, the problem can be seen if you uncomment (i.e., comment out
the |#ifdef DEBUG_dbaron|) the |#define CSS_REPORT_PARSE_ERRORS| at the
top of content/html/style/src/nsCSSScanner.h and then load the attached
page.  This should trigger the parser error in nsCSSParser.cpp, line 2354.
This is a call that is basically:

ReportUnexpectedToken(sc, tok,
                      NS_LITERAL_STRING("Expected ") +
                      nsLocalString(stopString, 1) +
                      NS_LITERAL_STRING(" but found"))

where the function is defined as:

static void ReportUnexpectedToken(nsCSSScanner *sc,
                                  nsCSSToken& tok,
                                  const nsAReadableString& err)
{
  nsAutoString error(err + NS_LITERAL_STRING(" '"));
  /* do other stuff */
}

The first line of this function is where I see the infinite loop.  I
think the problem is caused by calling |operator+(nsAString&,
nsAString&)| even though the first of the two arguments is really an
|nsPromiseConcatenation|.  This means the outermost
|nsPromiseConcatenation| passed to the constructor of |nsAutoString|
has its mFragmentIdentifierMask set to 1 rather than to (1<<2).  Then
|copy_string| is called from |nsAString::do_AppendFromReadable| which
is called from the constructor of |nsAutoString| (through about 3 other
functions).  However, once the nsReadableFragment<PRUnichar> indicating
the beginning of the nsReadingIterator<PRUnichar> is advanced to the
second fragment (which is the right fragment of the innermost
|nsPromiseConcatenation|), the outermost |nsPromiseConcatenation|
thinks that fragment is referring to its right fragment.  So when we
try to advance to the third fragment (via a call to
|nsReadingIterator<PRUnichar>::advance| that calls
|nsReadingIterator<PRUnichar>::normalize_forward| which calls
|nsPromiseConcatenation::GetReadableFragment|), the outermost promise
thinks there is no next fragment and returns null and leaves the
iterator's fragment pointing to the second fragment.  This means that
the iterator's |mPosition| is equal to |mFragment.mEnd| so
|nsReadingIterator<PRUnichar>::size_forward| returns 0, but the
iterator's mPosition is still not the the same as the |last| parameter
to |copy_string|, so |copy_string| goes into an infinite loop with the
assertion on line 77 of nsAlgorithm.h.

I can see ways to fix this (at the cost of an extra virtual function
call) in |operator+(nsAString&, nsAString&)| for the case when the
left operand to that function is an |nsPromiseConcatenation|.  I
can't see a way to fix it when the right operand is an
|nsPromiseConcatenation|.

Comment 1

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

Attachment: testcase triggering this infinite loop in CSS error reporting

Comment 2

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

I changed my mind again.  I think we could fix this by adding a |virtual
PRUInt32 GetFragmentIdentifierMask();| to |nsA[C]String|.  It would return 0 for
|nsA[C]String|, but |nsPromise[C]Concatenation| would override it and turn the
result of GetFragmentIdentifierMask, and |nsPromise[C]Substring| would override
it and forward the call to the string of which it's a substring.  In both forms
of |nsPromise[C]Concatenation::nsPromise[C]Concatenation| we would then have to
ensure that |mFragmentIdentifierMask| is set to |(tmp =
NS_MAX(mStrings[kLeftString]->GetFragmentIdentifierMask(),
mStrings[kRightString]->GetFragmentIdentifierMask()))?tmp<<1:1|.  The
|nsPromise[C]Concatenation| constructor and |operator+| that take an
|nsPromise[C]Concatenation| as their first argument would then not really be
needed, although they would save a virtual function call and a comparison to 0.

Comment 3

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

A danger of this is that one could exceed the limit of 32
|nsPromiseConcatenation|s by concatenating in different places (perhaps
recursively).

Comment 4

[Scott Collins]

This can actually be fixed as part of the construction of an aggregate string,
i.e., in this case, the concatenation.  Concatenation dispatching to particular
fragments is costly in the case of multifragment strings already.  We need to
change this to calculating the number of fragments on each leaf at construction
time, and stepping down exactly the right branch.  This fix will automatically
fix this problem as well.  The mirror of this bug is that one can't concatenate
a multi-fragment string currently, and for the same reason.  Moving this bug
under the demesne of the other fragment work.

Comment 5

[Scott Collins]

It sounds strange to be marking this bug as a duplicate of another, but here's
the story: correct iteration requires fixing iterators, fragments, the API for
|GetXXXFragment| and the routine currently named |Promises|.  Fixing iteration
will make this bug go away.  70083 is all about fixing iteration for just this
reason.

*** This bug has been marked as a duplicate of 70083 ***