Intermittent panic in fetch test (/fetch/stale-while-revalidate/fetch.html)

[jdm]

  ▶ CRASH [expected OK] /fetch/stale-while-revalidate/fetch.html
  │ 
  │ 
  │ 
  │ 
  │ 
  │ (servo:78333): GStreamer-WARNING **: External plugin loader failed. This most likely means that the plugin loader helper binary was not found or could not be run. You might need to set the GST_PLUGIN_SCANNER environment variable if your setup is unusual. This should normally not be required though.
  │ 0:00:00.040584256 78333 0x56078463e0a0 WARN      GST_PLUGIN_LOADING gstplugin.c:793:_priv_gst_plugin_load_file_for_registry: module_open failed: libasound.so.2: cannot open shared object file: No such file or directory
  │ 
  │ (servo:78333): GStreamer-WARNING **: Failed to load plugin '/home/servo/gst/lib/gstreamer-1.0/libgstalsa.so': libasound.so.2: cannot open shared object file: No such file or directory
  │ 0:00:00.040872261 78333 0x56078463e0a0 WARN      GST_PLUGIN_LOADING gstplugin.c:793:_priv_gst_plugin_load_file_for_registry: module_open failed: libmms.so.0: cannot open shared object file: No such file or directory
  │ 
  │ (servo:78333): GStreamer-WARNING **: Failed to load plugin '/home/servo/gst/lib/gstreamer-1.0/libgstmms.so': libmms.so.0: cannot open shared object file: No such file or directory
  │ 0:00:00.041028798 78333 0x56078463e0a0 WARN      GST_PLUGIN_LOADING gstplugin.c:793:_priv_gst_plugin_load_file_for_registry: module_open failed: libfaad.so.2: cannot open shared object file: No such file or directory
  │ 
  │ (servo:78333): GStreamer-WARNING **: Failed to load plugin '/home/servo/gst/lib/gstreamer-1.0/libgstfaad.so': libfaad.so.2: cannot open shared object file: No such file or directory
  │ assertion failed: response.is_some() (thread <unnamed>, at components/net/http_loader.rs:1166)
  │ stack backtrace:
  │    0: servo::main::{{closure}}::h5e7165b325cd5ec4 (0x56077e66d2bd)
  │    1: std::panicking::rust_panic_with_hook::h12ec097c8ec6e99f (0x56078199cf5c)
  │              at src/libstd/panicking.rs:477
  │    2: std::panicking::begin_panic::h1c12073e30056327 (0x56078069fcf5)
  │    3: net::http_loader::http_network_or_cache_fetch::wait_for_cached_response::h41839d21a93ed726 (0x560780649315)
  │    4: net::http_loader::http_network_or_cache_fetch::h38b4a9ed610d4f97 (0x560780646afc)
  │    5: net::http_loader::http_fetch::hc7cfaecec1be9ed2 (0x56078063f8f3)
  │    6: net::fetch::methods::scheme_fetch::h9116e440eefa59fe (0x560780652b9a)
  │    7: net::fetch::methods::main_fetch::h179d3d2c16c6f1bd (0x56078064f9b9)
  │    8: net::fetch::methods::fetch_with_cors_cache::hc9a4e58a54f0f225 (0x56078064f069)
  │    9: net::fetch::methods::fetch::hbe462d3901d30a3a (0x56078064eea6)
  │   10: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h08befa582e754e6a (0x56078078d5bf)
  │   11: _ZN3std9panicking3try7do_call17hd72a1ab76fb1b37cE.llvm.2672255499393434423 (0x56078069ffc4)
  │   12: __rust_maybe_catch_panic (0x5607819a6a3a)
  │              at src/libpanic_unwind/lib.rs:80
  │   13: _ZN77_$LT$rayon_core..job..HeapJob$LT$BODY$GT$$u20$as$u20$rayon_core..job..Job$GT$7execute17h74d9b71358531699E.llvm.11612821241170744107 (0x5607807de9bc)
  │   14: rayon_core::registry::WorkerThread::wait_until_cold::h998b4c1f5eccf721 (0x56078127664f)
  │   15: rayon_core::registry::ThreadBuilder::run::h623b552705f15d53 (0x560781274532)
  │   16: _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17h24d8835f800f7616E.llvm.7087006384071400510 (0x560781270df1)
  │   17: _ZN3std9panicking3try7do_call17h2ee6c28e57277723E.llvm.16261352180181574296 (0x560781271b70)
  │   18: __rust_maybe_catch_panic (0x5607819a6a3a)
  │              at src/libpanic_unwind/lib.rs:80
  │   19: core::ops::function::FnOnce::call_once{{vtable.shim}}::h9ff6be5e44e6f5f2 (0x560781272b07)
  │   20: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::hb018c40cf7dc8a37 (0x56078198b7bf)
  │              at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c/src/liballoc/boxed.rs:922
  │   21: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::h10e62b586f634658 (0x5607819a5d80)
  │              at /rustc/084beb83e0e87d673d5fabc844d28e8e8ae2ab4c/src/liballoc/boxed.rs:922
  │       std::sys_common::thread::start_thread::h6ada4f3bef71d083
  │              at src/libstd/sys_common/thread.rs:13
  │       std::sys::unix::thread::Thread::new::thread_start::h65408e9b6d37b640
  │              at src/libstd/sys/unix/thread.rs:79
  │   22: start_thread (0x7f4e886ff6ba)
  │   23: clone (0x7f4e86f9a41d)
  │   24: <unknown> (0x0)

[jdm]

I suspect this may be a regression from #24203.

[jdm]

For example, we could end up calling wait_for_cached_response with a None response if HttpCache::construct_response returns None.

[jdm]

cc @gterzian

[jdm]

Also we could end up calling wait_for_cached_response when revalidating and there will be a None response value.

[gterzian]

Ah yes, I added that assertion in #24318

For example, we could end up calling wait_for_cached_response with a None response if HttpCache::construct_response returns None.

Yes, and the assertion is nested within this block

servo/components/net/http_loader.rs

Line 1162 in 9c3e4a7

if let Some(ref ch) = *done_chan {

and logically if the done channel is set to something, the response should not be None, since that implies waiting on a channel for a non-existent response.

So if the cache constructs None, the channel should also be None.

Also we could end up calling wait_for_cached_response when revalidating and there will be a None response value.

Yes that's a good point, I think what is missing is setting DoneChan to None if the constructed response is expired at

servo/components/net/http_cache.rs

Line 343 in 9c3e4a7

let has_expired =

Although it's a strange case where the expired cached response is still receiving data, so the cache sets DoneChan to Some, yet then the cached response requires revalidation so the response is left as None, while the channel is still Some.

It might be clearer to set the DoneChan to None at

servo/components/net/http_loader.rs

Line 1109 in 9c3e4a7

if needs_revalidation {