Skip to content

/ servo

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

Removed some sources of panic from script thread. #11695

Merged
merged 1 commit into from Jun 24, 2016
Merged

Removed some sources of panic from script thread. #11695

merged 1 commit into from Jun 24, 2016

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Jun 9, 2016 •
edited by larsbergstrom

This PR needs some thought!

It removes some sources of panic from script_thread.rs, caused by pipeline lookup failure. It's incomplete, as there are some uses of get_browsing_context elsewhere in the code.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11693 and #11685 (and probably some other intermittents)
  • These changes do not require tests because it is fixing intermittent panic

This change is Reviewable
@highfive
Copy link

highfive commented Jun 9, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/script_thread.rs
@highfive
Copy link

highfive commented Jun 9, 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 Jun 9, 2016

cc @Ms2ger, @Manishearth and @mbrubeck
This was referenced Jun 10, 2016
Closed
Closed
@bors-servo
Copy link
Contributor

bors-servo commented Jun 11, 2016

☔ The latest upstream changes (presumably #11680) made this pull request unmergeable. Please resolve the merge conflicts.
@asajeffrey asajeffrey force-pushed the asajeffrey:script-thread-dont-panic branch from ae665c8 to 3c75ac9 Jun 12, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 12, 2016

Rebased.
@cbrewster cbrewster mentioned this pull request Jun 19, 2016
Merged
4 of 5 tasks complete
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 22, 2016

I feel like this is too ad-hoc in places. I think in general when we get a message with a pipeline id, we should immediately get the browsing context and return, before doing anything else.

Also, why don't we use find_child_context everywhere? That would also avoid a panic when the root browsing context is gone.

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/script_thread.rs, line 1089 [r2] (raw file):

        if let Some(ref context) = self.find_child_context(id) {
            let window = match context.find(id) {
                Some(browsing_context) => browsing_context.active_window(),

This is the same as context.

Comments from Reviewable
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 23, 2016

I'm not sure why this is more ad hoc than what we had before: I just replaced all uses of get_browsing_context(&context,id) by a match on context.find(id).

OK, I see what you mean about using find_child_context rather than context.find on the root context. Is that a separate PR?

Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/script_thread.rs, line 1089 [r2] (raw file):

Previously, Ms2ger wrote…

This is the same as context.

Fixed.

Comments from Reviewable
@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 24, 2016

r+ after squash, I guess.
@asajeffrey
Removed some sources of panic from script thread.
Loading status checks…
9da00e2
@asajeffrey asajeffrey force-pushed the asajeffrey:script-thread-dont-panic branch from b54a37c to 9da00e2 Jun 24, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2016

@Ms2ger you don't need to sound so excited :)
@bors-servo r=Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

📌 Commit 9da00e2 has been approved by Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

⌛ Testing commit 9da00e2 with merge 654104e...
bors-servo added a commit that referenced this pull request Jun 24, 2016
@bors-servo
Auto merge of #11695 - asajeffrey:script-thread-dont-panic, r=Ms2ger
654104e 
Removed some sources of panic from script thread.

<!-- Please describe your changes on the following line: -->
**This PR needs some thought!**

It removes some sources of panic from script_thread.rs, caused by pipeline lookup failure. It's incomplete, as there are some uses of `get_browsing_context` elsewhere in the code.

---
<!-- 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 fix #11693 and #11685 (and probably some other intermittents)
- [X] These changes do not require tests because it is fixing intermittent panic

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

💔 Test failed - linux-dev
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2016

@bors-servo retry

  • etc/ci/manifest_changed.sh failing for some reason.
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

⚡ Previous build results for android, arm32, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only arm64, linux-dev, linux-rel...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

💔 Test failed - arm64
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2016

@bors-servo retry

  • etc/ci/manifest_changed.sh
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

⌛ Testing commit 9da00e2 with merge 66d508d...
bors-servo added a commit that referenced this pull request Jun 24, 2016
@bors-servo
Auto merge of #11695 - asajeffrey:script-thread-dont-panic, r=Ms2ger
66d508d 
Removed some sources of panic from script thread.

<!-- Please describe your changes on the following line: -->
**This PR needs some thought!**

It removes some sources of panic from script_thread.rs, caused by pipeline lookup failure. It's incomplete, as there are some uses of `get_browsing_context` elsewhere in the code.

---
<!-- 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 fix #11693 and #11685 (and probably some other intermittents)
- [X] These changes do not require tests because it is fixing intermittent panic

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

💔 Test failed - mac-rel-wpt
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

⌛ Testing commit 9da00e2 with merge 0427112...
bors-servo added a commit that referenced this pull request Jun 24, 2016
@bors-servo
Auto merge of #11695 - asajeffrey:script-thread-dont-panic, r=Ms2ger
Loading status checks…
0427112 
Removed some sources of panic from script thread.

<!-- Please describe your changes on the following line: -->
**This PR needs some thought!**

It removes some sources of panic from script_thread.rs, caused by pipeline lookup failure. It's incomplete, as there are some uses of `get_browsing_context` elsewhere in the code.

---
<!-- 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 fix #11693 and #11685 (and probably some other intermittents)
- [X] These changes do not require tests because it is fixing intermittent panic

<!-- 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="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

💔 Test failed - android
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2016

@bors-servo retry

(Finally got rout to submitting an issue for that one.)
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

⚡ Previous build results for arm64, linux-dev, mac-dev-unit, windows are reusable. Rebuilding only android, arm32, linux-rel, mac-rel-css, mac-rel-wpt...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

💔 Test failed - linux-rel
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 24, 2016

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel, mac-rel-css...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 24, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo bors-servo merged commit 9da00e2 into servo:master Jun 24, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@timvandermeij timvandermeij mentioned this pull request Jun 25, 2016
Closed
@aneeshusa aneeshusa mentioned this pull request Jun 26, 2016
Merged
4 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
A-content/script I-panic S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Intermittent CRASH in /html/semantics/embedded-content/the-iframe-element/change_parentage.html
4 participants
@asajeffrey @highfive @bors-servo @Ms2ger
You can’t perform that action at this time.