Text Encoding: Fix "stop on error" for ICU-based codecs

third_party/WebKit/Source/wtf/text/TextCodecICU.cpp

 /*
  * Copyright (C) 2004, 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2006 Alexey Proskuryakov <ap@nypop.com>
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 293 matching lines @@
     return target - targetStart;
 }
 
 class ErrorCallbackSetter final {
     STACK_ALLOCATED();
 public:
     ErrorCallbackSetter(UConverter* converter, bool stopOnError)
         : m_converter(converter)
         , m_shouldStopOnEncodingErrors(stopOnError)
     {
-        if (m_shouldStopOnEncodingErrors) {
-            UErrorCode err = U_ZERO_ERROR;
-            ucnv_setToUCallBack(m_converter, UCNV_TO_U_CALLBACK_SUBSTITUTE,
-                UCNV_SUB_STOP_ON_ILLEGAL, &m_savedAction,
-                &m_savedContext, &err);
-            ASSERT(err == U_ZERO_ERROR);
-        }
+        UErrorCode err = U_ZERO_ERROR;
+        if (m_shouldStopOnEncodingErrors)
+            ucnv_setToUCallBack(m_converter, UCNV_TO_U_CALLBACK_STOP, 0, &m_savedAction, &m_savedContext, &err);
+        else
+            ucnv_setToUCallBack(m_converter, UCNV_TO_U_CALLBACK_SUBSTITUTE, 0, &m_savedAction, &m_savedContext, &err);

[jsbell, 2016/08/09 22:27:24]

Maybe we only want the `if (m_shouldStopOnEncodingErrors)` branch, and do
nothing if !m_shouldStopOnEncodingErrors 

The ICU docs are terrible for the combination of UCNV_TO_U_CALLBACK_SUBSTITUTE /
UCNV_SUB_STOP_ON_ILLEGAL - it's unclear to me what that even means, even though
it's called out explicitly as a valid combination.

[jungshik at Google, 2016/08/12 07:20:41]

Indeed, confusing. 

https://cs.chromium.org/chromium/src/third_party/icu/source/common/unicode/uc...

helps. 

For 'ToUnicode' direction, 
UCNV_SUB_STOP_ON_ILLEGAL means  that the conversion should stop right away when
coming across a byte/byte sequence NOT allowed by the current charset (that is,
*illegal*  or outside the range).  However, 'valid but unassigned' byte
sequences will STILL be substituted per UCNV_TO_U_CALLBACK_SUBSTITUTE,. 

Depending on how tightly valid byte-ranges are defined, there may or may not be 
byte sequences falling into the second group (valid but unassigned)> .

Because the encoding spec does not make a distinction between the former and the
latter, I think we don't need 'else' branch. 

+        DCHECK_EQ(err, U_ZERO_ERROR);
     }
     ~ErrorCallbackSetter()
     {
-        if (m_shouldStopOnEncodingErrors) {
-            UErrorCode err = U_ZERO_ERROR;
-            const void* oldContext;
-            UConverterToUCallback oldAction;
-            ucnv_setToUCallBack(m_converter, m_savedAction, m_savedContext, &oldAction, &oldContext, &err);
-            ASSERT(oldAction == UCNV_TO_U_CALLBACK_SUBSTITUTE);
-            ASSERT(!strcmp(static_cast<const char*>(oldContext), UCNV_SUB_STOP_ON_ILLEGAL));
-            ASSERT(err == U_ZERO_ERROR);
-        }
+        UErrorCode err = U_ZERO_ERROR;
+        const void* oldContext;
+        UConverterToUCallback oldAction;
+        ucnv_setToUCallBack(m_converter, m_savedAction, m_savedContext, &oldAction, &oldContext, &err);
+        DCHECK_EQ(oldAction, m_shouldStopOnEncodingErrors ? UCNV_TO_U_CALLBACK_STOP : UCNV_TO_U_CALLBACK_SUBSTITUTE);
+        DCHECK(!oldContext);
+        DCHECK_EQ(err, U_ZERO_ERROR);
     }
 
 private:
     UConverter* m_converter;
     bool m_shouldStopOnEncodingErrors;
     const void* m_savedContext;
     UConverterToUCallback m_savedAction;
 };
 
 String TextCodecICU::decode(const char* bytes, size_t length, FlushBehavior flush, bool stopOnError, bool& sawError)
@@ skipping 267 matching lines @@
 {
     return encodeCommon(characters, length, handling);
 }
 
 CString TextCodecICU::encode(const LChar* characters, size_t length, UnencodableHandling handling)
 {
     return encodeCommon(characters, length, handling);
 }
 
 } // namespace WTF