Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943843002:
Convert to CupsPrintJobManagerImpl to TaskScheduler. (Closed)
DescriptionConvert to CupsPrintJobManagerImpl to TaskScheduler.
Place status polling on a sequenced task runner. Run Cancel as a user visible task.
BUG=734280, 667892, 689520
Review-Url: https://codereview.chromium.org/2943843002
Cr-Commit-Position: refs/heads/master@{#483394}
Committed: https://chromium.googlesource.com/chromium/src/+/bab2208a02c09baa03cb7cffd1bc3f6c996fa04c
Patch Set 1 #Patch Set 2 : add an io assertion #
Total comments: 6
Patch Set 3 : ready #Patch Set 4 : notification must happen on the browser thread #
Total comments: 2
Patch Set 5 : somewhat reasonable #
Total comments: 12
Patch Set 6 : as weak ptr #
Total comments: 4
Patch Set 7 : format #
Total comments: 8
Patch Set 8 : strong pointers and DCHECKs #
Total comments: 5
Messages
Total messages: 24 (10 generated)
skau@chromium.org changed reviewers: + gab@chromium.org, justincarlson@chromium.org
Please review. Migrating to TaskScheduler. https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (left): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:304: content::BrowserThread::FILE_USER_BLOCKING, FROM_HERE, This was higher than necessary.
https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (left): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:197: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Are you removing the requirement, or just the DCHECK? If the requirement is being relaxed, update the comments as well? https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:210: FROM_HERE, base::TaskTraits(base::TaskPriority::USER_VISIBLE), I don't understand how concurrency safety works with this change. CancelJobImpl changes data also touched by other functions that could (AFAICT) be running at the same time.
lgtm (only potential issue I see is with pre-existing race) https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:210: FROM_HERE, base::TaskTraits(base::TaskPriority::USER_VISIBLE), On 2017/06/20 19:02:09, Carlson wrote: > I don't understand how concurrency safety works with this change. CancelJobImpl > changes data also touched by other functions that could (AFAICT) be running at > the same time. Right, seems this was racy before and still is..? Unless this is somehow sequenced at the end of everything (in which case there wouldn't be a disadvantage to not posting it in parallel) https://codereview.chromium.org/2943843002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:192: browser_thread_runner_(browser_thread_runner), std::move
Description was changed from ========== Convert to CupsPrintJobManagerImpl to TaskScheduler. Place status polling on a sequenced task runner. Run Cancel as a user visible task. BUG=734280 ========== to ========== Convert to CupsPrintJobManagerImpl to TaskScheduler. Place status polling on a sequenced task runner. Run Cancel as a user visible task. BUG=734280, 667892, 689520 ==========
I've been running this code under debug and found a sequencing bug IRT WeakPtrs. I've tried to fix it but if I'm reading the WeakPtrFactory documentation correctly, there could be a race during WeakPtrFactory destruction since it's on a different sequence. https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (left): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:197: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/06/20 19:02:09, Carlson wrote: > Are you removing the requirement, or just the DCHECK? If the requirement is > being relaxed, update the comments as well? The requirement still exits. It's now enforced as a sequence check. https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:210: FROM_HERE, base::TaskTraits(base::TaskPriority::USER_VISIBLE), On 2017/06/20 21:04:49, gab (OOO Friday) wrote: > On 2017/06/20 19:02:09, Carlson wrote: > > I don't understand how concurrency safety works with this change. > CancelJobImpl > > changes data also touched by other functions that could (AFAICT) be running at > > the same time. > > Right, seems this was racy before and still is..? Unless this is somehow > sequenced at the end of everything (in which case there wouldn't be a > disadvantage to not posting it in parallel) I've sequenced QueryJobs and CancelPrintJobImpl by wrapping it in an object and guaranteeing the sequencing. https://codereview.chromium.org/2943843002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:192: browser_thread_runner_(browser_thread_runner), On 2017/06/20 21:04:49, gab (OOO Friday) wrote: > std::move It's not injected anymore.
https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:219: DETACH_FROM_SEQUENCE(sequence_checker_); I don't understand what this does (which is probably just be being unfamiliar with this mechanism). Are constructors for sequence checkers normally automatically required to be sequenced? I don't understand why you need this here (as opposed to just *not* having a DCHECK_CALLED_ON_VALID_SEQUENCE() call). https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:278: // Be sure to copy out all relevant fields. |job| may be destroyed after we Is this comment still relevant? https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:39: class CupsWrapper { Class comment, esp why this class exists in the first place. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:53: // Access only on query_runner_. It's not obvious here what query_runner_ you're talking about. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:57: }; I think this is still unsafe to me, w.r.t destruction. You're using weak ptr to ensure that we don't *start* any callbacks after the Wrapper is destroyed, but as I understand it, that mechanism can't be used outside of an entirely sequenced context -- e.g., the weak ptr guarantee requires that the destructor be sequenced with respect to the callbacks it's cancelling. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:112: base::WeakPtrFactory<CupsWrapper> cups_wrapper_ptr_factory_; I don't understand why you aren't putting the weak ptr factory into CupsWrapper directly? I think what you've done here probably works, but it certainly breaks the normal WeakPtrFactory pattern; I'd be inclined to put the WeakPtrFactory into the wrapper and expose it as a member function just to preserve the pattern and make this easier for people to understand.
Addressed most of the comments. Trying to find a solution to the possible use-after-destroy bug. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:219: DETACH_FROM_SEQUENCE(sequence_checker_); On 2017/06/26 18:42:01, Carlson wrote: > I don't understand what this does (which is probably just be being unfamiliar > with this mechanism). Are constructors for sequence checkers normally > automatically required to be sequenced? I don't understand why you need this > here (as opposed to just *not* having a DCHECK_CALLED_ON_VALID_SEQUENCE() call). This object is actually being created on a different sequence than the one it's being used on. This macro indicates that. Otherwise, it associates with the sequence it was created on. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:278: // Be sure to copy out all relevant fields. |job| may be destroyed after we On 2017/06/26 18:42:01, Carlson wrote: > Is this comment still relevant? It's still true but it duplicates the comment on #271. Deleted. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:39: class CupsWrapper { On 2017/06/26 18:42:01, Carlson wrote: > Class comment, esp why this class exists in the first place. Done. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:53: // Access only on query_runner_. On 2017/06/26 18:42:01, Carlson wrote: > It's not obvious here what query_runner_ you're talking about. I think the presence of the sequence checker is sufficient to make the comment redundant. It's gone now. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:57: }; On 2017/06/26 18:42:01, Carlson wrote: > I think this is still unsafe to me, w.r.t destruction. You're using weak ptr to > ensure that we don't *start* any callbacks after the Wrapper is destroyed, but > as I understand it, that mechanism can't be used outside of an entirely > sequenced context -- e.g., the weak ptr guarantee requires that the destructor > be sequenced with respect to the callbacks it's cancelling. That is correct. I'm digging through the documentation to see if I can figure out how this is supposed to be done. AFAICT the task runner would need to be flushed to make this guarantee but I think the intended approach is something else. https://codereview.chromium.org/2943843002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:112: base::WeakPtrFactory<CupsWrapper> cups_wrapper_ptr_factory_; On 2017/06/26 18:42:01, Carlson wrote: > I don't understand why you aren't putting the weak ptr factory into CupsWrapper > directly? I think what you've done here probably works, but it certainly breaks > the normal WeakPtrFactory pattern; I'd be inclined to put the WeakPtrFactory > into the wrapper and expose it as a member function just to preserve the pattern > and make this easier for people to understand. It's SupportsWeakPtr now.
https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:57: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:110: CupsWrapper cups_wrapper_; To ensure this is deleted on its sequence you'll need to use the std::unique_ptr + base::OnTaskRunnerDeleter documented @ https://cs.chromium.org/chromium/src/base/sequenced_task_runner.h?rcl=e7e9085... That way your usage of WeakPtrs will be safe. In fact, you won't need the WeakPtrs at all as the deletion will always run after all tasks already posted (and there won't be any tasks posted after per this instance being gone and |query_runner_| no longer being accessible (other than through SequencedTaskRunnerHandle::Get() on the sequence itself but tasks on |query_runner_| don't post back to self so it's fine).
Thanks! PTAL. https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:57: }; On 2017/06/27 17:44:18, gab wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2943843002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:110: CupsWrapper cups_wrapper_; On 2017/06/27 17:44:18, gab wrote: > To ensure this is deleted on its sequence you'll need to use the std::unique_ptr > + base::OnTaskRunnerDeleter documented @ > https://cs.chromium.org/chromium/src/base/sequenced_task_runner.h?rcl=e7e9085... > > That way your usage of WeakPtrs will be safe. In fact, you won't need the > WeakPtrs at all as the deletion will always run after all tasks already posted > (and there won't be any tasks posted after per this instance being gone and > |query_runner_| no longer being accessible (other than through > SequencedTaskRunnerHandle::Get() on the sequence itself but tasks on > |query_runner_| don't post back to self so it's fine). Thanks! I was looking for that.
https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:258: cups_wrapper_(new CupsWrapper(), base::MakeUnique<CupsWrapper>() https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:41: class CupsWrapper : public base::SupportsWeakPtr<CupsWrapper> { As mentioned in previous paradigm comment: I don't think this even needs to vend WeakPtrs anymore as the deletion will always be the last task in the sequence. https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:115: // Task runner to for the Browser UI thread. rm "to" https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:116: scoped_refptr<base::SingleThreadTaskRunner> browser_thread_runner_; I'd say keep using DCHECK_CURRENTLY_ON(content::BrowserThread::UI) in the .cc and get rid of this member (BrowserThread::UI isn't going away anytime soon). (especially since your documentation requires that it's the "UI thread" not just any "owner thread")
Thanks for the review. It should be correct now. https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:258: cups_wrapper_(new CupsWrapper(), On 2017/06/28 15:55:57, gab (slow Tue-Wed) wrote: > base::MakeUnique<CupsWrapper>() That doesn't work with the custom deleter. AFAICT. https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.h (right): https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:41: class CupsWrapper : public base::SupportsWeakPtr<CupsWrapper> { On 2017/06/28 15:55:58, gab (slow Tue-Wed) wrote: > As mentioned in previous paradigm comment: I don't think this even needs to vend > WeakPtrs anymore as the deletion will always be the last task in the sequence. I misunderstood slightly. It's just unretained now. https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:115: // Task runner to for the Browser UI thread. On 2017/06/28 15:55:57, gab (slow Tue-Wed) wrote: > rm "to" Done. https://codereview.chromium.org/2943843002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.h:116: scoped_refptr<base::SingleThreadTaskRunner> browser_thread_runner_; On 2017/06/28 15:55:57, gab (slow Tue-Wed) wrote: > I'd say keep using DCHECK_CURRENTLY_ON(content::BrowserThread::UI) in the .cc > and get rid of this member (BrowserThread::UI isn't going away anytime soon). > > (especially since your documentation requires that it's the "UI thread" not just > any "owner thread") Done.
The CQ bit was checked by skau@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 skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2943843002/#ps140001 (title: "strong pointers and DCHECKs")
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": 1498755330082030,
"parent_rev": "cd3195be504036951b9caf812bd12ebf6fb15636", "commit_rev":
"bab2208a02c09baa03cb7cffd1bc3f6c996fa04c"}
Message was sent while issue was closed.
Description was changed from ========== Convert to CupsPrintJobManagerImpl to TaskScheduler. Place status polling on a sequenced task runner. Run Cancel as a user visible task. BUG=734280, 667892, 689520 ========== to ========== Convert to CupsPrintJobManagerImpl to TaskScheduler. Place status polling on a sequenced task runner. Run Cancel as a user visible task. BUG=734280, 667892, 689520 Review-Url: https://codereview.chromium.org/2943843002 Cr-Commit-Position: refs/heads/master@{#483394} Committed: https://chromium.googlesource.com/chromium/src/+/bab2208a02c09baa03cb7cffd1bc... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/bab2208a02c09baa03cb7cffd1bc...
Message was sent while issue was closed.
nits on last patch set https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:360: ->PostDelayedTask(FROM_HERE, BrowserThread::PostDelayedTask(BrowserThread::UI, ...) https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:360: ->PostDelayedTask(FROM_HERE, BrowserThread::PostDelayedTask(BrowserThread::UI, ...) https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:379: // Runs a query on query_runner_ which will rejoin this sequnece on |query_runner_|
Message was sent while issue was closed.
Addressed nits in https://codereview.chromium.org/2968063002 https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right): https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:360: ->PostDelayedTask(FROM_HERE, On 2017/06/29 17:12:20, gab (in-out til july17) wrote: > BrowserThread::PostDelayedTask(BrowserThread::UI, ...) Done. https://codereview.chromium.org/2943843002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:379: // Runs a query on query_runner_ which will rejoin this sequnece on On 2017/06/29 17:12:20, gab (in-out til july17) wrote: > |query_runner_| Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
