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

Fixed panic in ActiveTimers.set_timeout_or_interval. #8175

Merged
merged 1 commit into from Nov 9, 2015

Conversation

@benschulz
Copy link
Contributor

benschulz commented Oct 24, 2015

ActiveTimers.set_timeout_or_interval asserts that the pipeline is not currently frozen. Apparently that is too strict. When pending network requests complete after a pipeline is frozen, scripts may be executed and a timer scheduled.

With these changes scheduling a timer while the pipeline is frozen behaves as if the timer was scheduled at the time the pipeline was frozen.

To reproduce the panic

  1. ./mach run -r http://google.com,
  2. immediately click on any link and
  3. wait for the panic.

Review on Reviewable

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 24, 2015

I'm not entirely sure we should execute script after the pipeline is frozen, but I don't really know anything about freezing.

@benschulz
Copy link
Contributor Author

benschulz commented Oct 24, 2015

I'm not entirely sure we should execute script after the pipeline is frozen

That was my intuitive understanding of freezing as well, hence the assertion. However, now that I've run into the panic — after merging, to my shame — I can't find anything to support that intuition. The documentation for the relevant message says "Notifies script task to suspend all its timers".

@jdm
Copy link
Member

jdm commented Oct 24, 2015

Our freezing implementation is extremely incomplete, unfortunately :(

@jdm jdm self-assigned this Nov 2, 2015
@jdm
Copy link
Member

jdm commented Nov 9, 2015

We clearly need to sort out what freezing means in practice, but obviously that's not happening right now. This change improves the status quo.
@bors-servo: r+

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2015

📌 Commit a3859c0 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2015

Testing commit a3859c0 with merge f74754f...

bors-servo added a commit that referenced this pull request Nov 9, 2015
Fixed panic in ActiveTimers.set_timeout_or_interval.

`ActiveTimers.set_timeout_or_interval` asserts that the pipeline is not currently frozen. Apparently that is too strict. When pending network requests complete after a pipeline is frozen, scripts may be executed and a timer scheduled.

With these changes scheduling a timer while the pipeline is frozen behaves as if the timer was scheduled at the time the pipeline was frozen.

To reproduce the panic
 1. `./mach run -r http://google.com`,
 2. immediately click on any link and
 3. wait for the panic.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8175)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2015

@bors-servo bors-servo merged commit a3859c0 into servo:master Nov 9, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Nov 18, 2015
Correct undisciplined rebase. (Closes #8583.)

I screwed up the rebase on top of #8175; sorry to waste your time with this, @jdm. :(

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8591)
<!-- Reviewable:end -->
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.