Support surrogates in the DOM?

[SimonSapin]

JavaScript strings are potentially ill-formed UTF-16 (arbitrary Vec<u16>) and can contain unpaired surrogates. Rust’s String type is well-formed UTF-8 and can not contain any surrogate. Surrogates are never emitted when decoding bytes from the network, but they can sneak in through document.write, the Element.innerHtml setter, or other DOM APIs.

https://simonsapin.github.io/wtf-8/#motivation has some background and definitions.

This has been discussed before, but let’s try to centralize discussion here.

• https://www.mail-archive.com/dev-servo@lists.mozilla.org/msg00634.html
• https://www.w3.org/Bugs/Public/show_bug.cgi?id=11298
• surrogate Unicode code points in input stream html5lib/html5lib-tests#19
• https://github.com/servo/servo/wiki/Workweek-encoding
• https://www.mail-archive.com/dev-servo@lists.mozilla.org/msg00921.html

The status quo as of this writing is that DOMString in Servo is String, and jsstring_to_str panics and crashes the browser when encountering surrogates. This happens in Acid 3: #6519.

To align with the current spec and other implementations, we can make DOMString potentially ill-formed UTF-16 or WTF-8 (https://github.com/SimonSapin/rust-wtf8). This is a bunch of work in (at least) string-cache, html5ever, and Servo. (html5ever already uses Tendril which supports multiple encodings including UTF-8 and WTF-8, but html5ever is still hard-coded to StrTendril = Tendril<UTF8>.)

Another option is to willfully violate the spec and replace unpaired surrogates with U+FFFD when converting to UTF-8: #6541. (String::from_utf16_lossy in the Rust standard library does this.) Acid 3 tests for this and considers it an acceptable behavior in its # 68 test. @Ms2ger commented in #6541 that "This is definitely the wrong fix." but I’m not so sure. I think we should do this until we have evidence of real content that relies on surrogates being preserved.

For what it’s worth, WebKit/Blink preserve unpaired surrogates in the DOM, but fail to render text in a given TextNode after the first unpaired surrogate.

data:text/html,<script>document.write("Before \uD800 after<span> next element")</script>

[Ms2ger]

As long as Acid3 is the only thing that hits this, I think we should keep the panic.

[SimonSapin]

How is panic ever a useful behavior?

[Ms2ger]

It alerts us to the fact that someone relies on it, so that we have something to base our decision on.

Speaking of data, we should try to add instrumentation to Gecko.

[SimonSapin]

Crashing the browser seems very user-hostile, especially when there is no indication that a site would actually be unusable if unpaired surrogates are replaced. (Maybe it would just show � instead of something that looks like 􀀀.)

There’s also no guarantee that it’ll alert us. Should we add some kind of telemetry to Servo?

[metajack]

I agree with @SimonSapin here. Crashing is not really acceptable for non-developer users.

Telemetry is probably the correct way to solve this, but I'm guessing that will be a little while in coming. How would we go about adding telemetry to gecko to estimate impact of various things we could do?

[jdm]

https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

[pcwalton]

I think we should not keep the panic; it's causing actual crashes that are preventing things like fuzzers from working. We should never deliberately keep a panic in; it's a terribly blunt debugging instrument.

Here's a concrete compromise proposal. Replace surrogates with U+FFFD as in @SimonSapin's patch, but output a message with warn!() with the current URL when doing so. We don't use warn!() right now. This will give us as the same debuggability as the current setup, without crashing. Later, we can easily convert warn!() instances to telemetry probes when we get the infrastructure to do so.

[SimonSapin]

The plan for now per https://github.com/servo/servo/wiki/Meeting-2015-07-13 is to add a -Z option to replace surrogates (e.g. when fuzzing) and keep panicking by default.

[farodin91]

I get this error by testing acid3.

thread 'ScriptTask PipelineId(0)' panicked at 'Found an unpaired surrogate in a DOM string. If you see this in real web content, please comment on #6564 Use -Z replace-surrogates on the command line to make this non-fatal.', /home/farodin/Git/servo/components/script/dom/bindings/conversions.rs:472

[SimonSapin]

@farodin91, that’s #6519.

As the error message says, you can add that command line option to make Servo continue despite the error, and test 68 of Acid 3 will succeed. Many other Acid3 tests are currently failing, though, and we’re not actively trying to fix as Acid 3 is not very representative of real content.

[eternaleye]

As suggested by Servo when it crashed on real web content, reporting here:

Accessing https://vector.im (and bypassing the "Your browser is not supported" message, due to Servo failing its tests for flexbox and objectfit), Servo crashes with the following text:

thread 'ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }' panicked at 'Found an unpaired surrogate in a DOM string. If you see this in real web content, please comment on https://github.com/servo/servo/issues/6564 Use `-Z replace-surrogates` on the command line to make this non-fatal.', /home/user/Code/rust/servo/components/util/non_geckolib.rs:83
stack backtrace:
   1:     0x55d5cef7c420 - sys::backtrace::tracing::imp::write::hb39adb9cc1dae4a9Nyu
   2:     0x55d5cef807db - panicking::default_handler::_$u7b$$u7b$closure$u7d$$u7d$::closure.43683
   3:     0x55d5cef80448 - panicking::default_handler::hced6c0d85eb2cb83bez
   4:     0x55d5cef69fec - sys_common::unwind::begin_unwind_inner::hafb014b2dc8828386mt
   5:     0x55d5ce58c21f - sys_common::unwind::begin_unwind::h2067109826205288885
   6:     0x55d5ce58e762 - non_geckolib::jsstring_to_str::hddb36a66dfe17547gOa
   7:     0x55d5ce58e32c - non_geckolib::DOMString.FromJSValConvertible::from_jsval::ha08281e8b0873200wMa
   8:     0x55d5cdb3e13d - dom::bindings::codegen::Bindings::ElementBinding::set_innerHTML::h3b594b81c5968852zOT
   9:     0x55d5ce5ae7af - CallJitSetterOp
  10:     0x55d5cda8aea8 - dom::bindings::utils::call_setter::h04d2614905d9c3c7Ong
  11:     0x55d5cda8ad96 - dom::bindings::utils::generic_call::h94edb8b4f4b73fd38ig
  12:     0x55d5ce6b62cf - _ZN2js6InvokeEP9JSContextN2JS8CallArgsENS_14MaybeConstructE
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/jscntxtinlines.h:235
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/vm/Interpreter.cpp:502
  13:     0x55d5ce6b76d4 - _ZN2js6InvokeEP9JSContextRKN2JS5ValueES5_jPS4_NS2_13MutableHandleIS3_EE
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/vm/Interpreter.cpp:558
  14:     0x55d5ce6bb649 - _ZN2js20InvokeGetterOrSetterEP9JSContextP8JSObjectN2JS5ValueEjPS5_NS4_13MutableHandleIS5_EE
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/vm/Interpreter.cpp:628
  15:     0x55d5ce70a0b7 - _ZN2js5Shape3setEP9JSContextN2JS6HandleIPNS_12NativeObjectEEENS4_IP8JSObjectEENS3_13MutableHandleINS3_5ValueEEERNS3_14ObjectOpResultE
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/vm/Shape-inl.h:51
  16:     0x55d5ce701a58 - _ZN2js17NativeSetPropertyEP9JSContextN2JS6HandleIPNS_12NativeObjectEEENS3_IP8JSObjectEENS3_I4jsidEENS_13QualifiedBoolENS2_13MutableHandleINS2_5ValueEEERNS2_14ObjectOpResultE
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/vm/NativeObject.cpp:2194
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/vm/NativeObject.cpp:2230
  17:     0x55d5ce6bff2e - _ZN2js11PutPropertyEP9JSContextN2JS6HandleIP8JSObjectEENS3_I4jsidEENS2_13MutableHandleINS2_5ValueEEEb
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/vm/NativeObject.h:1451
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/jsobj.h:905
  18:     0x55d5ce839c7f - _ZN2js3jitL17DoSetPropFallbackEP9JSContextPNS0_13BaselineFrameEPNS0_18ICSetProp_FallbackEN2JS6HandleINS7_5ValueEEESA_NS7_13MutableHandleIS9_EE
                        at /home/user/Code/rust/servo/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/jit/BaselineIC.cpp:8367
  19:     0x7fb50ec5079f - <unknown>
fatal runtime error: Could not unwind stack, error = 5
Servo exited with return value -4

Note that https://vector.im gets updated over time, and accesses a chat system ( https://matrix.org ), and so the content may change. However, the code of vector.im at this time is release 0.2.0, so that at least can be reproduced.

[nox]

There are various Unicode-related bugs in the project that are getting fixed:

matrix-org/matrix-react-sdk#177
element-hq/element-web#990
element-hq/element-web#976

The last one seems to be the culprit here, given it happens when loading the list of rooms etc.

[notriddle]

So far, we've gotten Vector and Acid3. Replacing with FFFD seems like an acceptable choice in both cases (Acid3 allows it, and it's as good as any other way to deal with Vector).

[SimonSapin]

Stylo bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1355101

Spec issue to change CSSOM: w3c/csswg-drafts#1217

[SimonSapin]

w3c/csswg-drafts#1217 is resolved with w3c/csswg-drafts#1266: the spec introduces a CSSOMString in WebIDL, a typedef for either DOMString or USVString, chosen by implementations. Choosing USVString effectively means replacing surrogates with U+FFFD. This type is used for almost everything in CSSOM.

[sviter]

Got that error after login in Slack:

Found an unpaired surrogate in a DOM string. If you see this in real web content, please comment on #6564 Use -Z replace-surrogates on the command line to make this non-fatal.

URL:

Servo Version:

Servo 0.0.1-10779f0

Backtrace:

WARNING: <LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(11) }>: update_style_for_animation: Animation Atom('spin' type=inline) not found in style
WARNING: <LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(11) }>: update_style_for_animation: Animation Atom('spin' type=inline) not found in style
WARNING: <LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(11) }>: update_style_for_animation: Animation Atom('spin' type=inline) not found in style
WARNING: <LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(11) }>: update_style_for_animation: Animation Atom('spin' type=inline) not found in style
WARNING: <LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(11) }>: update_style_for_animation: Animation Atom('spin' type=inline) not found in style
WARNING: <LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: 

[notriddle]

Is it any team, or a particular team? For those not in the know, Slack is a multitenant program; naturally, every "team" (read: "tenant") has a lot of configuration that they can change.

If it's a particular team, then can a Servo dev join it? We're trying to answer the question of how Servo needs to handle the ill-formed UTF-16 to work.

[nox]

We have no immediate plan to support lone surrogates any time soon. Meanwhile, that Servo crashes on that on Slack means there is a bug on Slack somewhere.

[mateon1]

I hit this issue after following some links on Twitter. To reproduce, go to http://twitter.com/chrisledlin and scroll down a few screens worth.

EDIT (2017-09-10): Also affects mp3party.net, with no user interaction
EDIT (2017-09-15): mp3cc.com and zap.co.il, with no user interaction

[asajeffrey]

https://discoverthreejs.com/ produces

Found an unpaired surrogate in a DOM string. If you see this in real web content, please comment on https://github.com/servo/servo/issues/6564 Use `-Z replace-surrogates` on the command line to make this non-fatal. (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(3), index: PipelineIndex(12) }, at components/script/dom/bindings/conversions.rs:248)
stack backtrace:
   0: backtrace::backtrace::trace_unsynchronized
   1: <servo::backtrace::Print as core::fmt::Debug>::fmt
   2: core::fmt::write
   3: <std::io::stdio::Stdout as std::io::Write>::write_fmt
   4: std::io::stdio::_print
   5: servo::backtrace::print
   6: servo::main::{{closure}}
   7: std::panicking::rust_panic_with_hook
   8: std::panicking::begin_panic
   9: script::dom::bindings::conversions::jsstring_to_str
  10: script::dom::bindings::conversions::<impl mozjs::conversions::FromJSValConvertible for script::dom::bindings::str::DOMString>::from_jsval
  11: script::dom::bindings::codegen::Bindings::StorageBinding::StorageBinding::setItem::{{closure}}
  12: core::ops::function::FnOnce::call_once
  13: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  14: std::panicking::try::do_call
  15: __rust_maybe_catch_panic
  16: std::panicking::try
  17: std::panic::catch_unwind
  18: mozjs::panic::wrap_panic
  19: script::dom::bindings::codegen::Bindings::StorageBinding::StorageBinding::setItem
  20: CallJitMethodOp
  21: script::dom::bindings::utils::generic_call
  22: script::dom::bindings::utils::generic_method
  23: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  24: _ZL9InterpretP9JSContextRN2js8RunStateE
  25: _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
  26: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  27: _ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EE
  28: _ZN2js8fun_callEP9JSContextjPN2JS5ValueE
  29: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  30: _ZL9InterpretP9JSContextRN2js8RunStateE
  31: _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
  32: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  33: _ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EE
  34: _ZN2js8fun_callEP9JSContextjPN2JS5ValueE
  35: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  36: _ZL9InterpretP9JSContextRN2js8RunStateE
  37: _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
  38: _ZN2js13ExecuteKernelEP9JSContextN2JS6HandleIP8JSScriptEER8JSObjectRKNS2_5ValueENS_16AbstractFramePtrEPS9_
  39: _ZN2js7ExecuteEP9JSContextN2JS6HandleIP8JSScriptEER8JSObjectPNS2_5ValueE
  40: _ZL8EvaluateP9JSContextN2js9ScopeKindEN2JS6HandleIP8JSObjectEERKNS3_22ReadOnlyCompileOptionsERNS3_10SourceTextIDsEENS3_13MutableHandleINS3_5ValueEEE
  41: _ZN2JS12EvaluateUtf8EP9JSContextRKNS_22ReadOnlyCompileOptionsEPKcmNS_13MutableHandleINS_5ValueEEE
  42: mozjs::rust::wrappers::EvaluateUtf8
  43: script::dom::globalscope::GlobalScope::evaluate_script_on_global_with_result::{{closure}}
  44: profile_traits::time::profile
  45: script::dom::globalscope::GlobalScope::evaluate_script_on_global_with_result
  46: script::dom::htmlscriptelement::HTMLScriptElement::run_a_classic_script
  47: script::dom::htmlscriptelement::HTMLScriptElement::execute
  48: script::dom::servoparser::ServoParser::resume_with_pending_parsing_blocking_script
  49: script::dom::document::Document::process_pending_parsing_blocking_script
  50: script::dom::document::Document::pending_parsing_blocking_script_loaded
  51: <script::dom::htmlscriptelement::ClassicContext as net_traits::FetchResponseListener>::process_response_eof
  52: <net_traits::FetchResponseMsg as net_traits::Action<T>>::process
  53: <script::network_listener::ListenerTask<A,Listener> as script::task::TaskOnce>::run_once
  54: <script::task::CancellableTask<T> as script::task::TaskOnce>::run_once
  55: <T as script::task::TaskBox>::run_box
  56: script::script_thread::ScriptThread::handle_msg_from_script
  57: script::script_thread::ScriptThread::handle_msgs::{{closure}}
  58: script::script_thread::ScriptThread::profile_event
  59: script::script_thread::ScriptThread::handle_msgs
  60: script::script_thread::ScriptThread::start
  61: <script::script_thread::ScriptThread as script_traits::ScriptThreadFactory>::create::{{closure}}::{{closure}}
  62: profile_traits::mem::ProfilerChan::run_with_memory_reporting
  63: <script::script_thread::ScriptThread as script_traits::ScriptThreadFactory>::create::{{closure}}
  64: std::sys_common::backtrace::__rust_begin_short_backtrace
  65: std::thread::Builder::spawn_unchecked::{{closure}}::{{closure}}
  66: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  67: std::panicking::try::do_call
  68: __rust_maybe_catch_panic
  69: std::panicking::try
  70: std::panic::catch_unwind
  71: std::thread::Builder::spawn_unchecked::{{closure}}
  72: core::ops::function::FnOnce::call_once{{vtable.shim}}
  73: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once
  74: std::sys::unix::thread::Thread::new::thread_start
  75: _pthread_body
  76: _pthread_start

[SimonSapin]

Do we have a way to make that panic message print a JavaScript stack trace?

[jdm]

If we add some code that replicates https://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCDebug.cpp#37-60, yes.