Removed some sources of panic from script thread. by asajeffrey · Pull Request #11695 · servo/servo

asajeffrey · 2016-06-09T23:33:06Z

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 Intermittent CRASH in /html/semantics/embedded-content/the-iframe-element/change_parentage.html #11693 and Uses of panic! in script_thread #11685 (and probably some other intermittents)
These changes do not require tests because it is fixing intermittent panic

This change is

highfive · 2016-06-09T23:33:09Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/script_thread.rs

highfive · 2016-06-09T23:33:10Z

Warning

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

asajeffrey · 2016-06-09T23:33:36Z

cc @Ms2ger, @Manishearth and @mbrubeck

bors-servo · 2016-06-11T20:07:39Z

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

asajeffrey · 2016-06-12T16:38:29Z

Rebased.

Ms2ger · 2016-06-22T13:28:02Z

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 · 2016-06-23T17:58:05Z

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 · 2016-06-24T13:00:46Z

r+ after squash, I guess.

asajeffrey · 2016-06-24T13:20:22Z

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

bors-servo · 2016-06-24T13:20:23Z

📌 Commit 9da00e2 has been approved by Ms2ger

bors-servo · 2016-06-24T13:20:26Z

⌛ Testing commit 9da00e2 with merge 654104e...

Removed some sources of panic from script thread.  **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. ---  - [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   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)

bors-servo · 2016-06-24T13:39:11Z

💔 Test failed - linux-dev

asajeffrey · 2016-06-24T14:19:50Z

@bors-servo retry

etc/ci/manifest_changed.sh failing for some reason.

asajeffrey · 2016-06-24T14:33:28Z

@bors-servo retry

bors-servo · 2016-06-24T14:46:05Z

⚡ 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 · 2016-06-24T15:25:01Z

💔 Test failed - arm64

asajeffrey · 2016-06-24T19:50:12Z

@bors-servo retry

etc/ci/manifest_changed.sh

bors-servo · 2016-06-24T19:50:15Z

⌛ Testing commit 9da00e2 with merge 66d508d...

Removed some sources of panic from script thread.  **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. ---  - [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   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)

bors-servo · 2016-06-24T20:11:49Z

💔 Test failed - mac-rel-wpt

asajeffrey · 2016-06-24T20:18:59Z

@bors-servo retry

Intermittent failure of openssl #11858

bors-servo · 2016-06-24T20:19:02Z

⌛ Testing commit 9da00e2 with merge 0427112...

Removed some sources of panic from script thread.  **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. ---  - [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   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11695)

bors-servo · 2016-06-24T20:34:19Z

💔 Test failed - android

asajeffrey · 2016-06-24T20:52:55Z

@bors-servo retry

Intermittent failure of etc/ci/manifest_changed.sh #11859

(Finally got rout to submitting an issue for that one.)

bors-servo · 2016-06-24T20:52:58Z

⚡ 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 · 2016-06-24T21:09:52Z

💔 Test failed - linux-rel

asajeffrey · 2016-06-24T21:16:04Z

@bors-servo retry

Intermittent failure of openssl #11858

bors-servo · 2016-06-24T22:15:03Z

⚡ 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 · 2016-06-24T23:07:15Z

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

asajeffrey added A-content/script Related to the script thread I-panic Servo encounters a panic. labels Jun 9, 2016

highfive assigned Ms2ger Jun 9, 2016

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

This was referenced Jun 10, 2016

Handle pipeline lookup failure in webdriver #11712

Closed

Handle pipeline lookup failure in devtools #11714

Closed

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

asajeffrey force-pushed the script-thread-dont-panic branch from ae665c8 to 3c75ac9 Compare June 12, 2016 16:38

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

cbrewster mentioned this pull request Jun 19, 2016

Pipeline lookup in webdriver #11791

Merged

5 tasks

Removed some sources of panic from script thread.

9da00e2

asajeffrey force-pushed the script-thread-dont-panic branch from b54a37c to 9da00e2 Compare June 24, 2016 13:18

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 Jun 24, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 24, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Jun 24, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 24, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 24, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 24, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 24, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Jun 24, 2016

bors-servo merged commit 9da00e2 into servo:master Jun 24, 2016

timvandermeij mentioned this pull request Jun 25, 2016

Uses of panic! in script_thread #11685

Closed

aneeshusa mentioned this pull request Jun 26, 2016

Excise SubpageId and use only PipelineIds #11698

Merged

5 tasks

Uh oh!

Conversation

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

Uh oh!

Uh oh!

highfive commented Jun 9, 2016

Uh oh!

highfive commented Jun 9, 2016

Uh oh!

asajeffrey commented Jun 9, 2016

Uh oh!

bors-servo commented Jun 11, 2016

Uh oh!

asajeffrey commented Jun 12, 2016

Uh oh!

Ms2ger commented Jun 22, 2016

Uh oh!

asajeffrey commented Jun 23, 2016

Uh oh!

Ms2ger commented Jun 24, 2016

Uh oh!

asajeffrey commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

asajeffrey commented Jun 24, 2016

Uh oh!

asajeffrey commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

asajeffrey commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

asajeffrey commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

asajeffrey commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

asajeffrey commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

bors-servo commented Jun 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asajeffrey commented Jun 9, 2016 •

edited by larsbergstrom

Loading