472593 - "ASSERTION: Illegal value" in nsJISx4501LineBreaker.cpp when removing tab characters

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading http://www.trafficjot.com/TJMain.aspx triggers:

###!!! ASSERTION: Illegal value (length > position): 'aLen > aPos', file /Users/jruderman/central/intl/lwbrk/src/nsJISx4501LineBreaker.cpp, line 789

I have attached a simple testcase based on the site.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: check whether already reached end of text before calling Next()

The attached patch, to check that we're not already at the end before calling mLineBreaker->Next(), appears to resolve this. I will run reftests to check there are no unexpected side-effects before requesting review.

Comment 2

[Jonathan Kew [:jfkthame]]

Moved this bug from Layout: Text to Content, as the assertion fires (correctly) when nsPlainTextSerializer (in content/base) calls mLineBreaker->Next() with a current position that is already at the end of the text. It shouldn't be calling the line breaker for a Next() break position when there's no more text in the current line being processed.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: same patch, but also tidy up assertion messages in the linebreaker

The assertion message in Next() was poorly worded; same applies to the equivalent assertion in Prev(). So I have revised these in the updated patch.

The actual code change in content/base/src/nsPlainTextSerializer.cpp is unchanged from the earlier version of the patch.

Comment 4

[Jesse Ruderman]

> +  NS_ASSERTION(aLen > aPos, "Illegal value (position should be < length)");

If you're concerned that "aLen > aPos" is confusing, you can change the condition to "aPos < aLen" instead of repeating the condition in the assertion text.  I prefer assertion text that provides context ("Bad position passed to nsJISx4051LineBreaker::Next") or is easy to recognize ("Position is past the end of the text!").

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 356098 [details] [diff] [review]
same patch, but also tidy up assertion messages in the linebreaker

r=bzbarsky with Jesse's comments addressed.

For what it's worth, the mq diff would be a lot easier to review if you add the following to the [diff] section in your hgrc:

  showfunc = true

It would be even better if you also added

  unified = 8

but that also increases the risk of conflicts when applying the mq patches of course.  Generally speaking, I think it's worth it.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: updated assertion messages as suggested in review

Thanks for the comments; adjusted patch is attached.

Note that the assertion in nsJISx4051LineBreaker::Prev() is now slightly stricter than the original (it prohibits aPos == 0, which would be an equivalent error to the one with Next() that's actually being fixed in this patch). I've run the full layout reftest suite with this version and it doesn't break anything, so it seems worth tightening up the code while we're here.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Would be good to add a crashtest for this.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 3afab626d3aa