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 upBrowser.html causes a panic cascade in constellation #10017
Closed
Assignees
Labels
Comments
|
A debug build would be most useful here. |
|
I can confirm seeing similar panics. I'm rebuilding in debug mode at the moment. |
|
And of course now I can't recreate it :( @Manishearth can you reliably create the panic? Does it still occur with 4607ed2? |
|
Debug trace: https://manishearth.pastebin.mozilla.org/8863777 |
|
...that's not the same crash, though. |
|
Yeah, that's #10005. |
|
If you apply #10015 locally you should be able to see your original panic. |
|
@Manishearth on IRC gives the recipe for replicating the panic: "type some webpage, hit enter, click plus, start typing." |
|
Applying #10035 causes the panic not to happen at my end. Perhaps this was the underlying cause, and the constellation panic was just masking it? |
|
Now that #10035 has landed, we can reproduce this in debug mode in master. To reproduce, |
|
The IRC conversation with @jdm is at http://logs.glob.uno/?c=mozilla%23servo#c386447. Summarizing... To reproduce the problem, go to google.com and resize the window a few times. No browser.html needed, but it doesn't appear to happen on vanilla web pages (more on this later). A stack trace in debug mode: Our theory by us: a Our proposed solution: debug-log, but otherwise ignore, messages which arrive after their pipeline is closed, rather than panicking. We can't return an error because these are async messages. |
|
Makes sense to me - though I do wonder if something could be done with cancellation tokens or the like. Are there any other events that we know may still be triggered after iframe close? |
|
@samlh I think anything we try to do to stop this behaviour is radically going to change the constellation API (not that I'm against that, but I'd like to see this bug fixed pretty soon). The constellation API should be robust against events arriving in any order, i suspect anything else will result in subtle race conditions of the kind that are already causing problems with intermittents, |
|
Fair enough - my concern was things like timer events holding onto stuff after the tab was closed, but I think the existing code at least prevents the js stuff from being held onto, so I'm not fussed. Thanks for the reply! |
|
I created a commit which removes any panicking uses of Not so hooray is that under not-web-render, it causes a different panic: The problem is the for (pipeline_id, requests) in pipeline_requests {
let msg = ChromeToPaintMsg::Paint(requests, self.frame_tree_id);
if let Some(pipeline) = self.pipeline(pipeline_id) {
pipeline.chrome_to_paint_chan.send(msg).unwrap();
}
}An IRC conversation with @pcwalton followed: http://logs.glob.uno/?c=mozilla%23servo#c387328. It looks like the culprit here is a message being sent to a paint thread after the associated iframe has been removed, and so the paint thread has exited. I think we should declare this to be a different issue. I'll tidy up my commit and turn it into a PR tomorrow. |
|
Seeing that I can reproduce the same failure on master, we can definitely declare that a different issue. |
bors-servo
added a commit
that referenced
this issue
Mar 18, 2016
Removed panicking when frame or pipeline lookup fails. Removed the methods `pipeline(id)`, `pipeline_mut(id)`, `frame(id)` and `frame_mut(id)` from constellation, which panicked when the table lookup failed. The panics were causing race conditions, e.g. visiting google.com and resizing the page would cause a panic, most likely due to an iframe being added and removed, with the `DOMLoad` event arriving after the iframe had been removed, causing a panic. This patch fixes #10017 and #8769 (although in non-webrender builds there's now a different panic, see #10017 (comment)). There are a few `TODO` items in the initial commit, for cases where it's not completely obvious what to do in the case of failure. <!-- 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/10082) <!-- Reviewable:end -->
This was referenced Mar 21, 2016
bors-servo
added a commit
that referenced
this issue
Mar 30, 2016
Removed panicking when frame or pipeline lookup fails. Removed the methods `pipeline(id)`, `pipeline_mut(id)`, `frame(id)` and `frame_mut(id)` from constellation, which panicked when the table lookup failed. The panics were causing race conditions, e.g. visiting google.com and resizing the page would cause a panic, most likely due to an iframe being added and removed, with the `DOMLoad` event arriving after the iframe had been removed, causing a panic. This patch fixes #10017 and #8769 (although in non-webrender builds there's now a different panic, see #10017 (comment)). There are a few `TODO` items in the initial commit, for cases where it's not completely obvious what to do in the case of failure. <!-- 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/10082) <!-- Reviewable:end -->
bors-servo
added a commit
that referenced
this issue
Mar 31, 2016
Removed panicking when frame or pipeline lookup fails. Removed the methods `pipeline(id)`, `pipeline_mut(id)`, `frame(id)` and `frame_mut(id)` from constellation, which panicked when the table lookup failed. The panics were causing race conditions, e.g. visiting google.com and resizing the page would cause a panic, most likely due to an iframe being added and removed, with the `DOMLoad` event arriving after the iframe had been removed, causing a panic. This patch fixes #10017 and #8769 (although in non-webrender builds there's now a different panic, see #10017 (comment)). There are a few `TODO` items in the initial commit, for cases where it's not completely obvious what to do in the case of failure. <!-- 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/10082) <!-- Reviewable:end -->
bors-servo
added a commit
that referenced
this issue
Mar 31, 2016
Removed panicking when frame or pipeline lookup fails. Removed the methods `pipeline(id)`, `pipeline_mut(id)`, `frame(id)` and `frame_mut(id)` from constellation, which panicked when the table lookup failed. The panics were causing race conditions, e.g. visiting google.com and resizing the page would cause a panic, most likely due to an iframe being added and removed, with the `DOMLoad` event arriving after the iframe had been removed, causing a panic. This patch fixes #10017 and #8769 (although in non-webrender builds there's now a different panic, see #10017 (comment)). There are a few `TODO` items in the initial commit, for cases where it's not completely obvious what to do in the case of failure. <!-- 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/10082) <!-- Reviewable:end -->
bors-servo
added a commit
that referenced
this issue
Mar 31, 2016
Removed panicking when frame or pipeline lookup fails. Removed the methods `pipeline(id)`, `pipeline_mut(id)`, `frame(id)` and `frame_mut(id)` from constellation, which panicked when the table lookup failed. The panics were causing race conditions, e.g. visiting google.com and resizing the page would cause a panic, most likely due to an iframe being added and removed, with the `DOMLoad` event arriving after the iframe had been removed, causing a panic. This patch fixes #10017 and #8769 (although in non-webrender builds there's now a different panic, see #10017 (comment)). There are a few `TODO` items in the initial commit, for cases where it's not completely obvious what to do in the case of failure. <!-- 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/10082) <!-- Reviewable:end -->
|
This can't be verified until #10260 is fixed |
|
Stuff works now, I can browse bhtml fine |
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-comments-removed
that referenced
this issue
Oct 1, 2019
… fails (from asajeffrey:remove-constellation-panic); r=glennw Removed the methods `pipeline(id)`, `pipeline_mut(id)`, `frame(id)` and `frame_mut(id)` from constellation, which panicked when the table lookup failed. The panics were causing race conditions, e.g. visiting google.com and resizing the page would cause a panic, most likely due to an iframe being added and removed, with the `DOMLoad` event arriving after the iframe had been removed, causing a panic. This patch fixes #10017 and #8769 (although in non-webrender builds there's now a different panic, see servo/servo#10017 (comment)). There are a few `TODO` items in the initial commit, for cases where it's not completely obvious what to do in the case of failure. Source-Repo: https://github.com/servo/servo Source-Revision: 7518c4de9317af3a643fc35131e556104b8693fa UltraBlame original commit: e158381a4d5b911ea484cde77adb7b16b48e0450
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified-and-comments-removed
that referenced
this issue
Oct 1, 2019
… fails (from asajeffrey:remove-constellation-panic); r=glennw Removed the methods `pipeline(id)`, `pipeline_mut(id)`, `frame(id)` and `frame_mut(id)` from constellation, which panicked when the table lookup failed. The panics were causing race conditions, e.g. visiting google.com and resizing the page would cause a panic, most likely due to an iframe being added and removed, with the `DOMLoad` event arriving after the iframe had been removed, causing a panic. This patch fixes #10017 and #8769 (although in non-webrender builds there's now a different panic, see servo/servo#10017 (comment)). There are a few `TODO` items in the initial commit, for cases where it's not completely obvious what to do in the case of failure. Source-Repo: https://github.com/servo/servo Source-Revision: 7518c4de9317af3a643fc35131e556104b8693fa UltraBlame original commit: e158381a4d5b911ea484cde77adb7b16b48e0450
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified
that referenced
this issue
Oct 1, 2019
… fails (from asajeffrey:remove-constellation-panic); r=glennw Removed the methods `pipeline(id)`, `pipeline_mut(id)`, `frame(id)` and `frame_mut(id)` from constellation, which panicked when the table lookup failed. The panics were causing race conditions, e.g. visiting google.com and resizing the page would cause a panic, most likely due to an iframe being added and removed, with the `DOMLoad` event arriving after the iframe had been removed, causing a panic. This patch fixes #10017 and #8769 (although in non-webrender builds there's now a different panic, see servo/servo#10017 (comment)). There are a few `TODO` items in the initial commit, for cases where it's not completely obvious what to do in the case of failure. Source-Repo: https://github.com/servo/servo Source-Revision: 7518c4de9317af3a643fc35131e556104b8693fa UltraBlame original commit: e158381a4d5b911ea484cde77adb7b16b48e0450
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
On 075ce98 on Linux, running
./mach run -r -w -bgives me this long panicThe initial panic seems to be:
cc @mbrubeck