Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2044683002:
Prevent crash when resize is sent to a swapped out main frame (Closed)
|
Created:
4 years, 6 months ago by kenrb Modified:
4 years, 6 months ago Reviewers:
nasko CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent crash when resize is sent to a swapped out main frame
On Mac, it is possible for the UI code to trigger a resize on a main
frame that is in the process of being navigated and has already been
swapped out. This patch prevents a renderer crash in that scenario.
BUG=612606
Committed: https://crrev.com/5fd60fb8a0d4fd287ac6d407eacf40c7c64a565f
Cr-Commit-Position: refs/heads/master@{#399216}
Patch Set 1 #
Messages
Total messages: 12 (3 generated)
kenrb@chromium.org changed reviewers: + nasko@chromium.org
Nasko: PTAL? I am starting with you for review. Feel free to punt if you think somebody else is more qualified to comment here. Patchset 1 is the most naive possible fix, which may or may not be right. I am interested in thoughts on whether there is a reason why it should be considered an invariant that ViewMsg_Resize should not be received by a RenderView after the main frame has been swapped out. This happens on Mac as a navigation is committing, the bookmark controller triggers a resize on the old frame. Perhaps this should be considered a bug in the Mac UI code but I am not clear on what that would be.
On 2016/06/06 21:06:56, kenrb wrote: > Nasko: PTAL? > > I am starting with you for review. Feel free to punt if you think somebody else > is more qualified to comment here. > > Patchset 1 is the most naive possible fix, which may or may not be right. I am > interested in thoughts on whether there is a reason why it should be considered > an invariant that ViewMsg_Resize should not be received by a RenderView after > the main frame has been swapped out. The problem with ViewMsg_Resize is that it is overloaded with more than one piece of data. Some of it (device orientation for example) make sense to be page-level state and some of it is frame-level state. If we say that this invariant should be upheld, then we need to figure out how to update the page-level state. In the past, I've heard that the IPC is not split into more specific ones due to performance/jank concerns. > This happens on Mac as a navigation is > committing, the bookmark controller triggers a resize on the old frame. Perhaps > this should be considered a bug in the Mac UI code but I am not clear on what > that would be. I would also like to understand why we are sending a resize IPC for navigation without the browser changing its size between the two pages. Can we get someone with more Mac experience to help narrow this down and/or provide an explanation?
On 2016/06/06 23:56:45, nasko (slow) wrote: > On 2016/06/06 21:06:56, kenrb wrote: > > Nasko: PTAL? > > > > I am starting with you for review. Feel free to punt if you think somebody > else > > is more qualified to comment here. > > > > Patchset 1 is the most naive possible fix, which may or may not be right. I am > > interested in thoughts on whether there is a reason why it should be > considered > > an invariant that ViewMsg_Resize should not be received by a RenderView after > > the main frame has been swapped out. > > The problem with ViewMsg_Resize is that it is overloaded with more than one > piece of data. Some of it (device orientation for example) make sense to be > page-level state and some of it is frame-level state. If we say that this > invariant should be upheld, then we need to figure out how to update the > page-level state. In the past, I've heard that the IPC is not split into more > specific ones due to performance/jank concerns. That makes it sound like this CL is correct, I think. If we eventually need to be able to send a not-yet-created PageMsg_Resize to a RenderViewImpl, then it needs to not assume that the main frame is local. I agree that this needs to be dealt with, 'resize this frame' is different from 'resize the page', and when RenderWidget is disassociated with RenderView that will have to be made to work. Does this need a new bug or would we consider it implicit in the RenderView/RenderWidget split? > > > This happens on Mac as a navigation is > > committing, the bookmark controller triggers a resize on the old frame. > Perhaps > > this should be considered a bug in the Mac UI code but I am not clear on what > > that would be. > > I would also like to understand why we are sending a resize IPC for navigation > without the browser changing its size between the two pages. Can we get someone > with more Mac experience to help narrow this down and/or provide an explanation? It's because there is special Mac UI code to manage the bookmark bar. It affects the page size because it takes up a part of the top of the screen, and sends resizes when it updates. It gets updated in response to the navigation committing. I don't know if there is a convenient way to make an exception to the updates if we are navigating away from a page, but if the renderer-side patch is reasonable then is that necessary?
On 2016/06/07 16:08:09, kenrb wrote: > On 2016/06/06 23:56:45, nasko (slow) wrote: > > On 2016/06/06 21:06:56, kenrb wrote: > > > Nasko: PTAL? > > > > > > I am starting with you for review. Feel free to punt if you think somebody > > else > > > is more qualified to comment here. > > > > > > Patchset 1 is the most naive possible fix, which may or may not be right. I > am > > > interested in thoughts on whether there is a reason why it should be > > considered > > > an invariant that ViewMsg_Resize should not be received by a RenderView > after > > > the main frame has been swapped out. > > > > The problem with ViewMsg_Resize is that it is overloaded with more than one > > piece of data. Some of it (device orientation for example) make sense to be > > page-level state and some of it is frame-level state. If we say that this > > invariant should be upheld, then we need to figure out how to update the > > page-level state. In the past, I've heard that the IPC is not split into more > > specific ones due to performance/jank concerns. > > That makes it sound like this CL is correct, I think. If we eventually need to > be able to send a not-yet-created PageMsg_Resize to a RenderViewImpl, then it > needs to not assume that the main frame is local. I agree that this needs to be > dealt with, 'resize this frame' is different from 'resize the page', and when > RenderWidget is disassociated with RenderView that will have to be made to work. > Does this need a new bug or would we consider it implicit in the > RenderView/RenderWidget split? As discussed, this CL makes sense on its own, so LGTM. > > > > > This happens on Mac as a navigation is > > > committing, the bookmark controller triggers a resize on the old frame. > > Perhaps > > > this should be considered a bug in the Mac UI code but I am not clear on > what > > > that would be. > > > > I would also like to understand why we are sending a resize IPC for navigation > > without the browser changing its size between the two pages. Can we get > someone > > with more Mac experience to help narrow this down and/or provide an > explanation? > > It's because there is special Mac UI code to manage the bookmark bar. It affects > the page size because it takes up a part of the top of the screen, and sends > resizes when it updates. It gets updated in response to the navigation > committing. I don't know if there is a convenient way to make an exception to > the updates if we are navigating away from a page, but if the renderer-side > patch is reasonable then is that necessary? I think there is also a Mac specific bug somewhere, since it shouldn't be trying to resize the old RenderViewHost, but only the new one. Maybe avi@ can lend a hand here and see if there is something obvious in the Cocoa code.
Filed https://bugs.chromium.org/p/chromium/issues/detail?id=619063 for the UI code sending this message to be dealt with, and https://bugs.chromium.org/p/chromium/issues/detail?id=618353 for refactoring resize.
The CQ bit was checked by kenrb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2044683002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Prevent crash when resize is sent to a swapped out main frame On Mac, it is possible for the UI code to trigger a resize on a main frame that is in the process of being navigated and has already been swapped out. This patch prevents a renderer crash in that scenario. BUG=612606 ========== to ========== Prevent crash when resize is sent to a swapped out main frame On Mac, it is possible for the UI code to trigger a resize on a main frame that is in the process of being navigated and has already been swapped out. This patch prevents a renderer crash in that scenario. BUG=612606 Committed: https://crrev.com/5fd60fb8a0d4fd287ac6d407eacf40c7c64a565f Cr-Commit-Position: refs/heads/master@{#399216} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/5fd60fb8a0d4fd287ac6d407eacf40c7c64a565f Cr-Commit-Position: refs/heads/master@{#399216} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
