Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581143002:
Maintain HostZoom connection per-frame on browser side (Closed)
DescriptionMaintain HostZoom connection per-frame on browser side
HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo
connection is per-frame. Before this CL, HostZoomMapObserver was
maintaining one HostZoom connection and rebinding it every time a new
RenderFrame was created. This meant that HostZoomMapObserver was
continually losing connections to existing frames. This CL changes
HostZoomMapObserver to maintain one connection per-RenderFrame in a
map indexed by the corresponding RenderFrameHost.
BUG=673065
TEST=Visit news.ycombinator.com and increase the zoom level to 175%.
Click the top link. Hit back: news.ycombinator.com should still be
zoomed to 175%.
Patch Set 1 #Patch Set 2 : Handle unittest #
Total comments: 4
Patch Set 3 : Rebase #Patch Set 4 : Response to reviews #Patch Set 5 : Rebase #
Messages
Total messages: 39 (25 generated)
The CQ bit was checked by blundell@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by blundell@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
blundell@chromium.org changed reviewers: + scottmg@chromium.org, wjmaclean@chromium.org
The Android telemetry failures look unrelated.
I'm ok with this generally, though it needs a non-manual test. Presumably examples in site_isolation_browsertest and/or iframe_zoom_browsertest can provide a framework to help create a test? https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... File content/browser/host_zoom_map_observer.cc (right): https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... content/browser/host_zoom_map_observer.cc:56: // key here. Presumably this is on account of a rfh being deleted ... is there any possibility of a new rhf with the same pointer being given the wrong host_zoom_ptr in the meantime, on account of a 'stale' entry?
nick@chromium.org changed reviewers: + nick@chromium.org
https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... File content/browser/host_zoom_map_observer.cc (right): https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... content/browser/host_zoom_map_observer.cc:56: // key here. On 2016/12/16 14:45:35, wjmaclean wrote: > Presumably this is on account of a rfh being deleted ... is there any > possibility of a new rhf with the same pointer being given the wrong > host_zoom_ptr in the meantime, on account of a 'stale' entry? The scenario wjmaclean describes seems plausible. In general WCO::RenderFrameCreated overrides ought to be balanced by a WCO::RenderFrameDeleted override. rfh pointer references ought not be kept past RenderFrameDeleted.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by blundell@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by blundell@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Test added. https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... File content/browser/host_zoom_map_observer.cc (right): https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... content/browser/host_zoom_map_observer.cc:56: // key here. On 2016/12/16 18:21:12, ncarter wrote: > On 2016/12/16 14:45:35, wjmaclean wrote: > > Presumably this is on account of a rfh being deleted ... is there any > > possibility of a new rhf with the same pointer being given the wrong > > host_zoom_ptr in the meantime, on account of a 'stale' entry? > > The scenario wjmaclean describes seems plausible. > > In general WCO::RenderFrameCreated overrides ought to be balanced by a > WCO::RenderFrameDeleted override. rfh pointer references ought not be kept past > RenderFrameDeleted. Changed. For my own curiosity: the case where this would occur would be if a WCO::ReadyToCommitNavigation() came in for a given RenderFrame before WCO::RenderFrameCreated() came in for that RenderFrame. Is that possible? Understood that in any case this is the right change to make.
https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... File content/browser/host_zoom_map_observer.cc (right): https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... content/browser/host_zoom_map_observer.cc:56: // key here. On 2016/12/19 17:03:14, blundell wrote: > On 2016/12/16 18:21:12, ncarter wrote: > > On 2016/12/16 14:45:35, wjmaclean wrote: > > > Presumably this is on account of a rfh being deleted ... is there any > > > possibility of a new rhf with the same pointer being given the wrong > > > host_zoom_ptr in the meantime, on account of a 'stale' entry? > > > > The scenario wjmaclean describes seems plausible. > > > > In general WCO::RenderFrameCreated overrides ought to be balanced by a > > WCO::RenderFrameDeleted override. rfh pointer references ought not be kept > past > > RenderFrameDeleted. > > Changed. For my own curiosity: the case where this would occur would be if a > WCO::ReadyToCommitNavigation() came in for a given RenderFrame before > WCO::RenderFrameCreated() came in for that RenderFrame. Is that possible? > Understood that in any case this is the right change to make. I would hope we don't do ReadyToCommitNavigation on non-live RFH's. That's not exactly what I was worried about. It's more a general skepticism of patterns like "It is not safe to do anything with |pointer| other than use it as a key" because of the possibility of the memory being reallocated to a new object of the same type as the deleted object. The worry is more that RenderFrameDeleted (and RFH deletion / ~RenderFrameHostImpl) can occur basically at any point in time. One particularly abrupt example is: if the RFH is an oopif, and the process of the main frame crashes, we would delete the oopif RFH immediately since the document containing them has gone away. This event is triggered by something completely outside the RFH's operation, so it could really happen at any moment. In this case, if HostZoomMapObserver does not handle RenderFrameDeleted, I worry about whether OnConnectionError would run after the memory for the RFH has been freed, or else RenderFrameCreated happening on a new RFH that happened to be reallocated to the same pointer address, overwriting old the map entry. It's possible that there's some mojo behavior I don't know about, that would cause OnConnectionError to always (and only ever) happen before (or during) ~RenderFrameHostImpl. If that's the case then I'd be interested to learn about it.
On 2016/12/19 19:21:16, ncarter wrote: > https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... > File content/browser/host_zoom_map_observer.cc (right): > > https://codereview.chromium.org/2581143002/diff/20001/content/browser/host_zo... > content/browser/host_zoom_map_observer.cc:56: // key here. > On 2016/12/19 17:03:14, blundell wrote: > > On 2016/12/16 18:21:12, ncarter wrote: > > > On 2016/12/16 14:45:35, wjmaclean wrote: > > > > Presumably this is on account of a rfh being deleted ... is there any > > > > possibility of a new rhf with the same pointer being given the wrong > > > > host_zoom_ptr in the meantime, on account of a 'stale' entry? > > > > > > The scenario wjmaclean describes seems plausible. > > > > > > In general WCO::RenderFrameCreated overrides ought to be balanced by a > > > WCO::RenderFrameDeleted override. rfh pointer references ought not be kept > > past > > > RenderFrameDeleted. > > > > Changed. For my own curiosity: the case where this would occur would be if a > > WCO::ReadyToCommitNavigation() came in for a given RenderFrame before > > WCO::RenderFrameCreated() came in for that RenderFrame. Is that possible? > > Understood that in any case this is the right change to make. > > I would hope we don't do ReadyToCommitNavigation on non-live RFH's. That's not > exactly what I was worried about. It's more a general skepticism of patterns > like "It is not safe to do anything with |pointer| other than use it as a key" > because of the possibility of the memory being reallocated to a new object of > the same type as the deleted object. > > The worry is more that RenderFrameDeleted (and RFH deletion / > ~RenderFrameHostImpl) can occur basically at any point in time. One particularly > abrupt example is: if the RFH is an oopif, and the process of the main frame > crashes, we would delete the oopif RFH immediately since the document containing > them has gone away. This event is triggered by something completely outside the > RFH's operation, so it could really happen at any moment. > > In this case, if HostZoomMapObserver does not handle RenderFrameDeleted, I worry > about whether OnConnectionError would run after the memory for the RFH has been > freed, or else RenderFrameCreated happening on a new RFH that happened to be > reallocated to the same pointer address, overwriting old the map entry. > > It's possible that there's some mojo behavior I don't know about, that would > cause OnConnectionError to always (and only ever) happen before (or during) > ~RenderFrameHostImpl. If that's the case then I'd be interested to learn about > it. That makes sense, thanks! In general there would not be any guarantee that OnConnectionError() would be called before ~RenderFrameHostImpl (associated interfaces make this possible, but even in that case, you'd have to reason about it pretty carefully). It seems plausible that RenderFrameCreated() could be called for a new RFH before receiving OnConnectionError() for the old one.
lgtm
wjmaclean@, I'll wait for your review as well. Thanks!
On 2016/12/20 06:51:39, blundell wrote: > wjmaclean@, I'll wait for your review as well. Thanks! lgtm, thanks for the revisions and the test!
The CQ bit was checked by blundell@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": 100001, "attempt_start_ts": 1482244549459310,
"parent_rev": "99b5987d10d49eb02b8f7d242c25cf7a5b2cf2e4", "commit_rev":
"a230553653d6a18f4c9151a3456ba105a9a050a7"}
Message was sent while issue was closed.
Description was changed from ========== Maintain HostZoom connection per-frame on browser side HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo connection is per-frame. Before this CL, HostZoomMapObserver was maintaining one HostZoom connection and rebinding it every time a new RenderFrame was created. This meant that HostZoomMapObserver was continually losing connections to existing frames. This CL changes HostZoomMapObserver to maintain one connection per-RenderFrame in a map indexed by the corresponding RenderFrameHost. BUG=673065 TEST=Visit news.ycombinator.com and increase the zoom level to 175%. Click the top link. Hit back: news.ycombinator.com should still be zoomed to 175%. ========== to ========== Maintain HostZoom connection per-frame on browser side HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo connection is per-frame. Before this CL, HostZoomMapObserver was maintaining one HostZoom connection and rebinding it every time a new RenderFrame was created. This meant that HostZoomMapObserver was continually losing connections to existing frames. This CL changes HostZoomMapObserver to maintain one connection per-RenderFrame in a map indexed by the corresponding RenderFrameHost. BUG=673065 TEST=Visit news.ycombinator.com and increase the zoom level to 175%. Click the top link. Hit back: news.ycombinator.com should still be zoomed to 175%. Review-Url: https://codereview.chromium.org/2581143002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Maintain HostZoom connection per-frame on browser side HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo connection is per-frame. Before this CL, HostZoomMapObserver was maintaining one HostZoom connection and rebinding it every time a new RenderFrame was created. This meant that HostZoomMapObserver was continually losing connections to existing frames. This CL changes HostZoomMapObserver to maintain one connection per-RenderFrame in a map indexed by the corresponding RenderFrameHost. BUG=673065 TEST=Visit news.ycombinator.com and increase the zoom level to 175%. Click the top link. Hit back: news.ycombinator.com should still be zoomed to 175%. Review-Url: https://codereview.chromium.org/2581143002 ========== to ========== Maintain HostZoom connection per-frame on browser side HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo connection is per-frame. Before this CL, HostZoomMapObserver was maintaining one HostZoom connection and rebinding it every time a new RenderFrame was created. This meant that HostZoomMapObserver was continually losing connections to existing frames. This CL changes HostZoomMapObserver to maintain one connection per-RenderFrame in a map indexed by the corresponding RenderFrameHost. BUG=673065 TEST=Visit news.ycombinator.com and increase the zoom level to 175%. Click the top link. Hit back: news.ycombinator.com should still be zoomed to 175%. Committed: https://crrev.com/8e24b7d133424fb221891601d6254b7df58188d5 Cr-Commit-Position: refs/heads/master@{#439800} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8e24b7d133424fb221891601d6254b7df58188d5 Cr-Commit-Position: refs/heads/master@{#439800}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:100001) has been created in https://codereview.chromium.org/2595603002/ by blundell@chromium.org. The reason for reverting is: Seems to cause problems on Mac: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit....
Message was sent while issue was closed.
On 2016/12/20 15:53:48, blundell wrote: > A revert of this CL (patchset #4 id:100001) has been created in > https://codereview.chromium.org/2595603002/ by mailto:blundell@chromium.org. > > The reason for reverting is: Seems to cause problems on Mac: > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit.... Also showing up on WebKit Linux Trusty: (https://build.chromium.org/p/chromium.webkit/waterfall?builder=WebKit%20Linux...) fast/css/preserve-user-specified-zoom-level-on-reload.html
Message was sent while issue was closed.
Description was changed from ========== Maintain HostZoom connection per-frame on browser side HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo connection is per-frame. Before this CL, HostZoomMapObserver was maintaining one HostZoom connection and rebinding it every time a new RenderFrame was created. This meant that HostZoomMapObserver was continually losing connections to existing frames. This CL changes HostZoomMapObserver to maintain one connection per-RenderFrame in a map indexed by the corresponding RenderFrameHost. BUG=673065 TEST=Visit news.ycombinator.com and increase the zoom level to 175%. Click the top link. Hit back: news.ycombinator.com should still be zoomed to 175%. Committed: https://crrev.com/8e24b7d133424fb221891601d6254b7df58188d5 Cr-Commit-Position: refs/heads/master@{#439800} ========== to ========== Maintain HostZoom connection per-frame on browser side HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo connection is per-frame. Before this CL, HostZoomMapObserver was maintaining one HostZoom connection and rebinding it every time a new RenderFrame was created. This meant that HostZoomMapObserver was continually losing connections to existing frames. This CL changes HostZoomMapObserver to maintain one connection per-RenderFrame in a map indexed by the corresponding RenderFrameHost. BUG=673065 TEST=Visit news.ycombinator.com and increase the zoom level to 175%. Click the top link. Hit back: news.ycombinator.com should still be zoomed to 175%. ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
