Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2249133002:
Changes to how FindRequestManager reports find results. (Closed)
DescriptionChanges to how FindRequestManager reports find results.
I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why.
The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJDOzz_XZKItjF6oM/edit?usp=sharing
UPDATE: Anther find-in-page CL I landed recently (https://codereview.chromium.org/2186113002/) was reverted because of a test that began timing out. I believe that failure is caused by the problems fixed here, so that bugfix is now blocked on this CL.
Committed: https://crrev.com/bbaacbe524515e8eafcd04f6dc6d872d403ffed6
Cr-Commit-Position: refs/heads/master@{#415353}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Addressed comments. #Patch Set 3 : Expanded comment for FinalUpdate(), and renamed to FinalUpdateReceived(). #
Messages
Total messages: 35 (26 generated)
The CQ bit was checked by paulmeyer@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.
Patchset #1 (id:1) has been deleted
Description was changed from ========== WIP for changes to how FindRequestManager reports find results. ========== to ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest, I noticed that the tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... ==========
Description was changed from ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest, I noticed that the tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... ========== to ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... ==========
Description was changed from ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... ========== to ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... ==========
The CQ bit was checked by paulmeyer@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...
paulmeyer@chromium.org changed reviewers: + lfg@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by paulmeyer@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.
Description was changed from ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... ========== to ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... UPDATE: Anther find-in-page CL I landed recently (https://codereview.chromium.org/2186113002/) was reverted because of a test that began timing out. I believe that failure is caused by the problems fixed here, so that bugfix is now blocked on this CL. ==========
Added some comments. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:188: NotifyFindReply(request_id, false /* final_update */); I noticed we can send NotifyFindReply with an request_id that's different than the current request id. This is different from the old behavior, and from the perspective of the WebContents it seems strange to be receiving a reply from an find operation that's not the current one anymore. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:195: if (request_id == current_session_id_ && !pending_initial_replies_.empty()) Why did you remove the NotifyFindReply() from this reply? It seems that the final update from any frame should still be reported, even though it's not the final reply. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:202: DCHECK(pending_find_next_reply_ == rfh); nit: DCHECK_EQ. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:210: // issued find requests have completed. See my comment above, I don't think we should ever return a find ID that's different than the current one. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:260: NotifyFindReply(current_session_id_, final_update); I don't understand why we avoid calling FinalUpdate() here when final_update is true. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:268: FinalUpdate(current_session_id_, rfh); Do we want to send a FinalUpdate if pending_find_next_reply_ != null? https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:275: FinalUpdate(current_request_.id, rfh); Same as above, do we want to send a FinalUpdate if pending_initial_replies_.empty() == false? https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:501: return rfh && matches_per_frame_.count(rfh); I don't understand the reason for this change. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:533: pending_initial_replies_.empty() /* final_update */); The fact that FinalUpdate() doesn't always send a final update is confusing. (which means some of my comments above are wrong).
Patchset #2 (id:40001) has been deleted
PTAL https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:188: NotifyFindReply(request_id, false /* final_update */); On 2016/08/18 18:50:42, lfg wrote: > I noticed we can send NotifyFindReply with an request_id that's different than > the current request id. This is different from the old behavior, and from the > perspective of the WebContents it seems strange to be receiving a reply from an > find operation that's not the current one anymore. That sounds reasonable. I don't think there are any problems with sending the IDs out of order when the replies actually come out of order, but I agree that its a bit odd from the perspective of the receiver of the replies. I'll add a field to keep track of the last reported ID, and make sure that future replies have >= that ID. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:195: if (request_id == current_session_id_ && !pending_initial_replies_.empty()) On 2016/08/18 18:50:42, lfg wrote: > Why did you remove the NotifyFindReply() from this reply? It seems that the > final update from any frame should still be reported, even though it's not the > final reply. I can't remember why I did that. It may have been accidental. I agree that it should report here. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:202: DCHECK(pending_find_next_reply_ == rfh); On 2016/08/18 18:50:42, lfg wrote: > nit: DCHECK_EQ. Done. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:210: // issued find requests have completed. On 2016/08/18 18:50:42, lfg wrote: > See my comment above, I don't think we should ever return a find ID that's > different than the current one. Done. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:260: NotifyFindReply(current_session_id_, final_update); On 2016/08/18 18:50:42, lfg wrote: > I don't understand why we avoid calling FinalUpdate() here when final_update is > true. See comment on FinalUpdate(). In this case, a frame is being removed that there was no expected reply from, so this removal doesn't interact with an outstanding find request and so can't be the final update of any request. That being said, if ALL requests are already finished, then we still want to mark this reply as final since no more are coming. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:268: FinalUpdate(current_session_id_, rfh); On 2016/08/18 18:50:42, lfg wrote: > Do we want to send a FinalUpdate if pending_find_next_reply_ != null? See comment on FinalUpdate(). https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:275: FinalUpdate(current_request_.id, rfh); On 2016/08/18 18:50:42, lfg wrote: > Same as above, do we want to send a FinalUpdate if > pending_initial_replies_.empty() == false? See comment on FinalUpdate(). https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:501: return rfh && matches_per_frame_.count(rfh); On 2016/08/18 18:50:42, lfg wrote: > I don't understand the reason for this change. I discovered that I don't actually want to check IsRenderFrameLive() n all cases that CheckFrame() is called, so I took it out of here, and just put it next to CheckFrame() in the cases where it actually matters. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:533: pending_initial_replies_.empty() /* final_update */); On 2016/08/18 18:50:42, lfg wrote: > The fact that FinalUpdate() doesn't always send a final update is confusing. > (which means some of my comments above are wrong). FinalUpdate() is called once all the replies have been received for a single find request, which used to almost always result in the find reply from FindRequestManager being marked final. Now I have purposely changed that because of the case when one or more find next requests finish before the initial find request completes (in all frames). In that case, the reply outputted from FindRequestManager should not be marked final.
The CQ bit was checked by paulmeyer@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.
Just one more question and 2 nits. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:260: NotifyFindReply(current_session_id_, final_update); On 2016/08/29 22:27:41, paulmeyer wrote: > On 2016/08/18 18:50:42, lfg wrote: > > I don't understand why we avoid calling FinalUpdate() here when final_update > is > > true. > > See comment on FinalUpdate(). In this case, a frame is being removed that there > was no expected reply from, so this removal doesn't interact with an outstanding > find request and so can't be the final update of any request. That being said, > if ALL requests are already finished, then we still want to mark this reply as > final since no more are coming. Is it possible that the final update will be sent twice in this case? For example, all the frames report back, a final update is sent, then a frame is removed and another final update is sent with the same request id? https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:501: return rfh && matches_per_frame_.count(rfh); On 2016/08/29 22:27:41, paulmeyer wrote: > On 2016/08/18 18:50:42, lfg wrote: > > I don't understand the reason for this change. > > I discovered that I don't actually want to check IsRenderFrameLive() n all cases > that CheckFrame() is called, so I took it out of here, and just put it next to > CheckFrame() in the cases where it actually matters. Can you add add a comment in the places that this wasn't wanted explaining why it's not necessary? https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:533: pending_initial_replies_.empty() /* final_update */); On 2016/08/29 22:27:41, paulmeyer wrote: > On 2016/08/18 18:50:42, lfg wrote: > > The fact that FinalUpdate() doesn't always send a final update is confusing. > > (which means some of my comments above are wrong). > > FinalUpdate() is called once all the replies have been received for a single > find request, which used to almost always result in the find reply from > FindRequestManager being marked final. Now I have purposely changed that because > of the case when one or more find next requests finish before the initial find > request completes (in all frames). In that case, the reply outputted from > FindRequestManager should not be marked final. Can you update the comment on the header to reflect this change?
PTAL. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:260: NotifyFindReply(current_session_id_, final_update); On 2016/08/30 14:57:38, lfg wrote: > On 2016/08/29 22:27:41, paulmeyer wrote: > > On 2016/08/18 18:50:42, lfg wrote: > > > I don't understand why we avoid calling FinalUpdate() here when final_update > > is > > > true. > > > > See comment on FinalUpdate(). In this case, a frame is being removed that > there > > was no expected reply from, so this removal doesn't interact with an > outstanding > > find request and so can't be the final update of any request. That being said, > > if ALL requests are already finished, then we still want to mark this reply as > > final since no more are coming. > > Is it possible that the final update will be sent twice in this case? For > example, all the frames report back, a final update is sent, then a frame is > removed and another final update is sent with the same request id? Yes, that is possible, and intended. Once all frames report back, the last update from FindRequestManager should be final, since it does not intend to send another one (it can't predict or expect that a frame will ever be removed). If a frame is then removed, FindRequestManager needs to send another update, and again it doesn't expect to send another, so that update is final again. This is all fine from the perspective of the WebContentsDelegate. In fact, it used to get even more final updates before this change. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:501: return rfh && matches_per_frame_.count(rfh); On 2016/08/30 14:57:38, lfg wrote: > On 2016/08/29 22:27:41, paulmeyer wrote: > > On 2016/08/18 18:50:42, lfg wrote: > > > I don't understand the reason for this change. > > > > I discovered that I don't actually want to check IsRenderFrameLive() n all > cases > > that CheckFrame() is called, so I took it out of here, and just put it next to > > CheckFrame() in the cases where it actually matters. > > Can you add add a comment in the places that this wasn't wanted explaining why > it's not necessary? I don't think that is necessary. If you look at the comment for CheckFrame() in the header, it actually only claims to check whether |rfh| is one of the frames searched in the current find session, which is sufficiently done by just checking if it is in |matches_per_frame_|. It doesn't make any guarantee that the frame is still live. When that extra property is needed, it is checked explicitly. https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:533: pending_initial_replies_.empty() /* final_update */); On 2016/08/30 14:57:38, lfg wrote: > On 2016/08/29 22:27:41, paulmeyer wrote: > > On 2016/08/18 18:50:42, lfg wrote: > > > The fact that FinalUpdate() doesn't always send a final update is confusing. > > > (which means some of my comments above are wrong). > > > > FinalUpdate() is called once all the replies have been received for a single > > find request, which used to almost always result in the find reply from > > FindRequestManager being marked final. Now I have purposely changed that > because > > of the case when one or more find next requests finish before the initial find > > request completes (in all frames). In that case, the reply outputted from > > FindRequestManager should not be marked final. > > Can you update the comment on the header to reflect this change? Done. I also renamed FinalUpdate() to FinalUpdateReceived(), which I think more clearly indicates that it is called when a final update is received for a particular find request, not necessarily when a final update is going to be sent by FindRequestManager.
lgtm https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2249133002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:501: return rfh && matches_per_frame_.count(rfh); On 2016/08/30 15:40:12, paulmeyer wrote: > On 2016/08/30 14:57:38, lfg wrote: > > On 2016/08/29 22:27:41, paulmeyer wrote: > > > On 2016/08/18 18:50:42, lfg wrote: > > > > I don't understand the reason for this change. > > > > > > I discovered that I don't actually want to check IsRenderFrameLive() n all > > cases > > > that CheckFrame() is called, so I took it out of here, and just put it next > to > > > CheckFrame() in the cases where it actually matters. > > > > Can you add add a comment in the places that this wasn't wanted explaining why > > it's not necessary? > > I don't think that is necessary. If you look at the comment for CheckFrame() in > the header, it actually only claims to check whether |rfh| is one of the frames > searched in the current find session, which is sufficiently done by just > checking if it is in |matches_per_frame_|. It doesn't make any guarantee that > the frame is still live. When that extra property is needed, it is checked > explicitly. Acknowledged.
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...
Message was sent while issue was closed.
Description was changed from ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... UPDATE: Anther find-in-page CL I landed recently (https://codereview.chromium.org/2186113002/) was reverted because of a test that began timing out. I believe that failure is caused by the problems fixed here, so that bugfix is now blocked on this CL. ========== to ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... UPDATE: Anther find-in-page CL I landed recently (https://codereview.chromium.org/2186113002/) was reverted because of a test that began timing out. I believe that failure is caused by the problems fixed here, so that bugfix is now blocked on this CL. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... UPDATE: Anther find-in-page CL I landed recently (https://codereview.chromium.org/2186113002/) was reverted because of a test that began timing out. I believe that failure is caused by the problems fixed here, so that bugfix is now blocked on this CL. ========== to ========== Changes to how FindRequestManager reports find results. I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why. The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJ... UPDATE: Anther find-in-page CL I landed recently (https://codereview.chromium.org/2186113002/) was reverted because of a test that began timing out. I believe that failure is caused by the problems fixed here, so that bugfix is now blocked on this CL. Committed: https://crrev.com/bbaacbe524515e8eafcd04f6dc6d872d403ffed6 Cr-Commit-Position: refs/heads/master@{#415353} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bbaacbe524515e8eafcd04f6dc6d872d403ffed6 Cr-Commit-Position: refs/heads/master@{#415353} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
