Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report panics using the top-level frame id rather than the pipeline id #14173

Merged

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Nov 11, 2016

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 Reviewable

@highfive
Copy link

highfive commented Nov 11, 2016

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
Copy link

highfive commented Nov 11, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 11, 2016

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 asajeffrey force-pushed the asajeffrey:script-thread-stores-top-level-frame-id branch 2 times, most recently from b74fe56 to 3ee4354 Nov 11, 2016
@asajeffrey asajeffrey mentioned this pull request Nov 14, 2016
4 of 4 tasks complete
@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 15, 2016

@highfive highfive assigned paulrouget and unassigned larsbergstrom Nov 15, 2016
@asajeffrey asajeffrey force-pushed the asajeffrey:script-thread-stores-top-level-frame-id branch from 3ee4354 to 568cb37 Nov 15, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Nov 17, 2016

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-thread-stores-top-level-frame-id branch from 568cb37 to c228a4c Nov 18, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 18, 2016

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

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) {

This comment has been minimized.

Copy link
@paulrouget

paulrouget Nov 21, 2016

Contributor

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.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Nov 21, 2016

Author Member

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.

This comment has been minimized.

Copy link
@paulrouget

paulrouget Nov 21, 2016

Contributor

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).

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Nov 21, 2016

Author Member

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
Copy link
Contributor

paulrouget commented Nov 21, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2016

🔑 Insufficient privileges

@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 21, 2016

Ah, you're not on the blessed list?

@KiChjang
Copy link
Member

KiChjang commented Nov 21, 2016

@bors-servo r=paulrouget delegate=paulrouget

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2016

✌️ @paulrouget can now approve this pull request

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2016

📌 Commit c228a4c has been approved by paulrouget

@asajeffrey
Copy link
Member Author

asajeffrey commented Nov 21, 2016

Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2016

Testing commit c228a4c with merge bbc8216...

bors-servo added a commit that referenced this pull request Nov 21, 2016
…e-id, r=paulrouget

Report panics using the top-level frame id rather than the pipeline id

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14173)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2016

@bors-servo bors-servo merged commit c228a4c into servo:master Nov 21, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.