Removed panic channel, replaced by integrated logging and issue reporting #12468

asajeffrey · 2016-07-15T21:52:21Z

Remove the previous ad hoc panic channel, replace it by an integrated logging and panicking mechanism, including crash reporting. All thread panics are now reported, not just content threads.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11838
These changes do not require tests because we don't test error reporting

This change is

highfive · 2016-07-15T21:52:25Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/bindings/global.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script_layout_interface/message.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/workerglobalscope.rs

highfive · 2016-07-15T21:52:26Z

Warning

These commits modify gfx and script code, but no tests are modified. Please consider adding a test!

asajeffrey · 2016-07-15T22:03:43Z

This impacts issue reporting, so cc @Gozala, and removes almost all of the threading / panicking code from util, so cc @Manishearth.

Manishearth · 2016-07-16T04:14:58Z

components/constellation/constellation.rs

        if opts::get().hard_fail {
            // It's quite difficult to make Servo exit cleanly if some threads have failed.
            // Hard fail exists for test runners so we crash and that's good enough.
-            error!("Pipeline failed in hard-fail mode.  Crashing!");
+            warn!("Pipeline failed in hard-fail mode.  Crashing!");


Won't this feed back into the logging chan?

Yes, but the very next thing is an exit, so it shouldn't matter too much. I made it a warn! rather than an error! so we don't trigger the issue reporting in the (rather unlikely) --hard-fail --browserhtml case.

bors-servo · 2016-07-18T17:55:17Z

☔ The latest upstream changes (presumably #11727) made this pull request unmergeable. Please resolve the merge conflicts.

asajeffrey · 2016-07-18T20:21:43Z

Rebased.

emilio · 2016-07-19T21:33:34Z

First, sorry for the delay reviewing. I love negative diffs, and the code looks good to me.

I left two (uber-minor) nits, and one question.

r=me with or without the nits addressed, but preferably with the question answered please :)

-S-awaiting-review +S-awaiting-answer

Reviewed 21 of 21 files at r1.
Review status: 12 of 21 files reviewed at latest revision, 4 unresolved discussions.

components/constellation/constellation.rs, line 333 [r1] (raw file):

    fn log(&self, record: &LogRecord) {
        if let Some(entry) = log_entry(record) {
            let pipeline_id = PipelineId::installed();

Should we somehow detect when a thread is expected to have a pipeline id installed (via thread_state), and warn (even with a simple message to stderr, to prevent retriggering the mechanism) that is not found?

Or something like debug_assert!(!thread_state::get().is_script() || pipeline_id.is_some()) // etc, for example.

components/msg/constellation_msg.rs, line 313 [r1] (raw file):

    pub fn install(id: PipelineId) {
        PIPELINE_ID.with(|tls| { tls.set(Some(id)) })

nit: The braces here might be excessive.

components/msg/constellation_msg.rs, line 316 [r1] (raw file):

    }

    pub fn installed() -> Option<PipelineId> {

ditto.

Comments from Reviewable

bors-servo · 2016-07-20T11:26:08Z

☔ The latest upstream changes (presumably #12426) made this pull request unmergeable. Please resolve the merge conflicts.

asajeffrey · 2016-07-20T15:04:25Z

Rebased.

asajeffrey · 2016-07-20T15:07:21Z

It would be nice to add a check that the thread-local pipeline id is set whenever the thread state says it should be, but I'd be concerned about putting it in the logging code. For example, logging during panic is quite common, and if the logging panics, then we'll get a double panic and the whole of servo will close. So I'd rather leave this un-asserted, even in debug mode.

asajeffrey · 2016-07-20T15:12:54Z

@bors-servo r=emilio

bors-servo · 2016-07-20T15:12:56Z

📌 Commit fd27ca6 has been approved by emilio

bors-servo · 2016-07-20T15:23:54Z

⌛ Testing commit fd27ca6 with merge 29b0dea...


        Auto merge of #12468 - asajeffrey:constellation-remove-panic-channel,…

… r=emilio Removed panic channel, replaced by integrated logging and issue reporting  Remove the previous ad hoc panic channel, replace it by an integrated logging and panicking mechanism, including crash reporting. All thread panics are now reported, not just content threads. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11838 - [X] These changes do not require tests because we don't test error reporting   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12468)

bors-servo · 2016-07-21T02:23:17Z

💔 Test failed - linux-rel

asajeffrey · 2016-07-21T02:50:01Z

I think #12516 is a blocker for this landing, unless we disable some of the tests. In --mutliprocess mode, the intermittents become pretty consistent, and in the master branch that's fine because the panic is silently ignored. This PR sends the panic to the constellation, causing the tests to fail (which they should be, the panics shouldn't be dropped).

I think the thing to do may be to disable the consistently failing tests: /eventsource/dedicated-worker/eventsource-constructor-non-same-origin.htm and /eventsource/eventsource-constructor-non-same-origin.htm.


        Removed panic channel, replaced by integrated logging and issue repor…

…ting.

asajeffrey · 2016-07-21T02:59:08Z

Replaced the --hard-fail warning with a println, so the backtrace appears in wpt-test runs.

@bors-servo r=emilio (I'm expecting this to fail because of #12516 but you never know).

bors-servo · 2016-07-21T02:59:09Z

📌 Commit c889900 has been approved by emilio

bors-servo · 2016-07-21T03:26:37Z

⌛ Testing commit c889900 with merge 0bb27dd...


        Auto merge of #12468 - asajeffrey:constellation-remove-panic-channel,…

… r=emilio Removed panic channel, replaced by integrated logging and issue reporting  Remove the previous ad hoc panic channel, replace it by an integrated logging and panicking mechanism, including crash reporting. All thread panics are now reported, not just content threads. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11838 - [X] These changes do not require tests because we don't test error reporting   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12468)

bors-servo · 2016-07-21T04:23:13Z

💔 Test failed - linux-rel

asajeffrey · 2016-07-21T15:36:02Z

#12518 seems to fix the problems with #12516, hooray!

@bors-servo retry

bors-servo · 2016-07-21T16:20:38Z

⌛ Testing commit c889900 with merge df1b00d...


        Auto merge of #12468 - asajeffrey:constellation-remove-panic-channel,…

… r=emilio Removed panic channel, replaced by integrated logging and issue reporting  Remove the previous ad hoc panic channel, replace it by an integrated logging and panicking mechanism, including crash reporting. All thread panics are now reported, not just content threads. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11838 - [X] These changes do not require tests because we don't test error reporting   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12468)

bors-servo · 2016-07-21T17:23:22Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned emilio Jul 15, 2016

highfive added the S-awaiting-review label Jul 15, 2016

Manishearth reviewed Jul 16, 2016
View changes

highfive added the S-needs-rebase label Jul 18, 2016

asajeffrey force-pushed the asajeffrey:constellation-remove-panic-channel branch 2 times, most recently from 559a13c to b3019c6 Jul 18, 2016

asajeffrey removed the S-needs-rebase label Jul 18, 2016

emilio added S-awaiting-answer and removed S-awaiting-review labels Jul 19, 2016

highfive added the S-needs-rebase label Jul 20, 2016

asajeffrey force-pushed the asajeffrey:constellation-remove-panic-channel branch from b3019c6 to fd27ca6 Jul 20, 2016

highfive added the S-awaiting-review label Jul 20, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase S-awaiting-answer labels Jul 20, 2016

asajeffrey mentioned this pull request Jul 20, 2016

Handle panics for all pipeline-associated threads #11246

Closed

2 of 9 tasks complete

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 21, 2016

asajeffrey mentioned this pull request Jul 21, 2016

Intermittent timeout in /eventsource/dedicated-worker/eventsource-prototype.htm and /eventsource/dedicated-worker/eventsource-url.htm #12516

Closed

Removed panic channel, replaced by integrated logging and issue repor…

Loading status checks…

c889900

…ting.

asajeffrey force-pushed the asajeffrey:constellation-remove-panic-channel branch from 64b085f to c889900 Jul 21, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jul 21, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 21, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 21, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 21, 2016

bors-servo merged commit c889900 into servo:master Jul 21, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 21, 2016

servo / servo

Removed panic channel, replaced by integrated logging and issue reporting #12468

Removed panic channel, replaced by integrated logging and issue reporting #12468

asajeffrey commented Jul 15, 2016 •

edited by larsbergstrom

highfive commented Jul 15, 2016

highfive commented Jul 15, 2016

asajeffrey commented Jul 15, 2016

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jul 18, 2016

asajeffrey commented Jul 18, 2016

emilio commented Jul 19, 2016

bors-servo commented Jul 20, 2016

asajeffrey commented Jul 20, 2016

asajeffrey commented Jul 20, 2016

asajeffrey commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

servo / servo

Join GitHub today

Removed panic channel, replaced by integrated logging and issue reporting #12468

Removed panic channel, replaced by integrated logging and issue reporting #12468

Conversation

asajeffrey commented Jul 15, 2016 • edited by larsbergstrom

highfive commented Jul 15, 2016

highfive commented Jul 15, 2016

asajeffrey commented Jul 15, 2016

This comment has been minimized.

Manishearth Jul 16, 2016

This comment has been minimized.

asajeffrey Jul 17, 2016 • edited by jdm

bors-servo commented Jul 18, 2016

asajeffrey commented Jul 18, 2016

emilio commented Jul 19, 2016

bors-servo commented Jul 20, 2016

asajeffrey commented Jul 20, 2016

asajeffrey commented Jul 20, 2016

asajeffrey commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

asajeffrey commented Jul 21, 2016

bors-servo commented Jul 21, 2016

bors-servo commented Jul 21, 2016

asajeffrey commented Jul 15, 2016 •

edited by larsbergstrom

asajeffrey Jul 17, 2016 •

edited by jdm