Report panics using the top-level frame id rather than the pipeline id by asajeffrey · Pull Request #14173 · servo/servo

asajeffrey · 2016-11-11T16:23:22Z

At the moment, we report panics from script and layout using the root pipeline id. Once we are sharing more script threads, there won't be a root pipeline id any more. The plan is only to share script threads in the same tab, so there will be a top-level frame id that all the pipelines have in common. This PR reports panics using that top-level frame id.

This mostly makes a difference to the browser API: rather than targeting a mozbrowsererror event at the root iframe of the script thread that panicked, it targets it at the containing mozbrowser iframe.

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

This change is

highfive · 2016-11-11T16:23:26Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/dedicatedworkerglobalscope.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
@KiChjang: components/script/dom/dedicatedworkerglobalscope.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

highfive · 2016-11-11T16:23:27Z

Warning

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

asajeffrey · 2016-11-11T16:25:56Z

I learned the hard way not to cc people or mention issues in the PR summary...

This is part of fixing #633, hopefully the last remaining place that script threads assume there's a root pipeline.

cc @jdm @ConnorGBrewster @paulrouget

asajeffrey · 2016-11-15T15:55:49Z

r? @paulrouget

bors-servo · 2016-11-17T23:46:07Z

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

asajeffrey · 2016-11-18T00:10:38Z

@paulrouget? This is blocking fixing #633, so it would be nice to land it!

paulrouget · 2016-11-21T16:18:44Z

components/constellation/constellation.rs

-        warn!("Mozbrowser error after root pipeline closed.");
+        match self.frames.get(&top_level_frame_id) {
+            None => warn!("Mozbrowser error after top-level frame closed."),
+            Some(frame) => match self.pipelines.get(&frame.current.pipeline_id) {


Are we 100% sure it's the current pipeline that crashed?
As in, the pipeline we get in handle_send_error, is it always the current pipeline of that frame?

If not, we should pass pipeline_id to handle_panic and trigger_mozbrowsererror, not the frame_id.

In the case of handle_send_error, all we know is that we tried sending to a pipeline which had closed its constellation channel (most likely because the pipeline exited). We could try being more precise in this case, but most panics come in via as log messages, where all we get is the frame id, not the pipeline id.

Re-reading that code. I was confused about using frame.current, which is only used to get the parent frame (not to identify the failing pipeline).

Yes, this is an artefact of the parent info being stored in the pipeline rather than the frame, where it should be. At some point we might fix that.

paulrouget · 2016-11-21T16:58:44Z

@bors-servo r+

bors-servo · 2016-11-21T17:07:46Z

🔑 Insufficient privileges

asajeffrey · 2016-11-21T17:46:43Z

Ah, you're not on the blessed list?

KiChjang · 2016-11-21T20:08:53Z

@bors-servo r=paulrouget delegate=paulrouget

bors-servo · 2016-11-21T20:08:54Z

✌️ @paulrouget can now approve this pull request

bors-servo · 2016-11-21T20:08:54Z

📌 Commit c228a4c has been approved by paulrouget

asajeffrey · 2016-11-21T20:09:55Z

Thanks!

bors-servo · 2016-11-21T21:52:01Z

⌛ Testing commit c228a4c with merge bbc8216...

…e-id, r=paulrouget Report panics using the top-level frame id rather than the pipeline id  At the moment, we report panics from script and layout using the root pipeline id. Once we are sharing more script threads, there won't be a root pipeline id any more. The plan is only to share script threads in the same tab, so there will be a top-level frame id that all the pipelines have in common. This PR reports panics using that top-level frame id. This mostly makes a difference to the browser API: rather than targeting a `mozbrowsererror` event at the root iframe of the script thread that panicked, it targets it at the containing mozbrowser iframe. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because we don't test crash reporting   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14173)

bors-servo · 2016-11-21T23:20:17Z

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

asajeffrey added A-constellation Involves the constellation A-content/script Related to the script thread I-refactor No impact; the issue is one of maintainability or tidiness. Proposed solution requires refactoring. labels Nov 11, 2016

highfive assigned larsbergstrom Nov 11, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 11, 2016

asajeffrey force-pushed the script-thread-stores-top-level-frame-id branch 2 times, most recently from b74fe56 to 3ee4354 Compare November 11, 2016 18:07

asajeffrey mentioned this pull request Nov 14, 2016

Share script threads by tab and by eTLD+1 #14211

Merged

4 tasks

highfive assigned paulrouget and unassigned larsbergstrom Nov 15, 2016

asajeffrey force-pushed the script-thread-stores-top-level-frame-id branch from 3ee4354 to 568cb37 Compare November 15, 2016 19:50

highfive added the S-needs-rebase There are merge conflict errors. label Nov 17, 2016

Report errors using the top-level frame id rather than the pipeline id.

c228a4c

asajeffrey force-pushed the script-thread-stores-top-level-frame-id branch from 568cb37 to c228a4c Compare November 18, 2016 00:08

asajeffrey removed the S-needs-rebase There are merge conflict errors. label Nov 18, 2016

paulrouget reviewed Nov 21, 2016

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 21, 2016

bors-servo merged commit c228a4c into servo:master Nov 21, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 21, 2016

This was referenced Nov 21, 2016

Implement synchronous about:blank loading. #13996

Merged

Rename ScriptChan in constellation to EventLoop #14260

Merged

Use unguessable PipelineIds and FrameIds #14283

Closed

[WIP] Implement History State #13781

Closed

Uh oh!

Conversation

asajeffrey commented Nov 11, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Nov 11, 2016

Uh oh!

highfive commented Nov 11, 2016

Uh oh!

asajeffrey commented Nov 11, 2016

Uh oh!

asajeffrey commented Nov 15, 2016

Uh oh!

bors-servo commented Nov 17, 2016

Uh oh!

asajeffrey commented Nov 18, 2016

Uh oh!

paulrouget Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

paulrouget Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Nov 21, 2016

Choose a reason for hiding this comment

Uh oh!

paulrouget commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

asajeffrey commented Nov 21, 2016

Uh oh!

KiChjang commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

asajeffrey commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

bors-servo commented Nov 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asajeffrey commented Nov 11, 2016 •

edited by larsbergstrom

Loading