Last Comment Bug 827061 - ASSERTION: Bad position passed to nsJISx4051LineBreaker::Next: 'aLen > aPos', file ./mozilla/intl/lwbrk/src/nsJISx4501LineBreaker.cpp
: ASSERTION: Bad position passed to nsJISx4051LineBreaker::Next: 'aLen > aPos',...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 21.0
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
: 812622 (view as bug list)
Depends on:
Blocks: 826745
  Show dependency treegraph
 
Reported: 2013-01-05 17:17 PST by ISHIKAWA, Chiaki
Modified: 2013-08-29 12:08 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch to include aLen, aPos value in the log. (1.01 KB, text/plain)
2013-01-05 17:17 PST, ISHIKAWA, Chiaki
no flags Details
Patch to handle aPos == aLen == 0 case specially. (1.23 KB, patch)
2013-01-06 21:01 PST, ISHIKAWA, Chiaki
no flags Details | Diff | Splinter Review
ASSERTION and the stacktrace (built-in) leading to it. (33.46 KB, text/plain)
2013-01-07 19:52 PST, ISHIKAWA, Chiaki
no flags Details
don't attempt to find potential line-breaks within an empty string (1.18 KB, patch)
2013-01-08 02:59 PST, Jonathan Kew (:jfkthame)
standard8: review+
Details | Diff | Splinter Review

Description ISHIKAWA, Chiaki 2013-01-05 17:17:19 PST
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.)
Comment 1 ISHIKAWA, Chiaki 2013-01-06 21:01:09 PST
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
Comment 2 Jonathan Kew (:jfkthame) 2013-01-07 02:23:11 PST
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?)
Comment 3 Jonathan Kew (:jfkthame) 2013-01-07 02:30:37 PST
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.
Comment 4 ISHIKAWA, Chiaki 2013-01-07 17:51:18 PST
(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
Comment 5 ISHIKAWA, Chiaki 2013-01-07 19:52:39 PST
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
Comment 6 Jonathan Kew (:jfkthame) 2013-01-08 02:59:47 PST
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.
Comment 7 ISHIKAWA, Chiaki 2013-01-08 17:59:42 PST
(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?
Comment 8 Jonathan Kew (:jfkthame) 2013-01-09 00:14:07 PST
(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).
Comment 9 Jonathan Kew (:jfkthame) 2013-02-04 07:51:15 PST
mbanner: r? ping (or is there someone else who should review?)
Comment 10 Mark Banner (:standard8, limited time in Dec) 2013-02-07 01:29:24 PST
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.
Comment 11 Jonathan Kew (:jfkthame) 2013-02-07 06:42:55 PST
https://hg.mozilla.org/comm-central/rev/e63ea30fd18b
Comment 12 Magnus Melin 2013-08-29 04:38:46 PDT
*** Bug 812622 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.