Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2571923002:
Fix threading issues in WebDataRequestManager. (Closed)
|
Created:
4 years ago by Roger McFarlane (Chromium) Modified:
4 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix threading issues in WebDataRequestManager.
This CL fixes a number of race conditions on request cancellation
and completion. In particular:
- Multiple cancellation calls on a request would DCHECK/crash.
- A request could be cancelled concurrent to completion,
resulting in a DCHECK/crash.
- There was an ineffective lock internal to each request. Most
of the critical code was already protected (though sometimes
not properly) by the managers internal lock and the request
lock wasn't adding any value.
BUG=672823
Committed: https://crrev.com/95e2ecf771003012c97168343b58947e3ae93ef5
Cr-Commit-Position: refs/heads/master@{#438863}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Address mathp's comments and check for cancellation before SetResult #Patch Set 3 : Simplify handling of result and add request factory func #
Total comments: 20
Patch Set 4 : address pkasting's comments #
Total comments: 2
Patch Set 5 : fix a comment #
Messages
Total messages: 43 (28 generated)
The CQ bit was checked by rogerm@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...
rogerm@chromium.org changed reviewers: + avi@chromium.org, mathp@chromium.org
Hey Avi & Mathieu, Care to take a look before I send this for OWNERs review? https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.cc (left): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:49: base::AutoLock l(cancel_lock_); this lock isn't useful, it's not protecting against anything, either way, the caller either sees the request as cancelled or not. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:54: base::AutoLock l(cancel_lock_); this lock isn't useful. This code is called under the manager's lock, so the lock is redundant. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:65: result_ = std::move(r); this isn't thread safe, could leak r. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:69: return std::move(result_); this isn't thread safe, you could end up with multiple unique_ptrs to result_ https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:102: DCHECK(i != pending_requests_.end()); This DCHECK isn't valid. Given concurrent cancellation/completion, both synchronized using pending_lock_, if the completion grabs the lock first, it will remove the handle from pending_requests and DCHECK on debug builds, crash in release. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:129: DCHECK(i != pending_requests_.end()); This DCHECK isn't valid. Given concurrent cancellation/completion, both synchronized using pending_lock_, if the cancellation grabs the lock first, it will remove the handle from pending_requests and DCHECK on debug builds, crash in release. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:104: return ++next_request_handle_; Maybe make this an atomic type? As is, we take pending_lock_ twice on request creation: once to get the handle and once to register the instance. Could also make the handle non-const and set it after creating the request in a factory func. Hmmm... https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:134: if (request->IsCancelled()) An optimization, to attempt to avoid contending for the lock if the request has already been cancelled/completed. The code should be equally correct without this check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Small comments https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.cc (left): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:129: DCHECK(i != pending_requests_.end()); On 2016/12/13 22:33:55, Roger McFarlane (Chromium) wrote: > This DCHECK isn't valid. > > Given concurrent cancellation/completion, both synchronized using pending_lock_, > if the cancellation grabs the lock first, it will remove the handle from > pending_requests and DCHECK on debug builds, crash in release. It's still in your change, should we remove it? https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:40: bool WebDataRequest::IsCancelled() const { IsInactive? Could be completed at this point too if I read correctly https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:45: // Of manager_ is null then the request has already been cancelled or If https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:46: // completed, and the asserting context represents and invalid state. *an https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:134: if (request->IsCancelled()) On 2016/12/13 22:33:55, Roger McFarlane (Chromium) wrote: > An optimization, to attempt to avoid contending for the lock if the request has > already been cancelled/completed. The code should be equally correct without > this check. This would be a worthwhile comment for a reader that may wonder why it's checked again a few lines below https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.h (right): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.h:86: friend class WebDataRequestManager; I've seen those statements be right under the private: line https://cs.chromium.org/search/?q=%22friend+class%22+file:components&sq=packa...
The CQ bit was checked by rogerm@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 checked by rogerm@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...
https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:124: request->SetResult(std::move(result)); If we no longer have the WebDatabaseBackend call SetResult on the request, but are doing it here, should we make SetResult private and only callable by the WebDataRequestManager? Alternately, why do the SetResult here? Can't we just pass it along via the Bind call to RequestCompletedOnThread, and clear the result field out of WebDataRequest?
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_...)
Thanks. An update suggested by Avi is pending, I'll ping you once it's uploaded. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.cc (left): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:129: DCHECK(i != pending_requests_.end()); On 2016/12/14 01:13:46, Mathieu Perreault wrote: > On 2016/12/13 22:33:55, Roger McFarlane (Chromium) wrote: > > This DCHECK isn't valid. > > > > Given concurrent cancellation/completion, both synchronized using > pending_lock_, > > if the cancellation grabs the lock first, it will remove the handle from > > pending_requests and DCHECK on debug builds, crash in release. > > It's still in your change, should we remove it? It should be valid now, since we re-check while holding the lock for whether the request is cancelled or not. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:40: bool WebDataRequest::IsCancelled() const { On 2016/12/14 01:13:46, Mathieu Perreault wrote: > IsInactive? Could be completed at this point too if I read correctly Done. But used IsActive instead, to avoid the double negative. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:45: // Of manager_ is null then the request has already been cancelled or On 2016/12/14 01:13:46, Mathieu Perreault wrote: > If Done. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:46: // completed, and the asserting context represents and invalid state. On 2016/12/14 01:13:46, Mathieu Perreault wrote: > *an Done. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:124: request->SetResult(std::move(result)); On 2016/12/14 04:40:39, Avi wrote: > If we no longer have the WebDatabaseBackend call SetResult on the request, but > are doing it here, should we make SetResult private and only callable by the > WebDataRequestManager? > > Alternately, why do the SetResult here? Can't we just pass it along via the Bind > call to RequestCompletedOnThread, and clear the result field out of > WebDataRequest? Ah, yes... should have made that private. But passing it along via the Bind is an even better idea! I'll do that, send another patchset, and re-ping. https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:134: if (request->IsCancelled()) On 2016/12/14 01:13:46, Mathieu Perreault wrote: > On 2016/12/13 22:33:55, Roger McFarlane (Chromium) wrote: > > An optimization, to attempt to avoid contending for the lock if the request > has > > already been cancelled/completed. The code should be equally correct without > > this check. > > This would be a worthwhile comment for a reader that may wonder why it's checked > again a few lines below This is the double-checked locking pattern. I'll add a comment to that effect. https://en.wikipedia.org/wiki/Double-checked_locking https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.h (right): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.h:86: friend class WebDataRequestManager; On 2016/12/14 01:13:46, Mathieu Perreault wrote: > I've seen those statements be right under the private: line > > https://cs.chromium.org/search/?q=%22friend+class%22+file:components&sq=packa... Done.
The CQ bit was checked by rogerm@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.
rogerm@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting for OWNERS PTAL?
https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/1/components/webdata/common/w... components/webdata/common/web_data_request_manager.cc:104: return ++next_request_handle_; On 2016/12/13 22:33:55, Roger McFarlane (Chromium) wrote: > Maybe make this an atomic type? > > As is, we take pending_lock_ twice on request creation: once to get the handle > and once to register the instance. > > Could also make the handle non-const and set it after creating the request in a > factory func. Hmmm... added factory func.
I did not do a functional review for the actual thread-safety issues, I assume your other reviewers did that. (I don't really know the threading model here well enough to be a good reviewer for that anyway.) LGTM https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:21: WebDataRequest::WebDataRequest(WebDataRequestManager* manager, Nit: While here, maybe fix function definition order to match declaration order. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:27: DCHECK(manager_ != nullptr); Nit: DCHECK(IsActive())? https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:44: inline void WebDataRequest::AssertThreadSafe() const { Nit: Remove "inline" (doesn't match class declaration, not really necessary anyway) https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:46: // active (i.e., the manager is non-null) and the manager's lock is held Nit: Remove "(i.e., the manager is non-null)" and change the DCHECK below to check IsActive() directly. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:91: request->handle_ = ++next_request_handle_; Nit: I think this should be next_request_handle_++. That way, the name is actually truthful: next_request_handle_ is the next handle that will be given out, not the most recent one to have been given out. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:127: // Placeholders for the request consumer, handle and result... to be used Nit: Comment seems inaccurate? I don't see the other such placeholders. I would just nuke the comment and the initializer. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:142: tracked_objects::ScopedTracker tracking_profile1( Nit: Rename this variable and the one below to no longer have numeric suffixes, since they are no longer in the same scope. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... File components/webdata/common/web_data_request_manager.h (right): https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.h:42: // was cancelled or has already completed. Nit: "has been was cancelled" https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.h:71: // a ref_ptr so that it can be set to NULL when a request is cancelled or Nit: NULL -> null https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.h:109: void AssertLockedByCurrentThread() const { pending_lock_.AssertAcquired(); } Nit: Avoid inlining non-"unix_hacker()" functions
The CQ bit was checked by rogerm@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 checked by rogerm@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...
Patchset #4 (id:60001) has been deleted
Thanks. mathp@... ping? avi@... ping? https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:21: WebDataRequest::WebDataRequest(WebDataRequestManager* manager, On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: While here, maybe fix function definition order to match declaration order. Done. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:27: DCHECK(manager_ != nullptr); On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: DCHECK(IsActive())? Done. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:44: inline void WebDataRequest::AssertThreadSafe() const { On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: Remove "inline" (doesn't match class declaration, not really necessary > anyway) Done. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:46: // active (i.e., the manager is non-null) and the manager's lock is held On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: Remove "(i.e., the manager is non-null)" and change the DCHECK below to > check IsActive() directly. Done. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:91: request->handle_ = ++next_request_handle_; On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: I think this should be next_request_handle_++. That way, the name is > actually truthful: next_request_handle_ is the next handle that will be given > out, not the most recent one to have been given out. Done. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:127: // Placeholders for the request consumer, handle and result... to be used On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: Comment seems inaccurate? I don't see the other such placeholders. > > I would just nuke the comment and the initializer. Oops, out of date comment. I'd prefer to leave a comment to the next reader. I've updated it to better reflect the why. WDYT? https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:142: tracked_objects::ScopedTracker tracking_profile1( On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: Rename this variable and the one below to no longer have numeric suffixes, > since they are no longer in the same scope. Done. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... File components/webdata/common/web_data_request_manager.h (right): https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.h:42: // was cancelled or has already completed. On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: "has been was cancelled" Done. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.h:71: // a ref_ptr so that it can be set to NULL when a request is cancelled or On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: NULL -> null Done. https://codereview.chromium.org/2571923002/diff/40001/components/webdata/comm... components/webdata/common/web_data_request_manager.h:109: void AssertLockedByCurrentThread() const { pending_lock_.AssertAcquired(); } On 2016/12/15 01:05:34, Peter Kasting wrote: > Nit: Avoid inlining non-"unix_hacker()" functions Done.
https://codereview.chromium.org/2571923002/diff/80001/components/webdata/comm... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/80001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:131: // Used to export the consumer from the locked region below so that consumer Yeah, this comment seems fine. Nit: I'd say "so that the" instead of "so that".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks. https://codereview.chromium.org/2571923002/diff/80001/components/webdata/comm... File components/webdata/common/web_data_request_manager.cc (right): https://codereview.chromium.org/2571923002/diff/80001/components/webdata/comm... components/webdata/common/web_data_request_manager.cc:131: // Used to export the consumer from the locked region below so that consumer On 2016/12/15 07:50:17, Peter Kasting wrote: > Yeah, this comment seems fine. > > Nit: I'd say "so that the" instead of "so that". Done.
lgtm
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2571923002/#ps100001 (title: "fix a comment")
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": 1481820404440280,
"parent_rev": "e6756d140e7f520cf63465ab3ba5b12e1278dd6b", "commit_rev":
"2901f97f345160dc646ac683a3ed91b3b7010860"}
Message was sent while issue was closed.
Description was changed from ========== Fix threading issues in WebDataRequestManager. This CL fixes a number of race conditions on request cancellation and completion. In particular: - Multiple cancellation calls on a request would DCHECK/crash. - A request could be cancelled concurrent to completion, resulting in a DCHECK/crash. - There was an ineffective lock internal to each request. Most of the critical code was already protected (though sometimes not properly) by the managers internal lock and the request lock wasn't adding any value. BUG=672823 ========== to ========== Fix threading issues in WebDataRequestManager. This CL fixes a number of race conditions on request cancellation and completion. In particular: - Multiple cancellation calls on a request would DCHECK/crash. - A request could be cancelled concurrent to completion, resulting in a DCHECK/crash. - There was an ineffective lock internal to each request. Most of the critical code was already protected (though sometimes not properly) by the managers internal lock and the request lock wasn't adding any value. BUG=672823 Review-Url: https://codereview.chromium.org/2571923002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix threading issues in WebDataRequestManager. This CL fixes a number of race conditions on request cancellation and completion. In particular: - Multiple cancellation calls on a request would DCHECK/crash. - A request could be cancelled concurrent to completion, resulting in a DCHECK/crash. - There was an ineffective lock internal to each request. Most of the critical code was already protected (though sometimes not properly) by the managers internal lock and the request lock wasn't adding any value. BUG=672823 Review-Url: https://codereview.chromium.org/2571923002 ========== to ========== Fix threading issues in WebDataRequestManager. This CL fixes a number of race conditions on request cancellation and completion. In particular: - Multiple cancellation calls on a request would DCHECK/crash. - A request could be cancelled concurrent to completion, resulting in a DCHECK/crash. - There was an ineffective lock internal to each request. Most of the critical code was already protected (though sometimes not properly) by the managers internal lock and the request lock wasn't adding any value. BUG=672823 Committed: https://crrev.com/95e2ecf771003012c97168343b58947e3ae93ef5 Cr-Commit-Position: refs/heads/master@{#438863} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/95e2ecf771003012c97168343b58947e3ae93ef5 Cr-Commit-Position: refs/heads/master@{#438863} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
