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

Layout queries on elements in disconnected frames panics #23053

Closed
jdm opened this issue Mar 18, 2019 · 12 comments
Closed

Layout queries on elements in disconnected frames panics #23053

jdm opened this issue Mar 18, 2019 · 12 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Mar 18, 2019

<iframe></iframe>
<script>
  var i = document.querySelector('iframe');
  var d = i.contentDocument;
  var e = d.createElement('div');
  d.body.appendChild(e);
  i.remove();
  setTimeout(function() { e.getBoundingClientRect(); }, 10);
</script>

When we remove an iframe, we shut down its browsing context and layout thread. Layout queries assume that the layout thread always exists, however.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 27, 2019

Previously found in #19170.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 27, 2019

One way of fixing this would be to merge the script and layout threads.

@jdm
Copy link
Member Author

@jdm jdm commented Apr 10, 2019

I think we should fix this by removing this expect call and returning false if the send operation fails instead.

@jdm
Copy link
Member Author

@jdm jdm commented Apr 10, 2019

Then we can add an automated test based on the code I provided earlier. We will want to assert that the returned rectangle is zero-sized.

@soniasingla
Copy link
Contributor

@soniasingla soniasingla commented Apr 10, 2019

@highfive : assign me

@highfive highfive added the C-assigned label Apr 10, 2019
@highfive
Copy link

@highfive highfive commented Apr 10, 2019

Hey @soniasingla! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive
Copy link

@highfive highfive commented Jun 18, 2020

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in Matrix.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@gterzian
Copy link
Member

@gterzian gterzian commented Jun 18, 2020

I propose a slightly different solution to this, where at the top of force_reflow we'd add:

if !self.is_alive() {
            warn!("Reflow attempted on zombie window.");
            return false;
}

That way we narrow down the cases where we don't panic to iframes that have been removed from the document, instead of removing the panic entirely, which I think could simply hide other problems.

@jdm
Copy link
Member Author

@jdm jdm commented Jun 18, 2020

That sounds reasonable.

@gterzian
Copy link
Member

@gterzian gterzian commented Jun 21, 2020

Ok I think it would be even better to make:

pub fn layout_chan(&self) -> &Sender<Msg> {

return Option<&Sender<Msg>>,

where None would be returned if !self.is_alive()

and make every use of the layout chan go through this function, including inside window.rs.

@alarsyo
Copy link
Contributor

@alarsyo alarsyo commented Jul 2, 2020

@highfive: assign me

@highfive highfive added the C-assigned label Jul 2, 2020
@highfive
Copy link

@highfive highfive commented Jul 2, 2020

Hey @alarsyo! Thanks for your interest in working on this issue. It's now assigned to you!

@alarsyo alarsyo mentioned this issue Jul 3, 2020
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jul 3, 2020
…es-panic, r=<try>

Return Option for Window's layout channel

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #23053

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

This is my first contribution, I'm trying to figure things out!

This fix passes the test case shown in #23053, however I don't know what the behavior should be in `Document` and `ScriptThread` if `Window::is_alive()` is false : simply ignore it, don't do anything ? Or is this something that should not happen now that we return false in `Window::force_reflow()` ?

I'm not sure about the directory where the test case should go, any advice?
bors-servo added a commit that referenced this issue Jul 4, 2020
…es-panic, r=jdm

Return Option for Window's layout channel

<!-- Please describe your changes on the following line: -->

`Window::layout_chan()` now returns an `Option<Sender<Msg>>`, returning `None` if the window is dead.

FIX #26969
FIX #26429
FIX #21208
FIX #19092
FIX #22559
FIX #22584
FIX #22652

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #23053

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

This is my first contribution, I'm trying to figure things out!

This fix passes the test case shown in #23053, however I don't know what the behavior should be in `Document` and `ScriptThread` if `Window::is_alive()` is false : simply ignore it, don't do anything ? Or is this something that should not happen now that we return false in `Window::force_reflow()` ?

I'm not sure about the directory where the test case should go, any advice?
@bors-servo bors-servo closed this in 8916a42 Jul 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.