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

Prevent crash trying to freeze script task with no page. #7675

Merged
merged 1 commit into from Sep 24, 2015

Conversation

@jgraham
Copy link
Contributor

jgraham commented Sep 18, 2015

This fixes a crash resulting from a race between loading an initial
document and navigating to a subsequent document. If the navigation
happens before the initial document has had a chance to create its
root page, we crash trying to unwrap a None. Note that the are likely
further similar timing issues with more complex sequences of navigation
and history manipulation.

Review on Reviewable

@jdm
Copy link
Member

jdm commented Sep 18, 2015

Please file an issue about figuring out a proper solution for this and link to it in a comment.

@jdm
Copy link
Member

jdm commented Sep 23, 2015

Still waiting on the comment addition for this to merge :)

@jgraham
Copy link
Contributor Author

jgraham commented Sep 23, 2015

Oh, right I thought you meant a GitHub comment. Your way makes more sense though.

This fixes a crash resulting from a race between loading an initial
document and navigating to a subsequent document. If the navigation
happens before the initial document has had a chance to create its
root page, we crash trying to unwrap a None. Note that the are likely
further similar timing issues with more complex sequences of navigation
and history manipulation.
@jgraham jgraham force-pushed the jgraham:freeze_crash branch from ffd282b to f8cd0d9 Sep 24, 2015
@metajack
Copy link
Contributor

metajack commented Sep 24, 2015

@bors-servo r+


Reviewed 1 of 1 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2015

📌 Commit f8cd0d9 has been approved by metajack

@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2015

Testing commit f8cd0d9 with merge d2d6d0a...

bors-servo pushed a commit that referenced this pull request Sep 24, 2015
Prevent crash trying to freeze script task with no page.

This fixes a crash resulting from a race between loading an initial
document and navigating to a subsequent document. If the navigation
happens before the initial document has had a chance to create its
root page, we crash trying to unwrap a None. Note that the are likely
further similar timing issues with more complex sequences of navigation
and history manipulation.

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

bors-servo commented Sep 24, 2015

💔 Test failed - mac-rel-wpt

@metajack
Copy link
Contributor

metajack commented Sep 24, 2015

@bors-servo retry

  • #7422 (not the same test, but seems the same error)
@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2015

Testing commit f8cd0d9 with merge 30ffd09...

bors-servo pushed a commit that referenced this pull request Sep 24, 2015
Prevent crash trying to freeze script task with no page.

This fixes a crash resulting from a race between loading an initial
document and navigating to a subsequent document. If the navigation
happens before the initial document has had a chance to create its
root page, we crash trying to unwrap a None. Note that the are likely
further similar timing issues with more complex sequences of navigation
and history manipulation.

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

bors-servo commented Sep 24, 2015

💔 Test failed - mac-rel-wpt

@metajack
Copy link
Contributor

metajack commented Sep 24, 2015

@bors-servo retry

  • #7422 (this time it was the exact test in the bug)
@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2015

Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Sep 24, 2015

@bors-servo bors-servo merged commit f8cd0d9 into servo:master Sep 24, 2015
2 checks passed
2 checks passed
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.