Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2569813002:
Re-enable SSLUITest.TestGoodFrameNavigation (Closed)
DescriptionRe-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky as of revision 2677ed350ab354042313bc3b47c371d7a3b67292
BUG=668360
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TBR=agl@chromium.org
Committed: https://crrev.com/8f935ed7f5ea2cb3095ecb8bb0ee4bd4972ee5db
Cr-Commit-Position: refs/heads/master@{#438169}
Patch Set 1 #Patch Set 2 : Check local_frame_id_ instead of SurfaceId #
Total comments: 3
Patch Set 3 : Check if surface_quad->surface_is is valid before recurssion in SurfaceHittest::GetTransformToTarge… #
Total comments: 1
Patch Set 4 : Re-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky #Messages
Total messages: 25 (14 generated)
Description was changed from ========== RenderWidgetHostViewChildFrame::TransformPointToRootCoordSpace() does noop if the SurfaceId is invalid BUG= ========== to ========== RenderWidgetHostViewChildFrame::TransformPointToRootCoordSpace() does noop if the SurfaceId is invalid BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== RenderWidgetHostViewChildFrame::TransformPointToRootCoordSpace() does noop if the SurfaceId is invalid BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== RenderWidgetHostViewChildFrame::TransformPointToRootCoordSpace() does noop if local_frame_id_ is invalid. Targetting when a CompositorFrame hasn't been received or the frame is evicted causes the serialization of invalid LocalFrameId. BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by staraz@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...
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2569813002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2569813002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:527: if (!local_frame_id_.is_valid()) Huh? We already do that above. This doesn't look like a fix.
https://codereview.chromium.org/2569813002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2569813002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:527: if (!local_frame_id_.is_valid()) On 2016/12/12 18:31:54, Fady Samuel wrote: > Huh? We already do that above. This doesn't look like a fix. I removed this check and added another one in SurfaceHittest::GetTransformToTargetSurfaceInternal(). I think that is the only unchecked SurfaceId being sent to GetSurfaceForId().
https://codereview.chromium.org/2569813002/diff/40001/cc/surfaces/surface_hit... File cc/surfaces/surface_hittest.cc (right): https://codereview.chromium.org/2569813002/diff/40001/cc/surfaces/surface_hit... cc/surfaces/surface_hittest.cc:213: if (!surface_quad->surface_id.is_valid()) How do we get into this state? This suggests that invalid SurfaceIds are being sent. This seems bad...
https://codereview.chromium.org/2569813002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2569813002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:527: if (!local_frame_id_.is_valid()) On 2016/12/12 18:31:54, Fady Samuel wrote: > Huh? We already do that above. This doesn't look like a fix. The !local_frame_id_.is_valid() is added (at revision 2677ed350ab354042313bc3b47c371d7a3b67292) after the test got disabled. It should fix the bug. I'm going to close this CL and mark the bug fixed.
Description was changed from ========== RenderWidgetHostViewChildFrame::TransformPointToRootCoordSpace() does noop if local_frame_id_ is invalid. Targetting when a CompositorFrame hasn't been received or the frame is evicted causes the serialization of invalid LocalFrameId. BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Re-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky as of revision 2677ed350ab354042313bc3b47c371d7a3b67292 BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
staraz@chromium.org changed reviewers: + agl@chromium.org
agl@chromium.org: Please review my changes
Description was changed from ========== Re-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky as of revision 2677ed350ab354042313bc3b47c371d7a3b67292 BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Re-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky as of revision 2677ed350ab354042313bc3b47c371d7a3b67292 BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=agl@chromium.org ==========
The CQ bit was checked by staraz@chromium.org
RS LGTM.
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
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 staraz@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": 60001, "attempt_start_ts": 1481640773085060,
"parent_rev": "15d8fe23c555f60f0ecafd78c8038b98b53442dc", "commit_rev":
"670a60beacdb9d2cee8a57069e95198ab7261b6c"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky as of revision 2677ed350ab354042313bc3b47c371d7a3b67292 BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=agl@chromium.org ========== to ========== Re-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky as of revision 2677ed350ab354042313bc3b47c371d7a3b67292 BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=agl@chromium.org Review-Url: https://codereview.chromium.org/2569813002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Re-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky as of revision 2677ed350ab354042313bc3b47c371d7a3b67292 BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=agl@chromium.org Review-Url: https://codereview.chromium.org/2569813002 ========== to ========== Re-enable SSLUITest.TestGoodFrameNavigation since it's no longer flaky as of revision 2677ed350ab354042313bc3b47c371d7a3b67292 BUG=668360 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TBR=agl@chromium.org Committed: https://crrev.com/8f935ed7f5ea2cb3095ecb8bb0ee4bd4972ee5db Cr-Commit-Position: refs/heads/master@{#438169} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8f935ed7f5ea2cb3095ecb8bb0ee4bd4972ee5db Cr-Commit-Position: refs/heads/master@{#438169} |
