Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377453002:
Deflake RaceCrossOriginNavigationAndSamePageHistoryNavigation test. (Closed)
DescriptionDeflake RaceCrossOriginNavigationAndSamePageHistoryNavigation test.
This test was dropping the commit IPC message, which casued some flaky
runs. The fix for that is to not drop the message, but to still process
it and ensure it issues a GoBack navigation command before it is processed.
This CL also introduces a new observer object that can track only a
commit of a specific URL in a frame.
BUG=645022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/29b2a5a88cf2c558d67e2befaeb5d2bb9ecdfd36
Cr-Commit-Position: refs/heads/master@{#421044}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixes based on Charlie's review. #Patch Set 3 : Add a HasCommitted check. #
Total comments: 6
Messages
Total messages: 19 (7 generated)
Description was changed from ========== Deflake RaceCrossOriginNavigationAndSamePageHistoryNavigation test. This test was dropping the commit IPC message, which casued some flaky runs. The fix for that is to not drop the message, but to still process it and ensure it issues a GoBack navigation command before it is processed. This CL also introduces a new observer object that can track only a commit of a specific URL in a frame. BUG=645022 ========== to ========== Deflake RaceCrossOriginNavigationAndSamePageHistoryNavigation test. This test was dropping the commit IPC message, which casued some flaky runs. The fix for that is to not drop the message, but to still process it and ensure it issues a GoBack navigation command before it is processed. This CL also introduces a new observer object that can track only a commit of a specific URL in a frame. BUG=645022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, Can you review this CL for me? It should fix the flakiness in the browser test for the origin/URL mismatch race condition. Thanks in advance! Nasko
Thanks for fixing this! Looks good, with one question about a possible hang below, plus nits. https://codereview.chromium.org/2377453002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2377453002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5974: class CommitMessageFilter : public BrowserMessageFilter { nit: Let's rename this now that it isn't general (e.g., GoBackAndCommitFilter?). https://codereview.chromium.org/2377453002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6062: UrlCommitObserver history_commit_observer(root, start_url); Should we be declaring this before the LoadURL call? I worry that it might hang if the second commit occurs before we start listening (e.g., during the previous Wait). https://codereview.chromium.org/2377453002/diff/1/content/test/content_browse... File content/test/content_browser_test_utils_internal.cc (right): https://codereview.chromium.org/2377453002/diff/1/content/test/content_browse... content/test/content_browser_test_utils_internal.cc:392: if (navigation_handle->GetURL() == url_ && Do we care about checking HasCommitted(), to exclude canceled navigations?
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
https://codereview.chromium.org/2377453002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2377453002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5974: class CommitMessageFilter : public BrowserMessageFilter { On 2016/09/26 20:58:38, Charlie Reis (slow) wrote: > nit: Let's rename this now that it isn't general (e.g., GoBackAndCommitFilter?). Done. https://codereview.chromium.org/2377453002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6062: UrlCommitObserver history_commit_observer(root, start_url); On 2016/09/26 20:58:38, Charlie Reis (slow) wrote: > Should we be declaring this before the LoadURL call? I worry that it might hang > if the second commit occurs before we start listening (e.g., during the previous > Wait). Done. https://codereview.chromium.org/2377453002/diff/1/content/test/content_browse... File content/test/content_browser_test_utils_internal.cc (right): https://codereview.chromium.org/2377453002/diff/1/content/test/content_browse... content/test/content_browser_test_utils_internal.cc:392: if (navigation_handle->GetURL() == url_ && On 2016/09/26 20:58:38, Charlie Reis (slow) wrote: > Do we care about checking HasCommitted(), to exclude canceled navigations? And also error pages - yeah, I meant to do it and it slipped my mind. Added.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! LGTM.
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Deflake RaceCrossOriginNavigationAndSamePageHistoryNavigation test. This test was dropping the commit IPC message, which casued some flaky runs. The fix for that is to not drop the message, but to still process it and ensure it issues a GoBack navigation command before it is processed. This CL also introduces a new observer object that can track only a commit of a specific URL in a frame. BUG=645022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Deflake RaceCrossOriginNavigationAndSamePageHistoryNavigation test. This test was dropping the commit IPC message, which casued some flaky runs. The fix for that is to not drop the message, but to still process it and ensure it issues a GoBack navigation command before it is processed. This CL also introduces a new observer object that can track only a commit of a specific URL in a frame. BUG=645022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/29b2a5a88cf2c558d67e2befaeb5d2bb9ecdfd36 Cr-Commit-Position: refs/heads/master@{#421044} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/29b2a5a88cf2c558d67e2befaeb5d2bb9ecdfd36 Cr-Commit-Position: refs/heads/master@{#421044}
Message was sent while issue was closed.
avi@chromium.org changed reviewers: + avi@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5971: // A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionaLoad IPC ProvisionalLoad https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5991: rfh->OnMessageReceived(message); Does it matter that you moved the IPC message to a different thread? (It came in on the IO thread, and you're re-dispatching it on the UI thread.)
Message was sent while issue was closed.
https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5971: // A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionaLoad IPC On 2016/09/27 15:19:18, Avi wrote: > ProvisionalLoad Oops. Do you think it is worth a CL to fix? https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5991: rfh->OnMessageReceived(message); On 2016/09/27 15:19:18, Avi wrote: > Does it matter that you moved the IPC message to a different thread? (It came in > on the IO thread, and you're re-dispatching it on the UI thread.) I have to move it, as this method is only called for the FrameHostMsg_DidCommitProvisionalLoad IPC, which is handled on the UI thread. Since the OnMessageReceived of this object returns true, it tells the rest of the IPC system that it is handled, so now it is under the control of this object, which must do the work to dispatch it on the UI thread.
Message was sent while issue was closed.
https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5971: // A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionaLoad IPC On 2016/09/27 15:25:21, nasko (slow) wrote: > On 2016/09/27 15:19:18, Avi wrote: > > ProvisionalLoad > > Oops. Do you think it is worth a CL to fix? Nah. https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5991: rfh->OnMessageReceived(message); On 2016/09/27 15:25:21, nasko (slow) wrote: > I have to move it, as this method is only called for the > FrameHostMsg_DidCommitProvisionalLoad IPC, which is handled on the UI thread. How is DidCommit handled on the UI thread? I'm missing a piece of the puzzle here. Either a BrowserMessageFilter says to handle it on the UI thread, in which case we're already on the correct thread in OnMessageReceived, below, so why the PostTask, or OnMessageReceived is being called on the IO thread, in which case the PostTask is necessary, but then how does that work in the general case where we're not inserting a filter?
Message was sent while issue was closed.
> https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... > content/browser/frame_host/navigation_controller_impl_browsertest.cc:5991: > rfh->OnMessageReceived(message); > On 2016/09/27 15:25:21, nasko (slow) wrote: > > I have to move it, as this method is only called for the > > FrameHostMsg_DidCommitProvisionalLoad IPC, which is handled on the UI thread. > > How is DidCommit handled on the UI thread? I'm missing a piece of the puzzle > here. > > Either a BrowserMessageFilter says to handle it on the UI thread, in which case > we're already on the correct thread in OnMessageReceived, below, so why the > PostTask, or OnMessageReceived is being called on the IO thread, in which case > the PostTask is necessary, but then how does that work in the general case where > we're not inserting a filter? When an IPC arrives on the IO thread, it is delivered to all BrowserMessageFilters registered. By definition, BrowserMessageFilters run on the IO thread. If none of the filter handles the message, it is delivered to the UI thread to RenderProcessHostImpl::OnMessageReceived for further dispatching to the right object, based on routing id.
Message was sent while issue was closed.
On 2016/09/27 16:05:38, nasko (slow) wrote: > > > https://codereview.chromium.org/2377453002/diff/40001/content/browser/frame_h... > > content/browser/frame_host/navigation_controller_impl_browsertest.cc:5991: > > rfh->OnMessageReceived(message); > > On 2016/09/27 15:25:21, nasko (slow) wrote: > > > I have to move it, as this method is only called for the > > > FrameHostMsg_DidCommitProvisionalLoad IPC, which is handled on the UI > thread. > > > > How is DidCommit handled on the UI thread? I'm missing a piece of the puzzle > > here. > > > > Either a BrowserMessageFilter says to handle it on the UI thread, in which > case > > we're already on the correct thread in OnMessageReceived, below, so why the > > PostTask, or OnMessageReceived is being called on the IO thread, in which case > > the PostTask is necessary, but then how does that work in the general case > where > > we're not inserting a filter? > > When an IPC arrives on the IO thread, it is delivered to all > BrowserMessageFilters registered. By definition, BrowserMessageFilters run on > the IO thread. If none of the filter handles the message, it is delivered to the > UI thread to RenderProcessHostImpl::OnMessageReceived for further dispatching to > the right object, based on routing id. I did not know that. Thank you. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
