Auto merge of #5659 - aneeshusa:add-range-errors-use-for-textencoder-textdecoder, r=jdm

Fixes #5620, and adds a few extra test cases.

Currently waiting on a few upstream PRs in rust-encoding to land.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/5659)
<!-- Reviewable:end -->

components/script/dom/bindings/error.rs

@@ -12,7 +12,7 @@ use util::str::DOMString;
 
 use js::jsapi::{JSContext, JSBool, JSObject};
 use js::jsapi::{JS_IsExceptionPending, JS_SetPendingException, JS_ReportPendingException};
-use js::jsapi::{JS_ReportErrorNumber, JSErrorFormatString, JSEXN_TYPEERR};
+use js::jsapi::{JS_ReportErrorNumber, JSErrorFormatString, JSEXN_TYPEERR, JSEXN_RANGEERR};
 use js::jsapi::{JS_SaveFrameChain, JS_RestoreFrameChain};
 use js::glue::{ReportError};
 use js::rust::with_compartment;
@@ -59,6 +59,8 @@ pub enum Error {
 
     /// TypeError JavaScript Error
     Type(DOMString),
+    /// RangeError JavaScript Error
+    Range(DOMString),
 
     /// A JavaScript exception is already pending.
     JSFailed,
@@ -95,7 +97,12 @@ pub fn throw_dom_exception(cx: *mut JSContext, global: GlobalRef,
             assert!(unsafe { JS_IsExceptionPending(cx) } == 0);
             throw_type_error(cx, &message);
             return;
-        }
+        },
+        Error::Range(message) => {
+            assert!(unsafe { JS_IsExceptionPending(cx) } == 0);
+            throw_range_error(cx, &message);
+            return;
+        },
         Error::JSFailed => {
             assert!(unsafe { JS_IsExceptionPending(cx) } == 1);
             return;
@@ -135,7 +142,7 @@ pub fn throw_not_in_union(cx: *mut JSContext, names: &'static str) -> JSBool {
     return 0;
 }
 
-/// Format string used to throw `TypeError`s.
+/// Format string used to throw javascript errors.
 static ERROR_FORMAT_STRING_STRING: [libc::c_char; 4] = [
     '{' as libc::c_char,
     '0' as libc::c_char,
@@ -144,29 +151,53 @@ static ERROR_FORMAT_STRING_STRING: [libc::c_char; 4] = [
 ];
 
 /// Format string struct used to throw `TypeError`s.
-static mut ERROR_FORMAT_STRING: JSErrorFormatString = JSErrorFormatString {
+static mut TYPE_ERROR_FORMAT_STRING: JSErrorFormatString = JSErrorFormatString {
     format: &ERROR_FORMAT_STRING_STRING as *const libc::c_char,
     argCount: 1,
     exnType: JSEXN_TYPEERR as i16,
 };
 
-/// Callback used to throw `TypeError`s.
+/// Format string struct used to throw `RangeError`s.
+static mut RANGE_ERROR_FORMAT_STRING: JSErrorFormatString = JSErrorFormatString {
+    format: &ERROR_FORMAT_STRING_STRING as *const libc::c_char,
+    argCount: 1,
+    exnType: JSEXN_RANGEERR as i16,
+};
+
+/// Callback used to throw javascript errors.
+/// See throw_js_error for info about error_number.
 unsafe extern fn get_error_message(_user_ref: *mut libc::c_void,
                             _locale: *const libc::c_char,
                             error_number: libc::c_uint) -> *const JSErrorFormatString
 {
-    assert_eq!(error_number, 0);
+    match error_number as i32 {
-    &ERROR_FORMAT_STRING as *const JSErrorFormatString
+      JSEXN_TYPEERR => &TYPE_ERROR_FORMAT_STRING as *const JSErrorFormatString,
+      JSEXN_RANGEERR => &RANGE_ERROR_FORMAT_STRING as *const JSErrorFormatString,
+      _ => panic!("Bad js error number given to get_error_message: {}", error_number)
+    }
 }
 
-/// Throw a `TypeError` with the given message.
+/// Helper fn to throw a javascript error with the given message and number.
-pub fn throw_type_error(cx: *mut JSContext, error: &str) {
+/// Reuse the jsapi error codes to distinguish the error_number
+/// passed back to the get_error_message callback.
+/// c_uint is u32, so this cast is safe, as is casting to/from i32 from there.
+fn throw_js_error(cx: *mut JSContext, error: &str, error_number: u32) {
     let error = CString::new(error).unwrap();
     unsafe {
         JS_ReportErrorNumber(cx,
             Some(get_error_message as
                 unsafe extern "C" fn(*mut libc::c_void, *const libc::c_char,
                                      libc::c_uint) -> *const JSErrorFormatString),
-            ptr::null_mut(), 0, error.as_ptr());
+            ptr::null_mut(), error_number, error.as_ptr());
     }
 }
+
+/// Throw a `TypeError` with the given message.
+pub fn throw_type_error(cx: *mut JSContext, error: &str) {
+    throw_js_error(cx, error, JSEXN_TYPEERR as u32);
+}
+
+/// Throw a `RangeError` with the given message.
+pub fn throw_range_error(cx: *mut JSContext, error: &str) {
+    throw_js_error(cx, error, JSEXN_RANGEERR as u32);
+}

components/script/dom/textdecoder.rs

@@ -39,6 +39,10 @@ impl TextDecoder {
         }
     }
 
+    fn make_range_error() -> Fallible<Temporary<TextDecoder>> {
+        Err(Error::Range("The given encoding is not supported.".to_owned()))
+    }
+
     pub fn new(global: GlobalRef, encoding: EncodingRef, fatal: bool) -> Temporary<TextDecoder> {
         reflect_dom_object(box TextDecoder::new_inherited(encoding, fatal),
                            global,
@@ -51,14 +55,23 @@ impl TextDecoder {
                        options: &TextDecoderBinding::TextDecoderOptions)
                             -> Fallible<Temporary<TextDecoder>> {
         let encoding = match encoding_from_whatwg_label(&label) {
-            Some(enc) => enc,
+            None => return TextDecoder::make_range_error(),
-            // FIXME: Should throw a RangeError as per spec
+            Some(enc) => enc
-            None => return Err(Error::Syntax),
+        };
+        // The rust-encoding crate has WHATWG compatibility, so we are
+        // guaranteed to have a whatwg_name because we successfully got
+        // the encoding from encoding_from_whatwg_label.
+        // Use match + panic! instead of unwrap for better error message
+        match encoding.whatwg_name() {
+            None => panic!("Label {} fits valid encoding without valid name", label),
+            Some("replacement") => return TextDecoder::make_range_error(),
+            _ => ()
         };
         Ok(TextDecoder::new(global, encoding, options.fatal))
     }
 }
 
+
 impl<'a> TextDecoderMethods for JSRef<'a, TextDecoder> {
     fn Encoding(self) -> DOMString {
         self.encoding.whatwg_name().unwrap().to_owned()

components/script/dom/textencoder.rs

@@ -6,7 +6,7 @@ use dom::bindings::codegen::Bindings::TextEncoderBinding;
 use dom::bindings::codegen::Bindings::TextEncoderBinding::TextEncoderMethods;
 use dom::bindings::global::GlobalRef;
 use dom::bindings::error::Fallible;
-use dom::bindings::error::Error::IndexSize;
+use dom::bindings::error::Error::Range;
 use dom::bindings::js::{JSRef, Temporary};
 use dom::bindings::str::USVString;
 use dom::bindings::utils::{Reflector, reflect_dom_object};
@@ -54,8 +54,7 @@ impl TextEncoder {
             Some(enc) => enc,
             None => {
                 debug!("Encoding Label Not Supported");
-                // TODO: should throw RangeError
+                return Err(Range("The given encoding is not supported.".to_owned()))
-                return Err(IndexSize)
             }
         };
 
@@ -65,8 +64,7 @@ impl TextEncoder {
             }
             _ => {
                 debug!("Encoding Not UTF");
-                // TODO: should throw RangeError
+                return Err(Range("The encoding must be utf-8, utf-16le, or utf-16be.".to_owned()))
-                Err(IndexSize)
             }
         }
     }

tests/wpt/metadata/MANIFEST.json

@@ -10951,6 +10951,10 @@
         "path": "encoding/api-basics.html",
         "url": "/encoding/api-basics.html"
       },
+      {
+        "path": "encoding/api-invalid-label.html",
+        "url": "/encoding/api-invalid-label.html"
+      },
       {
         "path": "encoding/api-replacement-encodings.html",
         "url": "/encoding/api-replacement-encodings.html"

tests/wpt/metadata/encoding/api-replacement-encodings.html.ini

@@ -1,20 +1,4 @@
 [api-replacement-encodings.html]
   type: testharness
-  [The "replacement" label should not be a known encoding.]
-    expected: FAIL
-
-  [Label for "replacement" should be rejected by API: csiso2022kr]
-    expected: FAIL
-
   [Label for "replacement" should be rejected by API: hz-gb-2312]
     expected: FAIL
-
-  [Label for "replacement" should be rejected by API: iso-2022-cn]
-    expected: FAIL
-
-  [Label for "replacement" should be rejected by API: iso-2022-cn-ext]
-    expected: FAIL
-
-  [Label for "replacement" should be rejected by API: iso-2022-kr]
-    expected: FAIL
-

tests/wpt/metadata/encoding/textencoder-constructor-non-utf.html.ini

@@ -1,113 +1,7 @@
 [textencoder-constructor-non-utf.html]
   type: testharness
-  [Non-UTF encodings supported only for decode, not encode: ibm866]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-2]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-3]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-4]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-5]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-6]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-7]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-8]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-8-i]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-10]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-13]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-14]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-15]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-8859-16]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: koi8-r]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: koi8-u]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: macintosh]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-874]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1250]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1251]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1252]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1253]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1254]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1255]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1256]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1257]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: windows-1258]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: x-mac-cyrillic]
-    expected: FAIL
-
   [Non-UTF encodings supported only for decode, not encode: gbk]
     expected: FAIL
 
-  [Non-UTF encodings supported only for decode, not encode: gb18030]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: big5]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: euc-jp]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: iso-2022-jp]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: shift_jis]
-    expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: euc-kr]
-    expected: FAIL
-
   [UTF encodings are supported for encode and decode: utf-16le]
     expected: FAIL
-
-  [Non-UTF encodings supported only for decode, not encode: x-user-defined]
-    expected: FAIL
-

tests/wpt/web-platform-tests/encoding/api-invalid-label.html

@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<title>Encoding API: invalid label</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+
+var invalidLabel = "invalid-invalidLabel"
+
+test(function() {
+    assert_throws({name: 'RangeError'}, function() { new TextEncoder(invalidLabel); });
+    assert_throws({name: 'RangeError'}, function() { new TextDecoder(invalidLabel); });
+}, 'Invalid label "' + invalidLabel + '" should be rejected by API.');
+
+</script>