Calling matchMedia during a MQL change event will panic

[jdm]

WeakMediaQueryListVec::evaluate_and_report_changes calls borrow() in order to iterate over the list of known media query lists. This dispatches a DOM event for each MQL that needs to report a change, which could end up calling Window.matchMedia which calls borrow_mut() on the list of known media query lists. It shouldn't be too difficult to put together a test demonstrating this.

[jdm]

<script>
  var m = window.matchMedia("(min-width: 400px)");
  m.addListener(function() {
    window.matchMedia("(min-width: 200px)");
  });
</script>

DOMRefCell<T> already borrowed: BorrowMutError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }, at /Users/rustbuild/src/rust-buildbot/slave/nightly-dist-rustc-mac/build/src/libcore/result.rs:845)
stack backtrace:
   0:        0x108361881 - backtrace::backtrace::trace::hc5756b68286ad624
   1:        0x108361f48 - backtrace::capture::Backtrace::new::h1daab5812ca1e5e3
   2:        0x10764f0fb - servo::main::{{closure}}::h51e7bf1c1cd17173
   3:        0x10d5c2b34 - std::panicking::rust_panic_with_hook::h33761bada49f3713
   4:        0x10d5c29c4 - std::panicking::begin_panic::h37a3cca06423c62e
   5:        0x10d5c2932 - std::panicking::begin_panic_fmt::h4c0c43306d52c68f
   6:        0x10d5c2897 - rust_begin_unwind
   7:        0x10d5ec030 - core::panicking::panic_fmt::hbd633f652cd3a047
   8:        0x1099782b3 - core::result::unwrap_failed::hfabfbdce95623041
   9:        0x1097341e9 - <core::result::Result<T, E>>::expect::hd84f90b90ccce39b
  10:        0x109dcc746 - <script::dom::bindings::cell::DOMRefCell<T>>::borrow_mut::h7be6b4412667a77c
  11:        0x10a02cc3f - script::dom::mediaquerylist::WeakMediaQueryListVec::push::h8096a25bfb8b45cb
  12:        0x10a10eacd - <script::dom::window::Window as script::dom::bindings::codegen::Bindings::WindowBinding::WindowBinding::WindowMethods>::MatchMedia::hbe67c3a6c8a469c4
  13:        0x10a7f979b - script::dom::bindings::codegen::Bindings::WindowBinding::WindowBinding::matchMedia::{{closure}}::hfb3799a18b2d95f6
  14:        0x10a900f48 - core::ops::FnOnce::call_once::h621a4d54e2a2bbfa
  15:        0x109d1d27a - <std::panic::AssertUnwindSafe<F> as core::ops::FnOnce<()>>::call_once::h6b5c6189a0a9ec6d
  16:        0x10945dc03 - std::panicking::try::do_call::hc12738e2da63891f
  17:        0x10d5c3a4a - __rust_maybe_catch_panic
  18:        0x10915b29f - std::panicking::try::h56fcee91be860746
  19:        0x109008a06 - std::panic::catch_unwind::h6eaffd742a84110a
  20:        0x108bc6b3c - js::panic::wrap_panic::ha856114db8feef54
  21:        0x10a7f93ed - script::dom::bindings::codegen::Bindings::WindowBinding::WindowBinding::matchMedia::h9faadfc783884682
  22:        0x10a96f0ab - CallJitMethodOp
  23:        0x109ed5df0 - script::dom::bindings::utils::generic_call::h2ebc2953a88ce385
  24:        0x109ed5f31 - script::dom::bindings::utils::generic_method::hf505106309006d4e
  25:        0x10b36e19a - 2js12CallJSNativeEP9JSContextPFbS1_jPN2JS5ValueEERKNS2_8CallArgs
  26:        0x10b348732 - 2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstruct
  27:        0x10b348b27 - _ZL12InternalCallP9JSContextRKN2js13AnyInvokeArgsE
  28:        0x10b34891c - 2js13CallFromStackEP9JSContextRKN2JS8CallArgs
  29:        0x10b33d48e - _ZL9InterpretP9JSContextRN2js8RunStateE
  30:        0x10b3324fe - 2js9RunScriptEP9JSContextRNS_8RunState
  31:        0x10b34880a - 2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstruct
  32:        0x10b348b27 - _ZL12InternalCallP9JSContextRKN2js13AnyInvokeArgsE
  33:        0x10b348bb5 - 2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_E
  34:        0x10b02ff24 - _Z20JS_CallFunctionValueP9JSContextN2JS6HandleIP8JSObjectEENS2_INS1_5ValueEEERKNS1_16HandleValueArrayENS1_13MutableHandleIS6_EE
  35:        0x10a43fd0a - script::dom::bindings::codegen::Bindings::EventListenerBinding::EventListener::HandleEvent::hc9bfb53db4a8a924
  36:        0x10a43f1d0 - script::dom::bindings::codegen::Bindings::EventListenerBinding::EventListener::HandleEvent_::hed636558e57926f0
  37:        0x109f95b3d - script::dom::eventtarget::CompiledEventListener::call_or_handle_event::h49c25658ca2c5ff7
  38:        0x109f8dc8f - script::dom::eventdispatcher::handle_event::ha3c300725665d6f9
  39:        0x109f8f62b - script::dom::eventdispatcher::inner_invoke::h8b2d3e42baad532f
  40:        0x109f8f4d2 - script::dom::eventdispatcher::invoke::hc202969327fe3527
  41:        0x109f8e207 - script::dom::eventdispatcher::dispatch_to_listeners::he9d923b31357e898
  42:        0x109f8ef22 - script::dom::eventdispatcher::dispatch_event::hf1bddb8e05ef25c5
  43:        0x109f976d8 - script::dom::eventtarget::EventTarget::dispatch_event::h4386687a6bcf1aaf
  44:        0x109f8d6ca - script::dom::event::Event::fire::h5b39bd20e50aa735
  45:        0x10a02d037 - script::dom::mediaquerylist::WeakMediaQueryListVec::evaluate_and_report_changes::h0e9241a5b0a316af
  46:        0x10a115af5 - script::dom::window::Window::evaluate_media_queries_and_report_changes::h74be31482b50a014
  47:        0x10a17431d - script::script_thread::ScriptThread::handle_resize_event::h2bd4d9a0ebe3e419
  48:        0x10a170d36 - script::script_thread::ScriptThread::handle_event::he43cb22c4bd6a730
  49:        0x10a156699 - script::script_thread::ScriptThread::handle_msgs::hd3a61b4ef5e1f60e
  50:        0x10a155ff5 - script::script_thread::ScriptThread::start::he0768822bcff4344
  51:        0x10a152134 - <script::script_thread::ScriptThread as script_traits::ScriptThreadFactory>::create::{{closure}}::{{closure}}::hfa1f681212bbb3ab
  52:        0x1088f4326 - profile_traits::mem::ProfilerChan::run_with_memory_reporting::hec776a9bb4728f7a
  53:        0x10a1527e4 - <script::script_thread::ScriptThread as script_traits::ScriptThreadFactory>::create::{{closure}}::ha301d403e6202d2c
  54:        0x109d020ca - <std::panic::AssertUnwindSafe<F> as core::ops::FnOnce<()>>::call_once::h39531ea8f5ba7044
  55:        0x1094af409 - std::panicking::try::do_call::hf99b9c224c5e27bc
  56:        0x10d5c3a4a - __rust_maybe_catch_panic
  57:        0x10932b103 - std::panicking::try::hf52a88bbd7f6b6e6
  58:        0x108ff35f4 - std::panic::catch_unwind::h49876181a9427cad
  59:        0x10905c4db - std::thread::Builder::spawn::{{closure}}::h0c4f7c40082ea9f1
  60:        0x1099dde92 - <F as alloc::boxed::FnBox<A>>::call_box::hdea39b2352668566
  61:        0x10d5c1ab4 - std::sys::imp::thread::Thread::new::thread_start::he1b44499d44cc4a4
  62:     0x7fff8e409898 - _pthread_body
  63:     0x7fff8e409729 - _pthread_start

[jdm]

@prampey Are you interested in working on this?

[prampey]

@jdm So you want me to handle the error explicitly, instead of letting it panic right?

[Ms2ger]

What does the spec say? I imagine it supports this, but I'm not quite sure what the expected behaviour is. (It looks like the spec uses DOM events now; not sure if our implementation does to.)

[jdm]

https://drafts.csswg.org/cssom-view/#evaluate-media-queries-and-report-changes just says iterate over all the media query list objects for a document and evaluate if their matching status changed. That means we need to support this case by allowing the change event to append new elements to the list of known media query list objects.

[prampey]

@jdm But isn't the MediaQueryList object created after the change event is fired? matchMedia is again called inside the listener and we still might not be able to borrow_mut.

Perhaps, we could collect all the events formed during evaluate_and_report_changes and bubble them all at once, just after the mutable reference to mql goes out of scope. This way we might not have to check for more edge cases that might pop up in the future.

[jdm]

That sounds like a good solution to me.

[prampey]

@jdm My bad. I didn't realise that in order to fire the events, you still need a reference to mql:
event.upcast::<Event>().fire(mql.upcast::<EventTarget>())
So as we fire them one by one, we are still required to keep a reference to mql.
I have thought of another way. Perhaps, we could 'try' borrowing the reference to mql every time we need to fire an event and handle the error everytime we don't get the reference. Then, we can keep looping this until every event is fired whenever the reference to mqlis freed.
I'll come up with the code for this in sometime.

[jdm]

We could store a vector of changed MediaQueryList values and iterate over those using rooted_vec!.

[prampey]

@jdm Sounds simpler. But does rooted_vec! eliminate the use-after-free vulnerability?

[jdm]

Yes - it ensures that the elements stored inside of it cannot be GCed, because they are explicitly rooted.

[prampey]

@jdm So how do I store the vector with rooted_vec!?

[jdm]

@prampey You can declare it like this and push elements to it like this. Does that answer the question?

[prampey]

@jdm Yeah thanks for that.
So if I'm going to store the MediaQueryList values and use them later, I need to make copies of them right? However, Clone and Copy traits haven't been implemented.

[jdm]

You'll want to store the result of JS::from_ref.

[prampey]

@jdm Thanks. It works without errors now. Are there any tests to be written?

[jdm]

An automated test that demonstrates this case working would be ideal. See test/wpt/README.md for instruction on writing new tests, and it can live in tests/wpt/mozilla/tests/mozilla.

[prampey]

@jdm Okay so I decided to automate the MediaQuery matching up to 2 levels deep just to make sure that every instance has its own copy of Media Query lists to deal with.
First, I resize the window in the body tag to trigger the 'min-width' media listener and then inside, I resize again to trigger the 'orientation' media query.
Here's the code:

<html>

<head>
    <script src="/resources/testharness.js"></script>
    <script src="/resources/testharnessreport.js"></script>
    <script>
        test(function() {
            var match1 = window.matchMedia("(min-width: 400px)");
            match1.addListener(function() {
                var match2 = window.matchMedia("(orientation: landscape)");
                assert_not_equals(match2, undefined);
                match2.addListener(function() {

                    var match3 = window.matchMedia("(orientation: portrait)");
                    assert_not_equals(match2, undefined);
                });
                window.resizeTo(600, 400);
            });
        }, "No mql reference borrow clashes found");
    </script>
</head>

<body onload="window.resizeTo(600,600)"></body>

</html>

The code runs properly. Is this enough?

[nox]

@prampey Make a PR and we will review it.

[jdm]

@prampey The idea is right, but you will need to use async_test instead of test. I'm pretty sure that right now the test will be declared a success before the match1 listener is invoked.

[prampey]

@jdm Yeah, thanks. I finally found the web-platform-test documentation and changed it :). I'll send in the PR.