Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857953004:
Fix for flaky WebViewTest.Shim_TestFindAPI_findupdate test on msan bots. (Closed)
DescriptionFix for flaky WebViewTest.Shim_TestFindAPI_findupdate test on msan bots.
Part of what this test tests is canceling an in-progress find session by
searching for another search term. The problem that was causing the
flakiness was that the find session that was meant to get canceled
actually finished (rarely) before the subsequent request made it to the
renderer, and so the completed session was not reported as canceled. The
small adjustment made in this CL is to only attempt to cancel the search
that has many results, so that its find session is much less likely to
complete before the next find request comes through.
BUG=710486
Review-Url: https://codereview.chromium.org/2857953004
Cr-Commit-Position: refs/heads/master@{#469390}
Committed: https://chromium.googlesource.com/chromium/src/+/f5bea1f45cd62b26b49e4b8cd1ee1ad096351717
Patch Set 1 #Patch Set 2 : Added TODO. #Messages
Total messages: 24 (13 generated)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
paulmeyer@chromium.org changed reviewers: + wjmaclean@chromium.org
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.
On 2017/05/03 20:39:07, paulmeyer wrote: Can you explain, either by comments in the issue description, or main.js, or even both, why your change makes the test stop being flaky?
Description was changed from ========== Fix for flaky WebViewTest.Shim_TestFindAPI_findupdate test on msan bots. BUG=710486 ========== to ========== Fix for flaky WebViewTest.Shim_TestFindAPI_findupdate test on msan bots. Part of what this test tests is canceling an in-progress find session by searching for another search term. The problem that was causing the flakiness was that the find session that was meant to get canceled actually finished (rarely) before the subsequent request made it to the renderer, and so the completed session was not reported as canceled. The small adjustment made in this CL is to only attempt to cancel the search that has many results, so that its find session is much less likely to complete before the next find request comes through. BUG=710486 ==========
On 2017/05/04 13:10:49, wjmaclean wrote: > On 2017/05/03 20:39:07, paulmeyer wrote: > > Can you explain, either by comments in the issue description, or main.js, or > even both, why your change makes the test stop being flaky? Okay, I've added to the description. I don't think any comments in the code are actually warranted for this change.
On 2017/05/04 13:50:05, paulmeyer wrote:
> On 2017/05/04 13:10:49, wjmaclean wrote:
> > On 2017/05/03 20:39:07, paulmeyer wrote:
> >
> > Can you explain, either by comments in the issue description, or main.js, or
> > even both, why your change makes the test stop being flaky?
>
> Okay, I've added to the description. I don't think any comments in the code
are
> actually warranted for this change.
Thanks for the update.
Question: it sounds like, from the description, that the test could still flake,
is that right? I.e. can the 'large' search ('dog') ever complete before the next
search request comes along?
Nit: please wrap the lines in the issue description to 70 chars?
Description was changed from ========== Fix for flaky WebViewTest.Shim_TestFindAPI_findupdate test on msan bots. Part of what this test tests is canceling an in-progress find session by searching for another search term. The problem that was causing the flakiness was that the find session that was meant to get canceled actually finished (rarely) before the subsequent request made it to the renderer, and so the completed session was not reported as canceled. The small adjustment made in this CL is to only attempt to cancel the search that has many results, so that its find session is much less likely to complete before the next find request comes through. BUG=710486 ========== to ========== Fix for flaky WebViewTest.Shim_TestFindAPI_findupdate test on msan bots. Part of what this test tests is canceling an in-progress find session by searching for another search term. The problem that was causing the flakiness was that the find session that was meant to get canceled actually finished (rarely) before the subsequent request made it to the renderer, and so the completed session was not reported as canceled. The small adjustment made in this CL is to only attempt to cancel the search that has many results, so that its find session is much less likely to complete before the next find request comes through. BUG=710486 ==========
On 2017/05/04 13:56:17, wjmaclean wrote:
> On 2017/05/04 13:50:05, paulmeyer wrote:
> > On 2017/05/04 13:10:49, wjmaclean wrote:
> > > On 2017/05/03 20:39:07, paulmeyer wrote:
> > >
> > > Can you explain, either by comments in the issue description, or main.js,
or
> > > even both, why your change makes the test stop being flaky?
> >
> > Okay, I've added to the description. I don't think any comments in the code
> are
> > actually warranted for this change.
>
> Thanks for the update.
>
> Question: it sounds like, from the description, that the test could still
flake,
> is that right? I.e. can the 'large' search ('dog') ever complete before the
next
> search request comes along?
>
> Nit: please wrap the lines in the issue description to 70 chars?
I would say that it is technically still possible, though the number of terms is
10x compared to before, so I would say that it is very unlikely. There is not
currently any mechanism in the renderer to force this race, and I don't think
adding it would be warranted just for this test. I think we should try this fix
for now, and then see if it ever flakes again.
On 2017/05/04 14:11:14, paulmeyer wrote:
> On 2017/05/04 13:56:17, wjmaclean wrote:
> > On 2017/05/04 13:50:05, paulmeyer wrote:
> > > On 2017/05/04 13:10:49, wjmaclean wrote:
> > > > On 2017/05/03 20:39:07, paulmeyer wrote:
> > > >
> > > > Can you explain, either by comments in the issue description, or
main.js,
> or
> > > > even both, why your change makes the test stop being flaky?
> > >
> > > Okay, I've added to the description. I don't think any comments in the
code
> > are
> > > actually warranted for this change.
> >
> > Thanks for the update.
> >
> > Question: it sounds like, from the description, that the test could still
> flake,
> > is that right? I.e. can the 'large' search ('dog') ever complete before the
> next
> > search request comes along?
> >
> > Nit: please wrap the lines in the issue description to 70 chars?
>
> I would say that it is technically still possible, though the number of terms
is
> 10x compared to before, so I would say that it is very unlikely. There is not
> currently any mechanism in the renderer to force this race, and I don't think
> adding it would be warranted just for this test. I think we should try this
fix
> for now, and then see if it ever flakes again.
I'm ok with landing this as a speculative fix, but it would be nice if we can
include some output if/when the test fails warning of this possibility, so that
whoever triages the test failure has some idea what to do with it.
lgtm
On 2017/05/04 14:54:17, wjmaclean wrote:
> On 2017/05/04 14:11:14, paulmeyer wrote:
> > On 2017/05/04 13:56:17, wjmaclean wrote:
> > > On 2017/05/04 13:50:05, paulmeyer wrote:
> > > > On 2017/05/04 13:10:49, wjmaclean wrote:
> > > > > On 2017/05/03 20:39:07, paulmeyer wrote:
> > > > >
> > > > > Can you explain, either by comments in the issue description, or
> main.js,
> > or
> > > > > even both, why your change makes the test stop being flaky?
> > > >
> > > > Okay, I've added to the description. I don't think any comments in the
> code
> > > are
> > > > actually warranted for this change.
> > >
> > > Thanks for the update.
> > >
> > > Question: it sounds like, from the description, that the test could still
> > flake,
> > > is that right? I.e. can the 'large' search ('dog') ever complete before
the
> > next
> > > search request comes along?
> > >
> > > Nit: please wrap the lines in the issue description to 70 chars?
> >
> > I would say that it is technically still possible, though the number of
terms
> is
> > 10x compared to before, so I would say that it is very unlikely. There is
not
> > currently any mechanism in the renderer to force this race, and I don't
think
> > adding it would be warranted just for this test. I think we should try this
> fix
> > for now, and then see if it ever flakes again.
>
> I'm ok with landing this as a speculative fix, but it would be nice if we can
> include some output if/when the test fails warning of this possibility, so
that
> whoever triages the test failure has some idea what to do with it.
>
> lgtm
Okay, I've added a note to the bug.
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wjmaclean@chromium.org Link to the patchset: https://codereview.chromium.org/2857953004/#ps20001 (title: "Added TODO.")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by paulmeyer@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": 20001, "attempt_start_ts": 1493918973792950,
"parent_rev": "5b970b537e755c4421b9a412dfe20df0a9c6daad", "commit_rev":
"f5bea1f45cd62b26b49e4b8cd1ee1ad096351717"}
Message was sent while issue was closed.
Description was changed from ========== Fix for flaky WebViewTest.Shim_TestFindAPI_findupdate test on msan bots. Part of what this test tests is canceling an in-progress find session by searching for another search term. The problem that was causing the flakiness was that the find session that was meant to get canceled actually finished (rarely) before the subsequent request made it to the renderer, and so the completed session was not reported as canceled. The small adjustment made in this CL is to only attempt to cancel the search that has many results, so that its find session is much less likely to complete before the next find request comes through. BUG=710486 ========== to ========== Fix for flaky WebViewTest.Shim_TestFindAPI_findupdate test on msan bots. Part of what this test tests is canceling an in-progress find session by searching for another search term. The problem that was causing the flakiness was that the find session that was meant to get canceled actually finished (rarely) before the subsequent request made it to the renderer, and so the completed session was not reported as canceled. The small adjustment made in this CL is to only attempt to cancel the search that has many results, so that its find session is much less likely to complete before the next find request comes through. BUG=710486 Review-Url: https://codereview.chromium.org/2857953004 Cr-Commit-Position: refs/heads/master@{#469390} Committed: https://chromium.googlesource.com/chromium/src/+/f5bea1f45cd62b26b49e4b8cd1ee... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f5bea1f45cd62b26b49e4b8cd1ee... |
