Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upClose the correct pipelines when evicted from navigation context. #966
Conversation
bors-servo
pushed a commit
that referenced
this pull request
Sep 23, 2013
Fixes #967 and #965 This has been wrong for a long time. Previously, only the pipeline associated with the root frame evicted would be shut down. 1) It shouldn't necessarily be closed, because there could be references to it still in the navigation context, and 2) Presumably none of the children pipelines of the root frame were ever exiting. It's hard to test this right now because #965 covers up other pipeline exiting issues, but when that's fixed, a pathological case in which things would have broken down would be: 1) Load a page with an iframe that contains a link 2) Click the link 3) Press backspace to navigate back 4) Navigate to any new page, at which point the forward page would be evicted from the navigation context, and the outer frame's pipeline would be shut down improperly. 5) Press backspace, at which point there is no longer a pipeline for the old page, because it was shut down prematurely. Presumably this would cause a crash. I also changed the FrameTree function ```find_mut``` to ```find``` because find_mut implies it's doing something to cause mutability, but the mutability is caused by the type of object being iterated over, nothing else. Additionally, script was exiting completely when receiving an exit message. Instead, it needs to handle exit messages according to who sent it. It should only close the subframes of the frame whose pipeline sent the exit message. This is now fixed. Inexplicably, script was also closing the compositor upon receiving an exit message. This doesn't seem like it'd ever be the right thing to do. I've fixed that. I don't think anyone shuts down the compositor now. It seems like it should occur in servo.rc, but I haven't added it because I'm not positive.
bors-servo
pushed a commit
that referenced
this pull request
Sep 23, 2013
Fixes #967 and #965 This has been wrong for a long time. Previously, only the pipeline associated with the root frame evicted would be shut down. 1) It shouldn't necessarily be closed, because there could be references to it still in the navigation context, and 2) Presumably none of the children pipelines of the root frame were ever exiting. It's hard to test this right now because #965 covers up other pipeline exiting issues, but when that's fixed, a pathological case in which things would have broken down would be: 1) Load a page with an iframe that contains a link 2) Click the link 3) Press backspace to navigate back 4) Navigate to any new page, at which point the forward page would be evicted from the navigation context, and the outer frame's pipeline would be shut down improperly. 5) Press backspace, at which point there is no longer a pipeline for the old page, because it was shut down prematurely. Presumably this would cause a crash. I also changed the FrameTree function ```find_mut``` to ```find``` because find_mut implies it's doing something to cause mutability, but the mutability is caused by the type of object being iterated over, nothing else. Additionally, script was exiting completely when receiving an exit message. Instead, it needs to handle exit messages according to who sent it. It should only close the subframes of the frame whose pipeline sent the exit message. This is now fixed. Inexplicably, script was also closing the compositor upon receiving an exit message. This doesn't seem like it'd ever be the right thing to do. I've fixed that. I don't think anyone shuts down the compositor now. It seems like it should occur in servo.rc, but I haven't added it because I'm not positive.
|
Closing to reset bors. |
bors-servo
pushed a commit
that referenced
this pull request
Sep 23, 2013
Fixes #967 and #965 This has been wrong for a long time. Previously, only the pipeline associated with the root frame evicted would be shut down. 1) It shouldn't necessarily be closed, because there could be references to it still in the navigation context, and 2) Presumably none of the children pipelines of the root frame were ever exiting. It's hard to test this right now because #965 covers up other pipeline exiting issues, but when that's fixed, a pathological case in which things would have broken down would be: 1) Load a page with an iframe that contains a link 2) Click the link 3) Press backspace to navigate back 4) Navigate to any new page, at which point the forward page would be evicted from the navigation context, and the outer frame's pipeline would be shut down improperly. 5) Press backspace, at which point there is no longer a pipeline for the old page, because it was shut down prematurely. Presumably this would cause a crash. I also changed the FrameTree function ```find_mut``` to ```find``` because find_mut implies it's doing something to cause mutability, but the mutability is caused by the type of object being iterated over, nothing else. Additionally, script was exiting completely when receiving an exit message. Instead, it needs to handle exit messages according to who sent it. It should only close the subframes of the frame whose pipeline sent the exit message. This is now fixed. Inexplicably, script was also closing the compositor upon receiving an exit message. This doesn't seem like it'd ever be the right thing to do. I've fixed that. I don't think anyone shuts down the compositor now. It seems like it should occur in servo.rc, but I haven't added it because I'm not positive.
|
This is fine to merge, but seems to be causing contenttest not to exit on OS X. It's possible it's a bug somewhere else, but it seems suspicious. |
|
Closing to unstick bors. Will reopen shortly. |
bors-servo
pushed a commit
that referenced
this pull request
Sep 24, 2013
Fixes #967 and #965 This has been wrong for a long time. Previously, only the pipeline associated with the root frame evicted would be shut down. 1) It shouldn't necessarily be closed, because there could be references to it still in the navigation context, and 2) Presumably none of the children pipelines of the root frame were ever exiting. It's hard to test this right now because #965 covers up other pipeline exiting issues, but when that's fixed, a pathological case in which things would have broken down would be: 1) Load a page with an iframe that contains a link 2) Click the link 3) Press backspace to navigate back 4) Navigate to any new page, at which point the forward page would be evicted from the navigation context, and the outer frame's pipeline would be shut down improperly. 5) Press backspace, at which point there is no longer a pipeline for the old page, because it was shut down prematurely. Presumably this would cause a crash. I also changed the FrameTree function ```find_mut``` to ```find``` because find_mut implies it's doing something to cause mutability, but the mutability is caused by the type of object being iterated over, nothing else. Additionally, script was exiting completely when receiving an exit message. Instead, it needs to handle exit messages according to who sent it. It should only close the subframes of the frame whose pipeline sent the exit message. This is now fixed. Inexplicably, script was also closing the compositor upon receiving an exit message. This doesn't seem like it'd ever be the right thing to do. I've fixed that. I don't think anyone shuts down the compositor now. It seems like it should occur in servo.rc, but I haven't added it because I'm not positive.
added 3 commits
Sep 21, 2013
|
window.close() works again. r? @metajack |
This comment has been minimized.
This comment has been minimized.
metajack
commented on 103cd62
Sep 24, 2013
|
r+ |
This comment has been minimized.
This comment has been minimized.
|
saw approval from metajack |
This comment has been minimized.
This comment has been minimized.
|
merging tikue/servo/master = 103cd62 into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
all tests pass: |
This comment has been minimized.
This comment has been minimized.
|
fast-forwarding master to auto = 401176b |
bors-servo
pushed a commit
that referenced
this pull request
Sep 24, 2013
Fixes #967 and #965 This has been wrong for a long time. Previously, only the pipeline associated with the root frame evicted would be shut down. 1) It shouldn't necessarily be closed, because there could be references to it still in the navigation context, and 2) Presumably none of the children pipelines of the root frame were ever exiting. It's hard to test this right now because #965 covers up other pipeline exiting issues, but when that's fixed, a pathological case in which things would have broken down would be: 1) Load a page with an iframe that contains a link 2) Click the link 3) Press backspace to navigate back 4) Navigate to any new page, at which point the forward page would be evicted from the navigation context, and the outer frame's pipeline would be shut down improperly. 5) Press backspace, at which point there is no longer a pipeline for the old page, because it was shut down prematurely. Presumably this would cause a crash. I also changed the FrameTree function ```find_mut``` to ```find``` because find_mut implies it's doing something to cause mutability, but the mutability is caused by the type of object being iterated over, nothing else. Additionally, script was exiting completely when receiving an exit message. Instead, it needs to handle exit messages according to who sent it. It should only close the subframes of the frame whose pipeline sent the exit message. This is now fixed. Inexplicably, script was also closing the compositor upon receiving an exit message. This doesn't seem like it'd ever be the right thing to do. *Edit: this is _only_ the right thing to do when received from the window.* I've fixed that. I don't think anyone shuts down the compositor now. *Edit: the script shuts down the compositor only when receiving an exit from the window.*
bors-servo
merged commit 103cd62
into
servo:master
1 check passed
1 check passed
default
all tests passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
tikue commentedSep 21, 2013
Fixes #967 and #965
This has been wrong for a long time. Previously, only the pipeline associated with the root frame evicted would be shut down. 1) It shouldn't necessarily be closed, because there could be references to it still in the navigation context, and 2) Presumably none of the children pipelines of the root frame were ever exiting.
It's hard to test this right now because #965 covers up other pipeline exiting issues, but when that's fixed, a pathological case in which things would have broken down would be:
I also changed the FrameTree function
find_muttofindbecause find_mut implies it's doing something to cause mutability, but the mutability is caused by the type of object being iterated over, nothing else.Additionally, script was exiting completely when receiving an exit message. Instead, it needs to handle exit messages according to who sent it. It should only close the subframes of the frame whose pipeline sent the exit message. This is now fixed.
Inexplicably, script was also closing the compositor upon receiving an exit message. This doesn't seem like it'd ever be the right thing to do. Edit: this is only the right thing to do when received from the window. I've fixed that. I don't think anyone shuts down the compositor now. Edit: the script shuts down the compositor only when receiving an exit from the window.