Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
105853002:
Implement a background downloader using BITS in Windows Chrome. (Closed)
|
Created:
7 years ago by Sorin Jianu Modified:
7 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement a background downloader using BITS in Windows Chrome.
BUG=304354
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239347
Patch Set 1 #
Total comments: 9
Patch Set 2 : trybots and Josh's feedback. #
Total comments: 67
Patch Set 3 : cpu's feedback #
Total comments: 2
Patch Set 4 : removed useless helpers, fixed timer assert. #
Total comments: 2
Patch Set 5 : More fixes after cpu's review. #Patch Set 6 : Fixed ref counting. #Patch Set 7 : Removed BITS observer. #Patch Set 8 : Fixed typos. #
Total comments: 4
Patch Set 9 : Fixed error codes. #Messages
Total messages: 23 (0 generated)
There is one important case I want to address in a future CL: when the job is stuck in BITS we don't want to be stuck as well. I tested the code in the happy cases but not so much in the error cases, I'll keep testing it while we are reviewing the bulk of the code. I think the design is correct and issues would be relatively easy to fix.
lgtm, modulo the switch statement comment. The other comments are fluff. https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/background_downloader_win.cc:38: // the background, without any intervention. The job ban be completed next time ban -> can https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/background_downloader_win.cc:414: OnStateCancelled(); We fall through here? https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/crx_downloader.cc (right): https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/crx_downloader.cc:96: successor_->StartDownload(urls_); What if this guy returns false? I think this won't happen with the way the code is now, but maybe better to be safe than sorry? https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/test/component_updater_service_unittest.h (right): https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/test/component_updater_service_unittest.h:85: virtual bool UseBackgroundDownloader() const OVERRIDE; Can we also remove the extra \n from the non-virtuals below? I realize that you removed them here so that the comment was "scoped" around these, but I'm not sure I like that these are condensed but nothing else is.
Thank you! Btw, the Linux try bot failed. https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/background_downloader_win.cc:38: // the background, without any intervention. The job ban be completed next time On 2013/12/05 01:15:07, waffles wrote: > ban -> can Done. https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/background_downloader_win.cc:414: OnStateCancelled(); On 2013/12/05 01:15:07, waffles wrote: > We fall through here? we should not, thank you! I think this bug was an artifact on how I struggled to implement those states that should never occur. https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/crx_downloader.cc (right): https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/crx_downloader.cc:96: successor_->StartDownload(urls_); On 2013/12/05 01:15:07, waffles wrote: > What if this guy returns false? > > I think this won't happen with the way the code is now, but maybe better to be > safe than sorry? I will like to get rid of the is_active_ defensive programming too, so that becomes a moot point. https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/test/component_updater_service_unittest.h (right): https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/test/component_updater_service_unittest.h:85: virtual bool UseBackgroundDownloader() const OVERRIDE; On 2013/12/05 01:15:07, waffles wrote: > Can we also remove the extra \n from the non-virtuals below? I realize that you > removed them here so that the comment was "scoped" around these, but I'm not > sure I like that these are condensed but nothing else is. Done. https://codereview.chromium.org/105853002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/test/component_updater_service_unittest.h:85: virtual bool UseBackgroundDownloader() const OVERRIDE; On 2013/12/05 01:15:07, waffles wrote: > Can we also remove the extra \n from the non-virtuals below? I realize that you > removed them here so that the comment was "scoped" around these, but I'm not > sure I like that these are condensed but nothing else is. Done.
starting the review, more to come. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:32: // BITS job state. I haven't looked at the code yet but are we assuming that each component download url is unique? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:41: can you put here the minimalist command line to list the current bits jobs? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:58: const int kPurgeStaleJobsIntervalBetweenChecksDays = 1; These two seem to be good to expose in the configurator, but for a follow up CL. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.h (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.h:9: #include <windows.h> I think you need a space between 8 and 9, I am not style expert though.
https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:152: } seems we should have here the logic of JobDescriptionEqual() and thus the code will be cleaner. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:186: const base::Time creation_time(base::Time::FromFileTime(times.CreationTime)); seems incorrect to call 186 regardless of the value of hr. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:198: const string16& url) const { I think this should take a GURL& instead of a string. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:218: ScopedComPtr<IBackgroundCopyJob2> job2; is IBackgroundCopyJob2 Vista and above? also who calls this? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:226: Is there an option for no credentials? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:250: return hr; function also seems unused. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:263: HRESULT ResumeJob(IBackgroundCopyJob* job) { another unused function? unless you plan to use it in 559 https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:283: VLOG(1) << "Failed to instantiate BITS." << std::hex << hr; todo: add UMA or pings here. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:296: typedef base::Callback<void (void)> JobChangedCallback; extra space between typedef and base:: https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:324: DWORD reserved) OVERRIDE { indent 324 https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:365: base::Unretained(this), can it really be unretained? remind me of the lifetime of chain of responsibility pattern https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:372: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); dcheck(timer_ is null) ? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:431: can you use timer_->Reset() instead? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:547: hr = CreateOrOpenJob(url); I rather have CreateOrOpenJob just return the job (not set the job_) member and only after 556 be set as the member or even possibly on the caller of QueueBitsJob https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:553: if (FAILED(hr)) here you need to delete the job. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:575: hr = bits_manager_->CreateJob(L"", L"" ? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:592: &tempdir)) bikeshed purple: "chrome_BITS_" https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:596: base::SysUTF8ToWide(base::StringPiece(url.spec())).c_str(), you need stringpiece there?
how does it work if at 3pm we check and queue a BITs update for foo 1.5 and at 3:30 we check and get there is foo 1.6 ?
Thank you Carlos, let's have another look. All comments answered, and many of them acted upon. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:32: // BITS job state. On 2013/12/05 19:32:43, cpu wrote: > I haven't looked at the code yet but are we assuming that each component > download url is unique? We do, in the sense that we expect different files to be always hosted on different urls, in other words 1:1 between files and urls. Please tell me more about your concern, we want to get this assumption correct. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:41: On 2013/12/05 19:32:43, cpu wrote: > can you put here the minimalist command line to list the current bits jobs? Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:58: const int kPurgeStaleJobsIntervalBetweenChecksDays = 1; On 2013/12/05 19:32:43, cpu wrote: > These two seem to be good to expose in the configurator, but for a follow up CL. will do. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:152: } On 2013/12/05 21:16:31, cpu wrote: > seems we should have here the logic of JobDescriptionEqual() and thus the code > will be cleaner. This function takes any predicate and it iterates over all jobs. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:186: const base::Time creation_time(base::Time::FromFileTime(times.CreationTime)); On 2013/12/05 21:16:31, cpu wrote: > seems incorrect to call 186 regardless of the value of hr. Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:198: const string16& url) const { On 2013/12/05 21:16:31, cpu wrote: > I think this should take a GURL& instead of a string. The other predicates take string16 as well, at least for consistency reasons, string16 works nicely. The only difference would be where the conversion from GURL to string16 happens. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:218: ScopedComPtr<IBackgroundCopyJob2> job2; On 2013/12/05 21:16:31, cpu wrote: > is IBackgroundCopyJob2 Vista and above? also who calls this? yes. At the moment, nobody is calling it, I will have to add proxy support in a future CL. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:226: On 2013/12/05 21:16:31, cpu wrote: > Is there an option for no credentials? We can set proxies for a job. If we do a proxy request and the response is 407 then we would have to use this function to set the credentials. I don't know what happens if we set the credentials proactively for any proxy request. In general, the usage pattern seems to be: set the proxy, look at the response, if it is 401 or 407, set the credentials. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:250: return hr; On 2013/12/05 21:16:31, cpu wrote: > function also seems unused. Indeed, I wrote the functions that I will need soon. Is there an issue with this? If not used, it will not be linked in. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:263: HRESULT ResumeJob(IBackgroundCopyJob* job) { On 2013/12/05 21:16:31, cpu wrote: > another unused function? unless you plan to use it in 559 Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:283: VLOG(1) << "Failed to instantiate BITS." << std::hex << hr; On 2013/12/05 21:16:31, cpu wrote: > todo: add UMA or pings here. Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:296: typedef base::Callback<void (void)> JobChangedCallback; On 2013/12/05 21:16:31, cpu wrote: > extra space between typedef and base:: Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:324: DWORD reserved) OVERRIDE { On 2013/12/05 21:16:31, cpu wrote: > indent 324 Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:365: base::Unretained(this), On 2013/12/05 21:16:31, cpu wrote: > can it really be unretained? remind me of the lifetime of chain of > responsibility pattern The caller own the head of the chain, who owns the successors and so on. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:372: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); On 2013/12/05 21:16:31, cpu wrote: > dcheck(timer_ is null) ? Done. Nothing breaks in the code if the timer is not null. It does provide an indirect indication that some other invariant could be broken. Do you have a specific concern in mind with the timer object itself? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:431: On 2013/12/05 21:16:31, cpu wrote: > can you use timer_->Reset() instead? Done. Please notice I removed the Stop call at the beginning of this function, is that ok? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:547: hr = CreateOrOpenJob(url); On 2013/12/05 21:16:31, cpu wrote: > I rather have CreateOrOpenJob just return the job (not set the job_) member and > only > after 556 be set as the member or even possibly on the caller of QueueBitsJob I know what you mean. I tried to do the code that way and have CreateOrOpenJob be a factory method. I had an issue with the fact that until we can queue the job up, we have to created it, intialize it, and set an observer. These require a job_manager, a job, and a job observer be passed over between several functions as function arguments. The code looked unglier than what it is now. These functions are also only called from QueueBitsJob. QueueBitsJob either succeeds, or not, in which case, the job is cancelled, and we are done with the request. I am not pushing back on the idea, I am just articulating the thinking process. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:553: if (FAILED(hr)) On 2013/12/05 21:16:31, cpu wrote: > here you need to delete the job. job_ is refcounted. When the function runs again it will reset this interface pointer. Or it will be destroyed in the destructor. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:575: hr = bits_manager_->CreateJob(L"", On 2013/12/05 21:16:31, cpu wrote: > L"" ? I need a wide string literal, is the problem with the literal or the harcoded value? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:592: &tempdir)) On 2013/12/05 21:16:31, cpu wrote: > bikeshed purple: "chrome_BITS_" Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:596: base::SysUTF8ToWide(base::StringPiece(url.spec())).c_str(), On 2013/12/05 21:16:31, cpu wrote: > you need stringpiece there? I don't. Thank you for pointing it out. I noticed SysUTF8ToWide only takes a StringPiece argument but I did not occur to me that StringPiece has an implicit conversion from string. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.h (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.h:9: #include <windows.h> On 2013/12/05 19:32:43, cpu wrote: > I think you need a space between 8 and 9, I am not style expert though. Done.
On 2013/12/05 21:22:00, cpu wrote: > how does it work if at 3pm we check and queue a BITs update for foo 1.5 and at > 3:30 we check and get there is foo 1.6 ? If the download of 1.5 is successful, then we complete the job, we remove it from the BITS queue, and then we deal with 1.6 when its time comes. If the download of 1.5 in unsuccessful and the BITS job errors out, then we try a direct download. If Chrome shuts down before the download of 1.5 ends, then BITS continues the download unattended. If the then, Chrome wants to download 1.6, it will clean up 1.5 in 7 days from now. If the download of 1.5 encounters a recoverable error (network disconnect or some other error which is not final), then BITS will keep trying. I need to land a fix so that we don't block waiting for this to complete, I have a TODO in the code @432. As an idea, I wanted to use the PrefService to record the ids of jobs that we started so that we can go back and completed them proactively, instead of cleaning them up after 7 days.
Also could the observer be solely held by the reference keept by the job? Btw, overall is looking good. I am looking at the other files now. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:152: } On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > seems we should have here the logic of JobDescriptionEqual() and thus the > code > > will be cleaner. > > This function takes any predicate and it iterates over all jobs. Yes, but all the callers (predicates) filter out the ones without kJobDescription. Unless we are planning on iterating over other people's bits jobs? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:198: const string16& url) const { On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > I think this should take a GURL& instead of a string. > > The other predicates take string16 as well, at least for consistency reasons, > string16 works nicely. The only difference would be where the conversion from > GURL to string16 happens. It is frowned upon to use url params not being GURLs unless forced to, like for example in printf cases. If you are attached to this then rename the param to be remote_name. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:218: ScopedComPtr<IBackgroundCopyJob2> job2; On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > is IBackgroundCopyJob2 Vista and above? also who calls this? > Delete it please. > yes. At the moment, nobody is calling it, I will have to add proxy support in a > future CL. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:226: On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > Is there an option for no credentials? > > We can set proxies for a job. If we do a proxy request and the response is 407 > then we would have to use this function to set the credentials. I don't know > what happens if we set the credentials proactively for any proxy request. In > general, the usage pattern seems to be: set the proxy, look at the response, if > it is 401 or 407, set the credentials. > My only concern is leaking ntlm password hashes if we connect to an evil bits server. See the internet about pass-the-hash (ntlm proxy) attacks. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:250: return hr; On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > function also seems unused. > > Indeed, I wrote the functions that I will need soon. Is there an issue with > this? If not used, it will not be linked in. > delete it please, chrome style is not leave unused code. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:365: base::Unretained(this), On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > can it really be unretained? remind me of the lifetime of chain of > > responsibility pattern > > The caller own the head of the chain, who owns the successors and so on. > So the top dog in the chain does not get deleted until the CUS dies? https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:372: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > dcheck(timer_ is null) ? > > Done. > > Nothing breaks in the code if the timer is not null. It does provide an indirect > indication that some other invariant could be broken. Do you have a specific > concern in mind with the timer object itself? Not per se. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:431: On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > can you use timer_->Reset() instead? > > Done. Please notice I removed the Stop call at the beginning of this function, > is that ok? Cursory look tells me it is ok. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:547: hr = CreateOrOpenJob(url); On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > I rather have CreateOrOpenJob just return the job (not set the job_) member > and > > only > > after 556 be set as the member or even possibly on the caller of QueueBitsJob > > I know what you mean. I tried to do the code that way and have CreateOrOpenJob > be a factory method. I had an issue with the fact that until we can queue the > job up, we have to created it, intialize it, and set an observer. These require > a job_manager, a job, and a job observer be passed over between several > functions as function arguments. The code looked unglier than what it is now. > These functions are also only called from QueueBitsJob. QueueBitsJob either > succeeds, or not, in which case, the job is cancelled, and we are done with the > request. > > I am not pushing back on the idea, I am just articulating the thinking process. I see. I think there is a happy medium point, it can be kept as a member so you don't have to pass the job manager, etc. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:553: if (FAILED(hr)) On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > here you need to delete the job. > > job_ is refcounted. When the function runs again it will reset this interface > pointer. Or it will be destroyed in the destructor. it seems weird to have job_ point to a valid, yet broken bits job. I think we should strive to have whenever job_ is not null, it is a viable, healthy job. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:575: hr = bits_manager_->CreateJob(L"", On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > L"" ? > > I need a wide string literal, is the problem with the literal or the harcoded > value? The value, should it be something that helps finding them? https://codereview.chromium.org/105853002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/40001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:639: job_observer->AddRef(); you need this AddRef? at the very least SetNotifyInterface should have incremented it.
https://codereview.chromium.org/105853002/diff/60001/chrome/browser/component... File chrome/browser/component_updater/crx_downloader.cc (right): https://codereview.chromium.org/105853002/diff/60001/chrome/browser/component... chrome/browser/component_updater/crx_downloader.cc:54: urls.push_back(url); but we still have this method, so if you insist using an iterator then we need to do something here.
done with review.
I hope I addressed all the comments, many of them overlapped in flight. Thank you for doing the review. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:152: } On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > seems we should have here the logic of JobDescriptionEqual() and thus the > > code > > > will be cleaner. > > > > This function takes any predicate and it iterates over all jobs. > > Yes, but all the callers (predicates) filter out the ones without > kJobDescription. Unless we are planning on iterating over other people's bits > jobs? Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:198: const string16& url) const { On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > I think this should take a GURL& instead of a string. > > > > The other predicates take string16 as well, at least for consistency reasons, > > string16 works nicely. The only difference would be where the conversion from > > GURL to string16 happens. > > It is frowned upon to use url params not being GURLs unless forced to, like for > example in printf cases. > > If you are attached to this then rename the param to be remote_name. > > Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:218: ScopedComPtr<IBackgroundCopyJob2> job2; On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > is IBackgroundCopyJob2 Vista and above? also who calls this? > > > > Delete it please. > > yes. At the moment, nobody is calling it, I will have to add proxy support in > a > > future CL. > Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:226: On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > Is there an option for no credentials? > > > > We can set proxies for a job. If we do a proxy request and the response is 407 > > then we would have to use this function to set the credentials. I don't know > > what happens if we set the credentials proactively for any proxy request. In > > general, the usage pattern seems to be: set the proxy, look at the response, > if > > it is 401 or 407, set the credentials. > > > > My only concern is leaking ntlm password hashes if we connect to an evil bits > server. See the internet about pass-the-hash (ntlm proxy) attacks. > Understood. Let's talk about proxy authentication after this lands. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:250: return hr; On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > function also seems unused. > > > > Indeed, I wrote the functions that I will need soon. Is there an issue with > > this? If not used, it will not be linked in. > > > > delete it please, chrome style is not leave unused code. Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:263: HRESULT ResumeJob(IBackgroundCopyJob* job) { On 2013/12/05 22:34:36, Sorin Jianu wrote: > On 2013/12/05 21:16:31, cpu wrote: > > another unused function? unless you plan to use it in 559 > > Done. deleted. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:365: base::Unretained(this), On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > can it really be unretained? remind me of the lifetime of chain of > > > responsibility pattern > > > > The caller own the head of the chain, who owns the successors and so on. > > > > So the top dog in the chain does not get deleted until the CUS dies? The life cycle of the downloader and its successors is determined by the CUS. When CUS needs a download, it instantiates the downloader and retains the ownership of the download object until the download completes. See CrxUpdateService::UpdateComponent and CrxUpdateService::DownloadComplete where the downloader is created and destroyed. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:372: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > dcheck(timer_ is null) ? > > > > Done. > > > > Nothing breaks in the code if the timer is not null. It does provide an > indirect > > indication that some other invariant could be broken. Do you have a specific > > concern in mind with the timer object itself? > > Not per se. Done. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:431: On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > can you use timer_->Reset() instead? > > > > Done. Please notice I removed the Stop call at the beginning of this function, > > is that ok? > > Cursory look tells me it is ok. I put the call the Stop back since I check as a post condition somewhere that the timer should not be running when the download ends. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:547: hr = CreateOrOpenJob(url); On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > I rather have CreateOrOpenJob just return the job (not set the job_) member > > and > > > only > > > after 556 be set as the member or even possibly on the caller of > QueueBitsJob > > > > I know what you mean. I tried to do the code that way and have CreateOrOpenJob > > be a factory method. I had an issue with the fact that until we can queue the > > job up, we have to created it, intialize it, and set an observer. These > require > > a job_manager, a job, and a job observer be passed over between several > > functions as function arguments. The code looked unglier than what it is now. > > These functions are also only called from QueueBitsJob. QueueBitsJob either > > succeeds, or not, in which case, the job is cancelled, and we are done with > the > > request. > > > > I am not pushing back on the idea, I am just articulating the thinking > process. > > I see. I think there is a happy medium point, it can be kept as a member so you > don't have to pass the job manager, etc. Thank you! https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:553: if (FAILED(hr)) On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > here you need to delete the job. > > > > job_ is refcounted. When the function runs again it will reset this interface > > pointer. Or it will be destroyed in the destructor. > > it seems weird to have job_ point to a valid, yet broken bits job. I think we > should strive to have whenever job_ is not null, it is a viable, healthy job. Done. I moved resetting the job pointer @322 where the job is being cancelled as well. Any job that errors out after it is created and before it could start has to be cancelled, so it does not fill up the BITS queue. BackgroundDownloader::BeginDownload is the function that queues the job up, or cleans it up by cancelling it and now, resetting the interface pointer too. https://codereview.chromium.org/105853002/diff/20001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:575: hr = bits_manager_->CreateJob(L"", On 2013/12/06 00:12:51, cpu wrote: > On 2013/12/05 22:34:36, Sorin Jianu wrote: > > On 2013/12/05 21:16:31, cpu wrote: > > > L"" ? > > > > I need a wide string literal, is the problem with the literal or the harcoded > > value? > > The value, should it be something that helps finding them? Oh, that parameter is the job display name, which gets set one function call later, as the file name to download. I changed the code to initialize it with the description name, in case it is never set by accident. https://codereview.chromium.org/105853002/diff/60001/chrome/browser/component... File chrome/browser/component_updater/crx_downloader.cc (right): https://codereview.chromium.org/105853002/diff/60001/chrome/browser/component... chrome/browser/component_updater/crx_downloader.cc:54: urls.push_back(url); On 2013/12/06 00:23:19, cpu wrote: > but we still have this method, so if you insist using an iterator then we need > to do something here. Let's discuss this. Right off the bat, this function is not pushing into the existing container. This is just a convenience function that makes a new container out of an url, and calls the function that sets it object up for a download. Aside from the iterator vs. index discussion, the design issue here is: do we allow reentrant calls for StartDownload? After thoughtful consideration, we decided not to. This class can only do one download at one time, and since the cost of creating this class is small, after the download is complete, it should be disposed of. This has been the contract for the class from the beginning and I think although I might be wrong, that it is the idiomatic usage for such non-blocking objects. If we agree with this, then the next question is, should be protect this class against misuse? I had the code to defensively protect it using an is_active member, which I later on removed, for it was ugly and I did not think that was the spirit of Chromium anyway. Re: iterator vs. index, I'd like to make two statements. First, the code does not result in dereferencing invalid iterators even if called in a reentrant manner. First, the object is single threaded. Second, when called a second time, the iterator is reset to the beginning of the new container, and it is valid. If a notification completes later on, the iterator is valid and it is always checked against the end. An index would have the similar issues with the iterator. While the index as a value is always valid, it still need to be checked for out of bounds. I had the code with the index, I forgot to mention that I changed it back to an iterator, since the iterator is more convenient to erase from the container, in case I need to drop the 5xx urls.
On 2013/12/06 00:12:51, cpu wrote: > Also could the observer be solely held by the reference keept by the job? That is a neat idea. I am retaining the job observer ownership to employ some defensive programming at 606 (as much as I dislike it). I am afraid of spurious notification after I unregister the observer. Since the observer is ref counted but the rest of the CrxDownloader is not, I want to protect against a BITS event in flight that fires after CUS has destroyed the downloader and the observer has to call it back.
https://codereview.chromium.org/105853002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/40001/chrome/browser/component... chrome/browser/component_updater/background_downloader_win.cc:639: job_observer->AddRef(); On 2013/12/06 00:12:52, cpu wrote: > you need this AddRef? at the very least SetNotifyInterface should have > incremented it. We need the addref since we are taking over the interface pointer and the object is created with a m_dwRef == 0; But I've found the straightforward way to avoid the AddRef. Therefore, we own the object and before setting it up as an observer, the ref count is 1.
lgtm. Excellent job btw.
As for the questions above, I am not too concerned and mostly agree including no to reentrancy. Once the code lands I'll take a look directly on my trusty editor. I think now that the big missing pieces are landing we can talk about a wholesale restructuring of the CUS. I'll write an email about this.
Josh, I removed the observer, let's take another look please.
lgtm, except the two hr = job_->Cancels. https://codereview.chromium.org/105853002/diff/140001/chrome/browser/componen... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/140001/chrome/browser/componen... chrome/browser/component_updater/background_downloader_win.cc:384: hr = job_->Cancel(); As mentioned in chat, the hr= here seems wrong. Also on line 404. https://codereview.chromium.org/105853002/diff/140001/chrome/browser/componen... chrome/browser/component_updater/background_downloader_win.cc:405: EndDownload(hr); I think this should be error_code.
Done both, thank you JOsh! https://codereview.chromium.org/105853002/diff/140001/chrome/browser/componen... File chrome/browser/component_updater/background_downloader_win.cc (right): https://codereview.chromium.org/105853002/diff/140001/chrome/browser/componen... chrome/browser/component_updater/background_downloader_win.cc:384: hr = job_->Cancel(); On 2013/12/06 22:08:57, waffles wrote: > As mentioned in chat, the hr= here seems wrong. Also on line 404. Done. https://codereview.chromium.org/105853002/diff/140001/chrome/browser/componen... chrome/browser/component_updater/background_downloader_win.cc:405: EndDownload(hr); On 2013/12/06 22:08:57, waffles wrote: > I think this should be error_code. If we get here, then the error_code could not be retrieved.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/105853002/160001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sorin@chromium.org/105853002/160001
Message was sent while issue was closed.
Change committed as 239347 |
