Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817533004:
Improve PasswordProtectionService and PasswordProtectionRequest (Closed)
|
Created:
3 years, 8 months ago by Jialiu Lin Modified:
3 years, 8 months ago CC:
chromium-reviews, vakh+watch_chromium.org, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, timvolodine, blundell+watchlist_chromium.org, lpz Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
Review-Url: https://codereview.chromium.org/2817533004
Cr-Commit-Position: refs/heads/master@{#465695}
Committed: https://chromium.googlesource.com/chromium/src/+/3cffb12884df959d94c1e3bb67e9d8bcae2c339a
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 20
Patch Set 3 : address nparker's comments #
Total comments: 6
Patch Set 4 : address comments from lpz #Patch Set 5 : revert fieldtrial #
Total comments: 10
Patch Set 6 : address more comments #
Total comments: 6
Patch Set 7 : nits #
Messages
Total messages: 49 (37 generated)
The CQ bit was checked by jialiul@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Description was changed from
==========
Refactor PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) copy verdict_token field from csd_client.proto in google3 to
csd.proto, so message definition of LoginReputationClientResponse
is consisted on client and server.
(4) Add PasswordProtectionFrame struct, which will be used to communicate
password form information between ChromePasswordManagerClient and
PasswordProtectionService
BUG=698899
==========
to
==========
Refactor PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) copy verdict_token field from csd_client.proto in google3 to
csd.proto, so message definition of LoginReputationClientResponse
is consisted on client and server.
(4) Add PasswordProtectionFrame struct, which will be used to communicate
password form information between ChromePasswordManagerClient and
PasswordProtectionService
BUG=698899
==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Refactor PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) copy verdict_token field from csd_client.proto in google3 to
csd.proto, so message definition of LoginReputationClientResponse
is consisted on client and server.
(4) Add PasswordProtectionFrame struct, which will be used to communicate
password form information between ChromePasswordManagerClient and
PasswordProtectionService
BUG=698899
==========
to
==========
Refactor PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) copy verdict_token field from csd_client.proto in google3 to
csd.proto, so message definition of LoginReputationClientResponse
is consisted on client and server.
(4) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
==========
The CQ bit was checked by jialiul@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jialiul@chromium.org changed reviewers: + lpz@chromium.org
Hi lpz@, Can I bother you with one more CL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Refactor PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) copy verdict_token field from csd_client.proto in google3 to
csd.proto, so message definition of LoginReputationClientResponse
is consisted on client and server.
(4) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
==========
to
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) copy verdict_token field from csd_client.proto in google3 to
csd.proto, so message definition of LoginReputationClientResponse
is consisted on client and server.
(4) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
==========
jialiul@chromium.org changed reviewers: + nparker@chromium.org - lpz@chromium.org
-lpz@ Sorry, I didn't realize it is a public holiday in Canada. +nparker@, Could you take a look? Thanks!
I'm not clear on why at refcounted ptr is required (I may just be missing the problem w/ existing implementation). Can't the list of requests_ properly own them? https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:30: password_protection_service_(pps), Do we need both the PPS and the DBMGR? We can get the DBMGR from the PPS. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:59: bool* match_whitelist = new bool(false); This seems like it'd get leaked. Why a ptr, and then why not a unique_ptr? https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:65: base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone, this, What's wrong with the weak-ptr approach here? https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:81: // otherwise call CheckCachedVerdicts(). Comment on why this is a ptr. Does null mean something different? https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:66: PasswordProtectionFrame::PasswordProtectionFrame() { I wonder if you can delete this default ctor. Maybe "PasswordProtectionFrame() = delete;" in the .h file. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:98: bool* match_whitelist = new bool(false); bare pointer alert. :-) Could this be a unique_ptr? https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:111: if (url.is_valid()) So no-standard URLs will default to "whitelisted=false," ya? Actually, maybe we want the default to be "true." That's what MatchCsdWhitelistUrl() will default to (for pver4), if the store isn't available. BTW this whitelist isn't available on Chromium (non-chrome branded) builds currently, so it'll default to true. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:224: // Don't start request if |main_frame_url| is invalid or is NOT http or https. nit: this comment isn't necessary, since the code says the same thing. Could say: Skip URLs that we can't get a reliable reputation for https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:34: struct PasswordProtectionFrame; nit: You can remove this like if you move the "using PasswordProtectionFrames" below the struct definition. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:102: scoped_refptr<SafeBrowsingDatabaseManager> database_manager(); nit: const;
The CQ bit was checked by jialiul@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...
Thanks nparker@! According to http://dev.chromium.org/developers/design-documents/threading#base_Bind "...By default, the object that PostTask uses must be a thread-safe reference-counted object..." RefCounted class requires a protected or private destructor, and doesn't work too well together with unique_ptr or STL containers (by default they both need public destructor). This usage is similar to the DownloadProtectionService::CheckClientDownloadRequest class in download_protection_service.cc https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:30: password_protection_service_(pps), On 2017/04/17 20:28:02, Nathan Parker wrote: > Do we need both the PPS and the DBMGR? We can get the DBMGR from the PPS. Ah, you're totally right. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:59: bool* match_whitelist = new bool(false); On 2017/04/17 20:28:02, Nathan Parker wrote: > This seems like it'd get leaked. Why a ptr, and then why not a unique_ptr? I was following this example. http://dev.chromium.org/developers/design-documents/threading#CancelableTaskT... Here I relies on this bool object to live long enough till the end of OnWhitelistCheckDone(). unique_ptr may not be a good idea, since the first callback (PPS::CheckCsdWhitelistOnIOThread) does not take ownership of this bool, and unique_ptr is not thread safe. It is much easier to handle the ownership by using base::Owned() here. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.cc:65: base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone, this, On 2017/04/17 20:28:02, Nathan Parker wrote: > What's wrong with the weak-ptr approach here? WeakPtr is not thread safe when handling cancelable task across different threads. http://dev.chromium.org/developers/design-documents/threading#base_WeakPtr_an... The recommended way is to use CancelableTaskTracker. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:81: // otherwise call CheckCachedVerdicts(). On 2017/04/17 20:28:02, Nathan Parker wrote: > Comment on why this is a ptr. Does null mean something different? Done. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.cc (right): https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:66: PasswordProtectionFrame::PasswordProtectionFrame() { On 2017/04/17 20:28:02, Nathan Parker wrote: > I wonder if you can delete this default ctor. > Maybe "PasswordProtectionFrame() = delete;" in the .h file. Done. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:98: bool* match_whitelist = new bool(false); On 2017/04/17 20:28:02, Nathan Parker wrote: > bare pointer alert. :-) > > Could this be a unique_ptr? Similar to my reply in PasswordProtectionRequest class, a bare pointer works better in PostTaskAndReply(). And base::Owned() will take care of the ownership. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:111: if (url.is_valid()) On 2017/04/17 20:28:02, Nathan Parker wrote: > So no-standard URLs will default to "whitelisted=false," ya? > > Actually, maybe we want the default to be "true." That's what > MatchCsdWhitelistUrl() will default to (for pver4), if the store isn't > available. BTW this whitelist isn't available on Chromium (non-chrome branded) > builds currently, so it'll default to true. Thanks for pointing this out. Changed default to true. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.cc:224: // Don't start request if |main_frame_url| is invalid or is NOT http or https. On 2017/04/17 20:28:02, Nathan Parker wrote: > nit: this comment isn't necessary, since the code says the same thing. > > Could say: Skip URLs that we can't get a reliable reputation for Done. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:34: struct PasswordProtectionFrame; On 2017/04/17 20:28:02, Nathan Parker wrote: > nit: You can remove this like if you move the "using PasswordProtectionFrames" > below the struct definition. Done. https://codereview.chromium.org/2817533004/diff/40001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:102: scoped_refptr<SafeBrowsingDatabaseManager> database_manager(); On 2017/04/17 20:28:02, Nathan Parker wrote: > nit: const; Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lpz@chromium.org changed reviewers: + lpz@chromium.org
Just driving by with mostly nits. https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... components/safe_browsing/csd.proto:254: // A token which is consistent in the response and post-warning actions. So this field is not used in chrome code yet, right? Are there plans to? Might seem strange to have a field that is never accessed https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:81: // |match_whitelist| points to the boolean value set on IO thread by nit: further to Nathan's comment, can we explain more about *why* match_whitelist is a pointer? As a newbie, and after looking at the actual code of CheckCsdWhitelistOnIOThread, I can kind of glean that it's something to do with passing stuff between threads, but mentioning something about how ownership is done via base::Owned bit might help too? https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:51: using PasswordProtectionFrames = Perhaps PasswordProtectionFrameList? For a while I thought the unique_ptrs below had typos in them but the code was magically compiling anyway :o
Description was changed from
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) copy verdict_token field from csd_client.proto in google3 to
csd.proto, so message definition of LoginReputationClientResponse
is consisted on client and server.
(4) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
==========
to
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
(4) Add finch experiment to fieldtrail_testing_config.json file
BUG=698899
==========
Thanks lpz@! :-) https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... File components/safe_browsing/csd.proto (right): https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... components/safe_browsing/csd.proto:254: // A token which is consistent in the response and post-warning actions. On 2017/04/18 14:43:27, lpz wrote: > So this field is not used in chrome code yet, right? Are there plans to? Might > seem strange to have a field that is never accessed This field is going to be used in the post-incident reporting. I'll move it to later CL when it is actually being used. https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_request.h:81: // |match_whitelist| points to the boolean value set on IO thread by On 2017/04/18 14:43:27, lpz wrote: > nit: further to Nathan's comment, can we explain more about *why* > match_whitelist is a pointer? As a newbie, and after looking at the actual code > of CheckCsdWhitelistOnIOThread, I can kind of glean that it's something to do > with passing stuff between threads, but mentioning something about how ownership > is done via base::Owned bit might help too? Done. https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2817533004/diff/60001/components/safe_browsin... components/safe_browsing/password_protection/password_protection_service.h:51: using PasswordProtectionFrames = On 2017/04/18 14:43:27, lpz wrote: > Perhaps PasswordProtectionFrameList? For a while I thought the unique_ptrs below > had typos in them but the code was magically compiling anyway :o Sure. Done.
Description was changed from
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
(4) Add finch experiment to fieldtrail_testing_config.json file
BUG=698899
==========
to
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
==========
Description was changed from
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
==========
to
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
(4) Add experiment config in fieldtrial_testing_config.json
BUG=698899
==========
Description was changed from
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
(4) Add experiment config in fieldtrial_testing_config.json
BUG=698899
==========
to
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
==========
Thanks for the explanations, this makes sense to me now. Threading is hard. :-) Just a few comments https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:18: namespace safe_browsing { Maybe it'd be good to have a state diagram on what's happening on each thread (imagine you need to touch this code a year from now.) Something like UI: Add task to IO thread for whitelist IO: Check whitelist and add task to UI thread UI: If not whitelisted: start a timeout task on UI, and start fetcher on UI [wait for network response] UI: Handle response, or timeout, or cancel events. https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:147: BrowserThread::PostDelayedTask( Should this use the tracker_ too, so it'll get canceled? https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:187: tracker_.TryCancelAll(); Will this automatically get called in the destructor, in ->RequestFinished below? https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:24: class PasswordProtectionRequest Add a comment as to why it's recounted (destruction on different threads) https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:111: std::unique_ptr<PasswordProtectionFrameList> pending_password_frames_; Add a comment. It's not obvious to me what pending means here..
Thanks! https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.cc (right): https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:18: namespace safe_browsing { On 2017/04/18 23:07:26, Nathan Parker wrote: > Maybe it'd be good to have a state diagram on what's happening on each thread > (imagine you need to touch this code a year from now.) Something like > > UI: Add task to IO thread for whitelist > IO: Check whitelist and add task to UI thread > UI: If not whitelisted: start a timeout task on UI, and start fetcher on UI > [wait for network response] > UI: Handle response, or timeout, or cancel events. Added a flow diagram in the .h file. https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:147: BrowserThread::PostDelayedTask( On 2017/04/18 23:07:26, Nathan Parker wrote: > Should this use the tracker_ too, so it'll get canceled? Since this is posted to the same thread, WeakPtr will do the trick. In fact, CancelableTaskTracker doesn't handle PostDelayedTask(). https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.cc:187: tracker_.TryCancelAll(); On 2017/04/18 23:07:26, Nathan Parker wrote: > Will this automatically get called in the destructor, in ->RequestFinished > below? Yes. The destruction of PasswordProtectionRequest can be triggered in 2 cases only: (1) request finished, or canceled. --> Finish(..) is explicitly called. (2) password_protection_service_ is deleted. Then the calling sequence is: PasswordProtectionService::~PasswordProtectionService() PasswordProtectionService::CancelPendingRequests() PasswordProtectionRequest::Cancel() PasswordProtectionRequset::Finish() PasswordProtectionService::RequestFinished() PasswordProtectionRequest::~PasswordProtectionRequest() https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:24: class PasswordProtectionRequest On 2017/04/18 23:07:26, Nathan Parker wrote: > Add a comment as to why it's recounted (destruction on different threads) Thanks for the reminder! Actually, it should only be deleted on UI thread. Comment added, code fixed. https://codereview.chromium.org/2817533004/diff/100001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:111: std::unique_ptr<PasswordProtectionFrameList> pending_password_frames_; On 2017/04/18 23:07:26, Nathan Parker wrote: > Add a comment. It's not obvious to me what pending means here.. "pending_" is irrelevant here. Removed "pending_" prefix and added a comment.
The CQ bit was checked by jialiul@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.
lgtm https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:29: // (1) | UI | If incognito or !SBER, quit request. Nice! This not only helps for the future, it helps us spot any holes https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:65: base::WeakPtr<PasswordProtectionService> pps, One more question: if ~PPS destroys all the PPR's, then you don't need a weakptr? It looks like you're accessing the weakptr on multiple threads anyway, which may make its state unreliable. https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:101: const scoped_refptr<SafeBrowsingDatabaseManager> database_manager(); nit: I don't think the const adds anything, since it's going to call the copy constructor anyway.
Thank you nparker and lpz! All comments addressed. https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_request.h (right): https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:29: // (1) | UI | If incognito or !SBER, quit request. On 2017/04/19 17:20:53, Nathan Parker wrote: > Nice! This not only helps for the future, it helps us spot any holes :-) https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_request.h:65: base::WeakPtr<PasswordProtectionService> pps, On 2017/04/19 17:20:53, Nathan Parker wrote: > One more question: if ~PPS destroys all the PPR's, then you don't need a > weakptr? It looks like you're accessing the weakptr on multiple threads anyway, > which may make its state unreliable. Yep, you're right. Changing it to regular pointer. https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... File components/safe_browsing/password_protection/password_protection_service.h (right): https://codereview.chromium.org/2817533004/diff/120001/components/safe_browsi... components/safe_browsing/password_protection/password_protection_service.h:101: const scoped_refptr<SafeBrowsingDatabaseManager> database_manager(); On 2017/04/19 17:20:53, Nathan Parker wrote: > nit: I don't think the const adds anything, since it's going to call the copy > constructor anyway. Removed.
The CQ bit was checked by jialiul@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.
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2817533004/#ps140001 (title: "nits")
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": 140001, "attempt_start_ts": 1492628600551450,
"parent_rev": "bb7503ea498af3d78bf3577c5a32d400b74d29b1", "commit_rev":
"3cffb12884df959d94c1e3bb67e9d8bcae2c339a"}
Message was sent while issue was closed.
Description was changed from
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
==========
to
==========
Improve PasswordProtectionService and PasswordProtectionRequest
(1) refactor PaswordProtectionRequest to be RefCountedThreadSafe,
(2) use CancelableTaskTracker in PasswordProtectionRequest to make
sure tasks posted on IO thread can be canceled correctly upon
destructing PasswordProtectionService,
(3) Add the skeleton of PasswordProtectionFrame struct, which will be
used to communicate password form information between
ChromePasswordManagerClient and PasswordProtectionService
BUG=698899
Review-Url: https://codereview.chromium.org/2817533004
Cr-Commit-Position: refs/heads/master@{#465695}
Committed:
https://chromium.googlesource.com/chromium/src/+/3cffb12884df959d94c1e3bb67e9...
==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/3cffb12884df959d94c1e3bb67e9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
