Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at netwerk/base/src/nsUnicharStreamLoader.cpp:218 213 DebugOnly<nsresult> rv = 214 self->mDecoder->Convert(aSegment, 215 &srcLen, 216 self->mBuffer.BeginWriting() + haveRead, 217 &dstLen); * 218 MOZ_ASSERT(NS_SUCCEEDED(rv)); 219 MOZ_ASSERT(srcLen == static_cast<int32_t>(aCount)); 220 haveRead += dstLen; 221 222 self->mBuffer.SetLength(haveRead);

ISO-2022-JP decoder should honor the mErrBehavior...

is dstLen initialized to something or did line 220-222 just incorporate a random amount of junk into the string?

dstLen is initialized by GetMaxLength in line 206.

Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at ../../../../content/base/src/nsScriptLoader.cpp:1070

(In reply to Masatoshi Kimura [:emk] from comment #2) > ISO-2022-JP decoder should honor the mErrBehavior... Can you explain this further? The error code causing the assertion comes from nsISO2022JPToUnicodeV2::Convert: ================ switch(mState) { case mState_ASCII: if(0x1b == *src) { mLastLegalState = mState; mState = mState_ESC; } else if(*src & 0x80) { ERROR-> goto error2; } else { if (CHECK_OVERRUN(dest, destEnd, 1)) goto error1; *dest++ = (PRUnichar) *src; } break; ================ The state is mState_ASCII, and "if(*src & 0x80)" is true.

(In reply to Josh Aas (Mozilla Corporation) from comment #6) > (In reply to Masatoshi Kimura [:emk] from comment #2) > > ISO-2022-JP decoder should honor the mErrBehavior... > > Can you explain this further? 1. If mErrBehavior is not kOnError_Signal (default), the ISO-2022-JP decoder should replace the offending char with a REPLACEMENT CHARACTER (U+FFFD) rather than throwing NS_ERROR_UNEXPECTED. 2. Even if mErrBehavior is kOnError_Signal, the thrown error should be NS_ERROR_ILLEGAL_INPUT.

Thanks for the info. This avoids the assertion in the first test case, and gives what I'd expect for an error in the script case. I don't really know what this code is supposed to be doing though, so please review carefully!

Comment on attachment 722639 [details] [diff] [review] fix v1.0 You need to call CHECK_OVERRUN every time you write to dest.

Like so?

I will also write a test for this.

Comment on attachment 722911 [details] [diff] [review] fix v1.1 LGTM

This test works, but there may be a better test. The test here fails if the assertion is triggered.

Comment on attachment 723468 [details] [diff] [review] fix v1.1 w/test Canceling second review, turns out emk is a peer. Had smontagu update the module owernship wiki.

pushed to mozilla-inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/8d7a31fbb879

I found another testcase that still trips the assertion. Filed bug 851982.

Comment on attachment 723468 [details] [diff] [review] fix v1.1 w/test [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 638379 User impact if declined: Some Japanese production sites are broken (see bug 858433). Testing completed (on m-c, etc.): on m-c and aurora Risk to taking this patch (and alternatives if risky): very low String or IDL/UUID changes made by this patch: none

Approving on beta, as this is a recent regression from Bug 638379, which has impacted a few Japanese websites .The patch fixes the broken websites and is low risk,well baked on m-c and aurora. Please land asap for this to make it into our next beta going to build tomorrow morning PT .

Since this is sec-moderate, and we don't have indications that a large number of sites are affected, we'll ship this fix in FF21.