assertion failed: self.load.is_none()

[mateon1]

While trying to reproduce #18438 (http://baskino.co), I hit this assertion instead. I could not reproduce it a second time (but that URL is a gold mine).

Note: This panic happened after the Shutting down the Constellation after generating an output file or exit flag specified message was printed.

stack trace:

assertion failed: self.load.is_none() (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(2) }, at /shared/dev/rust/servo/components/script/document_loader.rs:78)
stack backtrace:
   0:     0x555ab12320dc - backtrace::backtrace::trace::h0f1eee7f17c620ad
   1:     0x555ab1232112 - backtrace::capture::Backtrace::new::h1abdde591da67f3d
   2:     0x555aaff91865 - servo::main::{{closure}}::h2775028620a930e9
   3:     0x555ab2746b26 - std::panicking::rust_panic_with_hook
                        at /checkout/src/libstd/panicking.rs:612
   4:     0x555ab065207a - std::panicking::begin_panic::h99ca1191232fecb5
   5:     0x555ab09297f1 - core::ptr::drop_in_place::h9cca0903c5314d35
   6:     0x555ab0783c34 - std::panicking::try::do_call::h933e23f632df6f0e
   7:     0x555ab274dbcc - panic_unwind::__rust_maybe_catch_panic
                        at /checkout/src/libpanic_unwind/lib.rs:99
   8:     0x555ab0f77133 - script::dom::bindings::codegen::Bindings::HTMLImageElementBinding::HTMLImageElementBinding::_finalize::hacf9178a2b564947
   9:     0x555ab18ceec9 - _ZNK2js5Class10doFinalizeEPNS_6FreeOpEP8JSObject
                        at /shared/dev/rust/servo/target/release/build/mozjs_sys-6a7905cfd7acaabc/out/dist/include/js/Class.h:815
  10:     0x555ab18d76a6 - _ZN8JSObject8finalizeEPN2js6FreeOpE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsobjinlines.h:85
  11:     0x555ab18f1941 - _ZN2js2gc5Arena8finalizeI8JSObjectEEmPNS_6FreeOpENS0_9AllocKindEm
  12:     0x555ab18c5bc5 - FinalizeTypedArenas<JSObject>
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:505
  13:     0x555ab1893490 - FinalizeArenas
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:539
  14:     0x555ab18d9eec - _ZN2js2gc10ArenaLists16forceFinalizeNowEPNS_6FreeOpENS0_9AllocKindENS1_14KeepArenasEnumEPPNS0_5ArenaE
  15:     0x555ab18d9d95 - _ZN2js2gc10ArenaLists11finalizeNowEPNS_6FreeOpENS0_9AllocKindENS1_14KeepArenasEnumEPPNS0_5ArenaE
  16:     0x555ab189ae24 - _ZN2js2gc10ArenaLists30queueForegroundObjectsForSweepEPNS_6FreeOpE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:2839
  17:     0x555ab18a2e49 - _ZN2js2gc9GCRuntime22beginSweepingZoneGroupERNS_26AutoLockForExclusiveAccessE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:5127
  18:     0x555ab18a35a3 - _ZN2js2gc9GCRuntime15beginSweepPhaseEbRNS_26AutoLockForExclusiveAccessE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:5215
  19:     0x555ab18a5e0e - _ZN2js2gc9GCRuntime23incrementalCollectSliceERNS_11SliceBudgetEN2JS8gcreason6ReasonERNS_26AutoLockForExclusiveAccessE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:5937
  20:     0x555ab18a696e - _ZN2js2gc9GCRuntime7gcCycleEbRNS_11SliceBudgetEN2JS8gcreason6ReasonE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:6174
  21:     0x555ab18a6eaf - _ZN2js2gc9GCRuntime7collectEbNS_11SliceBudgetEN2JS8gcreason6ReasonE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:6287
  22:     0x555ab18a723e - _ZN2js2gc9GCRuntime2gcE18JSGCInvocationKindN2JS8gcreason6ReasonE
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsgc.cpp:6354
  23:     0x555ab17e0fff - _Z5JS_GCP9JSRuntime
                        at /shared/dev/rust/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/834ce35/mozjs/js/src/jsapi.cpp:1340
  24:     0x555ab0d510f2 - script::dom::window::Window::clear_js_runtime::h05b206a41cc0f413
  25:     0x555ab0dbde0a - script::script_thread::ScriptThread::handle_exit_pipeline_msg::h6f3c5cff0dd87471
  26:     0x555ab0da94e5 - script::script_thread::ScriptThread::handle_msg_from_constellation::h37d80dc8659cb70e
  27:     0x555ab0da7468 - script::script_thread::ScriptThread::handle_msgs::{{closure}}::hdd3fd9041a1efa8f
  28:     0x555ab0da38cf - script::script_thread::ScriptThread::handle_msgs::h4aadf07f4dff3bf3
  29:     0x555ab0d9eceb - script::script_thread::ScriptThread::start::h543e6bc16496788a
  30:     0x555ab064e8f0 - std::sys_common::backtrace::__rust_begin_short_backtrace::hcbce24426485d7f2
  31:     0x555ab077b605 - std::panicking::try::do_call::h8ef268d0617cbd63
  32:     0x555ab274dbcc - panic_unwind::__rust_maybe_catch_panic
                        at /checkout/src/libpanic_unwind/lib.rs:99
  33:     0x555ab09850b2 - <F as alloc::boxed::FnBox<A>>::call_box::h4e198348b3252fbb
  34:     0x555ab27457ab - alloc::boxed::{{impl}}::call_once<(),()>
                        at /checkout/src/liballoc/boxed.rs:692
                         - std::sys_common::thread::start_thread
                        at /checkout/src/libstd/sys_common/thread.rs:21
                         - std::sys::imp::thread::{{impl}}::new::thread_start
                        at /checkout/src/libstd/sys/unix/thread.rs:84
  35:     0x7fe4b3edc493 - start_thread
  36:     0x7fe4b3a09abe - __clone
  37:                0x0 - <unknown>
ERROR:servo: assertion failed: self.load.is_none()

[jdm]

This is a symptom of the more general problem we have that quitting Servo while a load is in progress triggers a panic.

[ejmg]

Fairly sure I have hit upon the same bug and have found a consistent way to reproduce it. By total chance, here is what I did to reproduce this bug (and continue to do so):

1. Run ./mach run https://google.com
2. look up "wu tang clan"
3. click on the wikipedia entry
4. wait for it to crash

I'm fairly sure this workflow reproduces for just about anything. Doing the steps above but searching for "GZA" and clicking on the resulting wikipedia link yields the same result, etc.

The only difference between what I am experiencing and the original issue is that I seem to get about three of these errors and a corresponding panic log about three times as big.

Here's a gist of my crash output when attempting to look up the wu tang clan.

[jdm]

Ok, I think this assertion is misguided and not helping at all. We should make the Drop implementation for LoadBlocker do an if let check on self.load.take(), and call self.doc.finish_load if there is a load present.

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[jdm]

Code: components/script/dom/document_loader.rs

[ejmg]

@highfive: assign me

[highfive]

Hey @ejmg! Thanks for your interest in working on this issue. It's now assigned to you!

[ejmg]

So I changed

impl Drop for LoadBlocker {
    fn drop(&mut self) {
        if !thread::panicking() {
            debug_assert!(self.load.is_none());
        }
    }
}

to

impl Drop for LoadBlocker {
    fn drop(&mut self) {
        if let Some(n) = self.load.take() {
            self.doc.finish_load(n);
        }
    }
}

Running through the steps I documented above a few times, I don't hit the error anymore. Clicking around to on links to other Wiki entries also doesn't seem to trigger a panic of any sort, let alone of the one described in this issue.

How would one go about implementing a test for this change?

[ejmg]

Correction: running it two more times made me hit the following error (stack elided):

called `Option::unwrap()` on a `None` value (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(1) }, at src/libcore/option.rs:345)
stack backtrace:
   0:     0x55ffce826e86 - backtrace::backtrace::libunwind::trace::h7136d75a088cdea6

Here is the full log. Is this related to my edit? It looks similar to the issue linked earlier, #18438.

Update: I am having difficulty replicating it, so maybe not. Hard to say for something that is intermittent.

[jdm]

Mmm, yes, the backtrace from that panic shows that it is caused by the change that I asked you to make:

  14:     0x55ffca0be4ff - script::dom::document::Document::finish_load::hcf16d50a4509c9ec
                        at components/script/dom/document.rs:1784
  15:     0x55ffc99a3338 - <script::document_loader::LoadBlocker as core::ops::drop::Drop>::drop::hc0bb5931aa34d758
at components/script/document_loader.rs:81

This happens when the LoadBlocker is dropped during the final GC sweep of all of the JS objects when a ScriptThread is shutting down, which is why ScriptThread::mark_document_with_no_blocked_loads ends up unwrapping a None SCRIPT_THREAD_ROOT value.

[jdm]

It's possible that we should just use fewer unwrap calls in methods like mark_document_with_no_blocked_loads and silently ignore it if there's no script thread available any more.

[ejmg]

Here's what I've changed mark_document_no_blocked_loads() to:

    pub fn mark_document_with_no_blocked_loads(doc: &Document) {
        SCRIPT_THREAD_ROOT.with(|root| {
            unsafe {
            if let Some(script_thread) = root.get() {
                (*script_thread)
                    .docs_with_no_blocking_loads
                    .borrow_mut()
                    .insert(Dom::from_ref(doc));
            }}
       })
    }