Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647963002:
Fix ScrollingCoordinator-related crash with page popups (Closed)
DescriptionFix ScrollingCoordinator-related crash with page popups
Rev 444497 introduced a crash that can happen when popups are opened,
because in some cases they access the wrong compositor animation host
and timeline. This fixes the issue by ensure they always use those
resources on the top frame.
BUG=683282
Review-Url: https://codereview.chromium.org/2647963002
Cr-Commit-Position: refs/heads/master@{#445192}
Committed: https://chromium.googlesource.com/chromium/src/+/f15f669d84991252dd186c7093b885d7e909fad3
Patch Set 1 #
Messages
Total messages: 17 (8 generated)
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kenrb@chromium.org changed reviewers: + bokan@chromium.org
bokan@: WDYT? This makes it so that popups always use the host and timeline on the root frame. This would be fine with no OOPIFs, although OOPIF popups can now possibly be contending with each other for this resources. Trying this locally for a few minutes I can't see any problems, and if the only downside is that some popups might not scroll if a user tries to open multiple from different OOPIFs, then it seems like a low impact bug that can wait for the ScrollCoordinator refactor.
Actually, scratch that, I found a separate crash stack that is unrelated to popups, so there is more than one bug here.
On 2017/01/20 19:40:54, kenrb wrote: > bokan@: WDYT? This makes it so that popups always use the host and timeline on > the root frame. This would be fine with no OOPIFs, although OOPIF popups can now > possibly be contending with each other for this resources. Trying this locally > for a few minutes I can't see any problems, and if the only downside is that > some popups might not scroll if a user tries to open multiple from different > OOPIFs, then it seems like a low impact bug that can wait for the > ScrollCoordinator refactor. Hmm, do you know why it's crashing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/20 19:48:07, bokan wrote: > On 2017/01/20 19:40:54, kenrb wrote: > > bokan@: WDYT? This makes it so that popups always use the host and timeline on > > the root frame. This would be fine with no OOPIFs, although OOPIF popups can > now > > possibly be contending with each other for this resources. Trying this locally > > for a few minutes I can't see any problems, and if the only downside is that > > some popups might not scroll if a user tries to open multiple from different > > OOPIFs, then it seems like a low impact bug that can wait for the > > ScrollCoordinator refactor. > > Hmm, do you know why it's crashing? For the one case, yes. We initialize the layer tree view on the popup's Page, which is different from the main Page. I didn't initially realize that. Looking at this further, nullptr might be entirely correct to pass here. However, I think there might be a separate crash related to my CL yesterday, which I am still working on debugging.
On 2017/01/20 21:54:27, kenrb wrote: > On 2017/01/20 19:48:07, bokan wrote: > > On 2017/01/20 19:40:54, kenrb wrote: > > > bokan@: WDYT? This makes it so that popups always use the host and timeline > on > > > the root frame. This would be fine with no OOPIFs, although OOPIF popups can > > now > > > possibly be contending with each other for this resources. Trying this > locally > > > for a few minutes I can't see any problems, and if the only downside is that > > > some popups might not scroll if a user tries to open multiple from different > > > OOPIFs, then it seems like a low impact bug that can wait for the > > > ScrollCoordinator refactor. > > > > Hmm, do you know why it's crashing? > > For the one case, yes. We initialize the layer tree view on the popup's Page, > which is different from the main Page. I didn't initially realize that. Looking > at this further, nullptr might be entirely correct to pass here. > > However, I think there might be a separate crash related to my CL yesterday, > which I am still working on debugging. Okay, the other crash I was looking at is not due to my change, I finally managed to reproduce it with my CL reverted locally. Are you okay with this CL?
lgtm
lgtm
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1484951903677530, "parent_rev":
"d9007bd3709012ee07fb298bf3881da3e34fcfdf", "commit_rev":
"f15f669d84991252dd186c7093b885d7e909fad3"}
Message was sent while issue was closed.
Description was changed from ========== Fix ScrollingCoordinator-related crash with page popups Rev 444497 introduced a crash that can happen when popups are opened, because in some cases they access the wrong compositor animation host and timeline. This fixes the issue by ensure they always use those resources on the top frame. BUG=683282 ========== to ========== Fix ScrollingCoordinator-related crash with page popups Rev 444497 introduced a crash that can happen when popups are opened, because in some cases they access the wrong compositor animation host and timeline. This fixes the issue by ensure they always use those resources on the top frame. BUG=683282 Review-Url: https://codereview.chromium.org/2647963002 Cr-Commit-Position: refs/heads/master@{#445192} Committed: https://chromium.googlesource.com/chromium/src/+/f15f669d84991252dd186c7093b8... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/f15f669d84991252dd186c7093b8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
