ASSERTION: Bad position passed to nsJISx4051LineBreaker::Next: 'aLen > aPos', file ./mozilla/intl/lwbrk/src/nsJISx4501LineBreaker.cpp

RESOLVED FIXED in Thunderbird 21.0

Status

Thunderbird
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ISHIKAWA, Chiaki, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 21.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 698352 [details]
Patch to include aLen, aPos value in the log.

While testing thunderbird (debug build of comm-central source) by
running "make mozmill" locally, I noticed many ASSERTION messages.
One of them is the following.


 ###!!! ASSERTION: Bad position passed to nsJISx4051LineBreaker::Next: 'aLen > aPos', file /TB-NEW/TB-3HG/new-src/mozilla/intl/lwbrk/src/nsJISx4501LineBreaker.cpp, line 791

I was not sure what the problematic value was:
it turns out both aLen and aPos were zero in mozmill tests.
(I am halfway through running the modified source with the attached  patch.
So there may be other cases where the values are different, but
I think this aLen=0, aPos=0 case needs attention immediately.
Maybe aLen==0, aPos==0 case needs to be handled as a special case (and return
immediately)?
The patch produces the following output:

###!!! ASSERTION: Bad position passed to nsJISx4051LineBreaker::Next aLen (0) <= aPos(0): 'aLen > aPos', file /TB-NEW/TB-3HG/new-src/mozilla/intl/lwbrk/src/nsJISx4501LineBreaker.cpp, line 798


(Could be related to Bug 439012? ""ASSERTION: Illegal value (length > position): 'aLen > aPos'" when replying to a message with Chinese characters".
But I am using Japanese locale under Debian GNU/linux. So the problem may be universal for multi-octect character code systems.)
(Reporter)

Updated

5 years ago
Blocks: 826745
Summary: ASSERTION: Bad position passed to nsJISx4051LineBreaker::Next: 'aLen > aPos', file /TB-NEW/TB-3HG/new-src/mozilla/intl/lwbrk/src/nsJISx4501LineBreaker.cpp → ASSERTION: Bad position passed to nsJISx4051LineBreaker::Next: 'aLen > aPos', file ./mozilla/intl/lwbrk/src/nsJISx4501LineBreaker.cpp
(Reporter)

Comment 1

4 years ago
Created attachment 698524 [details] [diff] [review]
Patch to handle aPos == aLen == 0 case specially.

I think the case of (aPos, aLen) = (0,0) requires special handling although
I have no idea why this function is called this manner in TB.
If FF calls this function in this manner, I believe someone spotted it 
already.

 (Not sure if the
TB's comm-central source of mozilla subdirectory is quite the same as
the one for Firefox.)

Attached patch returns an error value immediately when
aPos == aLen == 0. (The call to wordmove() will reference an array with "-1" index in one place and it is not nice.)

Anyway, looking at the "hg blame" listing and 
Bug 472593 - "ASSERTION: Illegal value" in nsJISx4501LineBreaker.cpp when removing tab characters
I am adding jfkthame@gmail.com as a possible reviewer.

TIA
Attachment #698524 - Flags: review?(jfkthame)
(Assignee)

Comment 2

4 years ago
Before we decide to add a special-case to nsJISx4051LineBreaker::Next, I think we should look at where those values are coming from. Maybe it's better to fix the caller, and preserve the existing assumption that it's incorrect to call Next() if aPos is already at th end of the available text.

Could you post a backtrace showing where this call with (aPos, aLen) = (0, 0) is happening? (From a quick look at the comm-central tree, I'm guessing it may be nsMsgComposeService::GetOrigWindowSelection, as there are probably cases where nsISelection::ToString at http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#441 returns an empty string. Is that what's triggering this assertion?)
(Assignee)

Comment 3

4 years ago
Comment on attachment 698524 [details] [diff] [review]
Patch to handle aPos == aLen == 0 case specially.

Clearing r? for now, as I think my preference will be to fix the caller once we've confirmed where this is coming from; may reconsider that if it looks like a more widespread or complex issue.
Attachment #698524 - Flags: review?(jfkthame)
(Reporter)

Comment 4

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Before we decide to add a special-case to nsJISx4051LineBreaker::Next, I
> think we should look at where those values are coming from. Maybe it's
> better to fix the caller, and preserve the existing assumption that it's
> incorrect to call Next() if aPos is already at th end of the available text.

Fair enough.
 
> Could you post a backtrace showing where this call with (aPos, aLen) = (0,
> 0) is happening? (From a quick look at the comm-central tree, I'm guessing
> it may be nsMsgComposeService::GetOrigWindowSelection, as there are probably
> cases where nsISelection::ToString at
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/
> nsMsgComposeService.cpp#441 returns an empty string. Is that what's
> triggering this assertion?)

Attached is the session log of where I observe the problem.
The stack trace leading to the assertion is all the same (for a couple of dozen 
appearances).

The strange and perplexing thing is the built-in backtrace does not show the symbolic names of the functions.
(The stack trace shows the address only in hexadecimal numeric value
except for a few entries. : I wonder if this is due to JIT-compiled binary (?) or
some kind of magic, or buggy backtrace code...)
Also, I am not entirely sure how to enable debugger during the automated
"make mozmill" run. So I am not entirely sure how to use gdb to kick in
to obtain meaningful backtrace (if gdb can trace this meaningfully.)

TIA
(Reporter)

Comment 5

4 years ago
Created attachment 699001 [details]
ASSERTION and the stacktrace (built-in) leading to it.

Sorry forgot the attachment.
An instance of the assertion triggered during the make mozmill run of
debug build of TB.

I hate to see so many warnings in a session log, but that is another matter.
(Hard to discriminate a real bug from chaff, so to speak.)

TIA
(Assignee)

Comment 6

4 years ago
Created attachment 699081 [details] [diff] [review]
don't attempt to find potential line-breaks within an empty string

It's difficult to be sure until that stack gets cleaned-up to provide real symbols, but judging by the testcase being run, I think my guess in comment 2 is probably right. Here's a speculative patch (untested) that I believe would fix the issue, if you care to try it.
(Assignee)

Updated

4 years ago
Attachment #699081 - Flags: feedback?(ishikawa)
(Reporter)

Comment 7

4 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> Created attachment 699081 [details] [diff] [review]
> don't attempt to find potential line-breaks within an empty string
> 
> It's difficult to be sure until that stack gets cleaned-up to provide real
> symbols, but judging by the testcase being run, I think my guess in comment
> 2 is probably right. Here's a speculative patch (untested) that I believe
> would fix the issue, if you care to try it.

Thank you for the patch. It indeed seems to cure the problem.
I no longer see the particular assertion anymore. In particular the particular test that issued warning passed without the assertion message.

I wonder, though, if returning NS_ERROR_ABORT is a little too harsh(?)

I am not sure if this is related to the patch or not, but
suddenly I am seeing TEST-UNEXPECTED-FAILURE related to
plugins and wonder if this seemingly unrelated issue is 
caused by the patch. NO!. Now I realize that the
new TEST-UNEXPECTED-FAILUREs seem to be introduced by the addition of new tests(!). (the tests were not in the session log
about a week old.( I don't think new new failures have anything to do with them.


So yes, the patch worked. Great.
What do we do for review and approval?
(Assignee)

Updated

4 years ago
Attachment #699081 - Flags: feedback?(ishikawa) → review?(mbanner)
(Assignee)

Comment 8

4 years ago
(In reply to ISHIKAWA, chiaki from comment #7)
> I wonder, though, if returning NS_ERROR_ABORT is a little too harsh(?)

It's fine; that's the same return code used throughout the function to indicate that it failed - for whatever reason - to find an appropriate selection for quoting. It doesn't mean anything "bad" will happen.

The existing code ends up returning the same result, it just fires an assertion en route because it calls the line-breaker with an empty string, which is unexpected (though not actually dangerous, afaics).

> What do we do for review and approval?

I've flagged Mark for review (please redirect if necessary).
Component: Untriaged → General
(Assignee)

Comment 9

4 years ago
mbanner: r? ping (or is there someone else who should review?)
Comment on attachment 699081 [details] [diff] [review]
don't attempt to find potential line-breaks within an empty string

Sorry for the delay. This looks fine, as it retains the same functionality.
Attachment #699081 - Flags: review?(mbanner) → review+
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/comm-central/rev/e63ea30fd18b
Assignee: nobody → jfkthame
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0

Updated

4 years ago
Duplicate of this bug: 812622
You need to log in before you can comment on or make changes to this bug.