Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647323004:
Move download attribution right after CheckDownloadUrl (Closed)
|
Created:
3 years, 11 months ago by Jialiu Lin Modified:
3 years, 10 months ago CC:
asanka, chromium-reviews, grt+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSince SafeBrowsingNavigationObserverManager cleans up navigation
events every two minutes, if downloading a file takes more than 2
minutes, we'll get nothing from the attribution logic.
Therefore, this CL moves the download attribution logic to
after download url checking (before download starts).
This CL also changes the way of how referrer chain is constructed.
Instead of putting referrer entries in a temporary vector, I put
them directly into a RepeatedPtrField<ReferrerChainEntry> to avoid
extra copying/moving operations. This should mitigate the out-of
-memory issue.
BUG=683299, 676675
Review-Url: https://codereview.chromium.org/2647323004
Cr-Commit-Position: refs/heads/master@{#446761}
Committed: https://chromium.googlesource.com/chromium/src/+/20d16215e1bd867da6a23887046f4d6894222c00
Patch Set 1 #Patch Set 2 : nit #Patch Set 3 : threading refine comments #
Total comments: 10
Patch Set 4 : address naprker's comments #
Total comments: 8
Patch Set 5 : address comments from asanka@ #Patch Set 6 : nit #
Total comments: 12
Patch Set 7 : address potential race conditions #
Total comments: 2
Patch Set 8 : move callback to the end of CheckDone(). #Patch Set 9 : nit #Patch Set 10 : nits #Messages
Total messages: 56 (43 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: This issue passed the CQ dry run.
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.
Description was changed from ========== Move attribution to earlier BUG=683299 ========== to ========== Since SafeBrowsingNavigationObserverManager cleans up navigation events every two minutes, if downloading a file takes more than 2 minutes, we'll get nothing from the attribution logic. Therefore, this CL moves the download attribution logic to after download url checking (before download starts). This CL also changes the way of how referrer chain is constructed. Instead of putting referrer entries in a temporary vector, I put them directly into a RepeatedPtrField<ReferrerChainEntry> to avoid extra copying/moving operations. This should mitigate the out-of -memory issue. BUG=683299,676675 ==========
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...)
jialiul@chromium.org changed reviewers: + asanka@chromium.org, nparker@chromium.org
asanka@,nparker@, PTAL
https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:173: DownloadProtectionService* service, Could service_ get destroyed before this DownloadSBClient is done? If so, then you'd need something like a weakptr on DownloadProtectionService. Or, since SafeBrowsingService owns DownloadProtectionService, you could verify that SBS destroys its SafeBrowsingService _before_ the DPS (thus closing out all the Clients). That might be fragile, unless we document it carefully. https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:226: base::Bind(&DownloadSBClient::IdentifyReferrerChain, Why is this triggered in CheckDone() and SAFE? I thought the idea was to do it at the beginning. https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:300: DownloadProtectionService* service, Same lifetime question here as above. https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.h:263: static const void* const kDownloadReferrerChainDataKey; nit: could this be in a anonymous namespace in the .cc file instead? This and the others here. If they're used in tests, then never mind. https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:466: *(referrer_chain->Add()) = referrer_chain_entry; To avoid the copy, how about: referrer_chain->Add()->Swap(&referrer_chain_entry) Or do the Add() up top and use that ptr when setting the various fields.
Thanks! https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:173: DownloadProtectionService* service, On 2017/01/24 22:46:46, Nathan Parker wrote: > Could service_ get destroyed before this DownloadSBClient is done? If so, then > you'd need something like a weakptr on DownloadProtectionService. > > Or, since SafeBrowsingService owns DownloadProtectionService, you could verify > that SBS destroys its SafeBrowsingService _before_ the DPS (thus closing out all > the Clients). That might be fragile, unless we document it carefully. Done. Passed in a weakptr of DPS instead. https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:226: base::Bind(&DownloadSBClient::IdentifyReferrerChain, On 2017/01/24 22:46:46, Nathan Parker wrote: > Why is this triggered in CheckDone() and SAFE? I thought the idea was to do it > at the beginning. This is not the final SAFE verdict. At this point, chrome just finished download URL blacklist checking. If this download is already marked as dangerous (!SB_THREAT_TYPE_SAFE) because of its url, warning will be shown WITHOUT proceeding to content checking (thus, no ClientDownloadRequest will be sent out). So eventually, only these downloads pass blacklist checking and finishes actual downloading will have a chance to send out its referrer chain info (as part of ClientDownloadRequest). Potentially, we can include referrer info in the hit report too for these url blacklisted downloads. But I need to talk to SB team first to make sure there is an interested consumer of this data. https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:300: DownloadProtectionService* service, On 2017/01/24 22:46:46, Nathan Parker wrote: > Same lifetime question here as above. Done. https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.h (right): https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.h:263: static const void* const kDownloadReferrerChainDataKey; On 2017/01/24 22:46:46, Nathan Parker wrote: > nit: could this be in a anonymous namespace in the .cc file instead? This and > the others here. If they're used in tests, then never mind. Done. kDownloadPingTokenKey is used in a browser test. So I only put kDownloadReferrerChainDataKey in the anonymous namespace. https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2647323004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:466: *(referrer_chain->Add()) = referrer_chain_entry; On 2017/01/24 22:46:46, Nathan Parker wrote: > To avoid the copy, how about: > > referrer_chain->Add()->Swap(&referrer_chain_entry) > > Or do the Add() up top and use that ptr when setting the various fields. Yes! Done
gentle ping~
https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:262: if (service_) { The DownloadProtectionService and the SafeBrowsingService outlives the main loop (i.e. the UI thread). https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:269: item_->SetUserData( DownloadItem can go away abruptly for a number of reasons. I'd suggest: * Adding DownloadSBClient as a DownloadItem::Observer * Listening for DownloadItem::Observer::OnDownloadDestroyed(), and if seen, setting item_ to nullptr * Condition all UI thread interactions on item_ not being nullptr. You don't need to test for service_ nor retain a weak ref to it since it outlives the UI thread if it was ever alive.
lgtm after addressing asanka's comments https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:114: nit: 3 blank lines https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:269: item_->SetUserData( On 2017/01/26 18:54:47, asanka wrote: > DownloadItem can go away abruptly for a number of reasons. I'd suggest: > > * Adding DownloadSBClient as a DownloadItem::Observer > * Listening for DownloadItem::Observer::OnDownloadDestroyed(), and if seen, > setting item_ to nullptr > * Condition all UI thread interactions on item_ not being nullptr. > > You don't need to test for service_ nor retain a weak ref to it since it > outlives the UI thread if it was ever alive. The CheckDone() method above is called on the IO thread. I feel like we need to ensure that DPS hasn't been destroyed already. I think it depends on the destruction order within SafeBrowsingService. [looking..] The DB_manager (in SBS) will be destroyed before ServicesDelegate (which owns DPS), so then we're safe all around.
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 asanka@ and nparker@! DownloadSBClient is changed to a DownloadItem::Observer, and all the callings of item_ is conditioned. Also remove the WeakPtr wrapping DPS. https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:114: On 2017/01/26 19:29:10, Nathan Parker wrote: > nit: 3 blank lines Done. https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:262: if (service_) { On 2017/01/26 18:54:47, asanka wrote: > The DownloadProtectionService and the SafeBrowsingService outlives the main loop > (i.e. the UI thread). Thanks for confirming this. Removed checking for service_ and made it a regular pointer (instead of WeakPtr). https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:269: item_->SetUserData( On 2017/01/26 19:29:10, Nathan Parker wrote: > On 2017/01/26 18:54:47, asanka wrote: > > DownloadItem can go away abruptly for a number of reasons. I'd suggest: > > > > * Adding DownloadSBClient as a DownloadItem::Observer > > * Listening for DownloadItem::Observer::OnDownloadDestroyed(), and if seen, > > setting item_ to nullptr > > * Condition all UI thread interactions on item_ not being nullptr. > > > > You don't need to test for service_ nor retain a weak ref to it since it > > outlives the UI thread if it was ever alive. > > The CheckDone() method above is called on the IO thread. I feel like we need to > ensure that DPS hasn't been destroyed already. I think it depends on the > destruction order within SafeBrowsingService. > > [looking..] > > The DB_manager (in SBS) will be destroyed before ServicesDelegate (which owns > DPS), so then we're safe all around. Thanks for checking this out! https://codereview.chromium.org/2647323004/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/download_protection_service.cc:269: item_->SetUserData( On 2017/01/26 18:54:47, asanka wrote: > DownloadItem can go away abruptly for a number of reasons. I'd suggest: > > * Adding DownloadSBClient as a DownloadItem::Observer > * Listening for DownloadItem::Observer::OnDownloadDestroyed(), and if seen, > setting item_ to nullptr > * Condition all UI thread interactions on item_ not being nullptr. > > You don't need to test for service_ nor retain a weak ref to it since it > outlives the UI thread if it was ever alive. Done.
Lifetimes are hard. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:190: if (item_) |item_| had better be true here :-). For cases like this, it's best to just DCHECK since there's no point constructing a DownloadSBClient if we don't have a DownloadItem. Might as well DCHECK service_ as well. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:210: if (item_) FYI: This is totally fine. But another way to do this is to use a base::ScopedObserver<DownloadItem, DownloadItem::Observer>. This way we don't need to worry about manually removing the observer at the destruction time. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:221: base::Bind(callback_, result)); This is racy. |callback_| will be scheduled on the UI thread prior to IdentifyReferrerChain() below. Hence it's technically possible for the DownloadItem to proceed to CheckClientDownloadRequest(). If it does, then the referrer chain will not be present on the DownloadItem by then. This won't happen in practice due to the history and FILE thread hops that happen in-between, but those are implementation details and not things that are guaranteed by the APIs nor are they enforced via tests. It would be more robust to schedule |callback_| after IdentifyReferrerChain. Note that the ReportMalware task is OK because it doesn't modify our DownloadItem. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:230: } else if (service_ && service_->navigation_observer_manager() && There's a race here because CheckDone() could execute after the UI thread has been shutdown and service_ has been freed already. In that case this will be a UAF. We only verified that service_ is safe to access on the UI thread :-). One way to avoid this problem is to do check service_->navigation_observer_manager() in the DownloadSBCLient constructor and store a bool indicating whether we need to identify the referrer chain here. That way you don't have a racy dependency at CheckDone(). https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:272: if (item_) { Nit: return early if !item_. That way you'll have one fewer set of {}s to worry about. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:275: service_->IdentifyReferrerChain( Minor nit: Something nice to have would be for DPS::IdentifyReferrerChain to just return a unique_ptr<ReferrerChain>.
Patchset #7 (id:120001) has been deleted
Thank you so much asanka@! I probably won't realize these race conditions if you don't point them out. Thanks! https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:190: if (item_) On 2017/01/27 00:10:48, asanka wrote: > |item_| had better be true here :-). For cases like this, it's best to just > DCHECK since there's no point constructing a DownloadSBClient if we don't have a > DownloadItem. > > Might as well DCHECK service_ as well. You're right. DCHECK makes more sense. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:210: if (item_) On 2017/01/27 00:10:48, asanka wrote: > FYI: This is totally fine. But another way to do this is to use a > base::ScopedObserver<DownloadItem, DownloadItem::Observer>. This way we don't > need to worry about manually removing the observer at the destruction time. ScopedObserver is much cleaner. Thanks! https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:221: base::Bind(callback_, result)); On 2017/01/27 00:10:49, asanka wrote: > This is racy. |callback_| will be scheduled on the UI thread prior to > IdentifyReferrerChain() below. Hence it's technically possible for the > DownloadItem to proceed to CheckClientDownloadRequest(). If it does, then the > referrer chain will not be present on the DownloadItem by then. > > This won't happen in practice due to the history and FILE thread hops that > happen in-between, but those are implementation details and not things that are > guaranteed by the APIs nor are they enforced via tests. > > It would be more robust to schedule |callback_| after IdentifyReferrerChain. > > Note that the ReportMalware task is OK because it doesn't modify our > DownloadItem. You're right. Moved callback to the end of this function. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:230: } else if (service_ && service_->navigation_observer_manager() && On 2017/01/27 00:10:49, asanka wrote: > There's a race here because CheckDone() could execute after the UI thread has > been shutdown and service_ has been freed already. In that case this will be a > UAF. > > We only verified that service_ is safe to access on the UI thread :-). One way > to avoid this problem is to do check service_->navigation_observer_manager() in > the DownloadSBCLient constructor and store a bool indicating whether we need to > identify the referrer chain here. That way you don't have a racy dependency at > CheckDone(). Moved navigation_observer_manager() and feature checking to DownloadSBClient constructor. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:272: if (item_) { On 2017/01/27 00:10:49, asanka wrote: > Nit: return early if !item_. That way you'll have one fewer set of {}s to worry > about. Done. https://codereview.chromium.org/2647323004/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:275: service_->IdentifyReferrerChain( On 2017/01/27 00:10:49, asanka wrote: > Minor nit: Something nice to have would be for DPS::IdentifyReferrerChain to > just return a unique_ptr<ReferrerChain>. Done.
lgtm https://codereview.chromium.org/2647323004/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:224: base::Bind(callback_, result)); Forgot to move this? :-P
https://codereview.chromium.org/2647323004/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/download_protection_service.cc (right): https://codereview.chromium.org/2647323004/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/download_protection_service.cc:224: base::Bind(callback_, result)); On 2017/01/27 01:40:40, asanka wrote: > Forgot to move this? :-P Oops! Thanks for catching this.
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: 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
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Patchset #9 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Patchset #9 (id:200001) has been deleted
Patchset #9 (id:220001) has been deleted
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) 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...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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, asanka@chromium.org Link to the patchset: https://codereview.chromium.org/2647323004/#ps260001 (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": 260001, "attempt_start_ts": 1485546404501210,
"parent_rev": "06a97e0925f3f363b7968aae0fc7a9a5c3c070b4", "commit_rev":
"20d16215e1bd867da6a23887046f4d6894222c00"}
Message was sent while issue was closed.
Description was changed from ========== Since SafeBrowsingNavigationObserverManager cleans up navigation events every two minutes, if downloading a file takes more than 2 minutes, we'll get nothing from the attribution logic. Therefore, this CL moves the download attribution logic to after download url checking (before download starts). This CL also changes the way of how referrer chain is constructed. Instead of putting referrer entries in a temporary vector, I put them directly into a RepeatedPtrField<ReferrerChainEntry> to avoid extra copying/moving operations. This should mitigate the out-of -memory issue. BUG=683299,676675 ========== to ========== Since SafeBrowsingNavigationObserverManager cleans up navigation events every two minutes, if downloading a file takes more than 2 minutes, we'll get nothing from the attribution logic. Therefore, this CL moves the download attribution logic to after download url checking (before download starts). This CL also changes the way of how referrer chain is constructed. Instead of putting referrer entries in a temporary vector, I put them directly into a RepeatedPtrField<ReferrerChainEntry> to avoid extra copying/moving operations. This should mitigate the out-of -memory issue. BUG=683299,676675 Review-Url: https://codereview.chromium.org/2647323004 Cr-Commit-Position: refs/heads/master@{#446761} Committed: https://chromium.googlesource.com/chromium/src/+/20d16215e1bd867da6a23887046f... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001) as https://chromium.googlesource.com/chromium/src/+/20d16215e1bd867da6a23887046f... |
