Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
50093002:
Added a timeout for platform verification key generation. (Closed)
|
Created:
7 years, 1 month ago by Darren Krahn Modified:
7 years, 1 month ago CC:
chromium-reviews, nkostylev+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, dkrahn+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded a timeout for platform verification key generation.
The first time platform verification is invoked for a particular site,
at least one key generation must be performed by the TPM. This
typically takes around two seconds but on some models can take over a
minute. Adding a timeout allows for a better user experience.
BUG=chromium:309169
TEST=unit, manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232637
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 9
Patch Set 3 : use base::Timer #
Total comments: 1
Patch Set 4 : rebased #Patch Set 5 : rebased #Patch Set 6 : fix clang error #Patch Set 7 : fixed browsertest #
Messages
Total messages: 25 (0 generated)
pastarmovj - owner of attestation dmichael - owner of pepper
https://chromiumcodereview.appspot.com/50093002/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://chromiumcodereview.appspot.com/50093002/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/attestation/platform_verification_flow.h:47: : public base::RefCountedThreadSafe<PlatformVerificationFlow> { From looking at this CL it is not obvious to me why do you need this class to be RefCountedThreadSafe please put this explnation in the comment above this class since this is a rather important detail IMO.
https://codereview.chromium.org/50093002/diff/1/chrome/browser/chromeos/attes... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/50093002/diff/1/chrome/browser/chromeos/attes... chrome/browser/chromeos/attestation/platform_verification_flow.cc:131: base::Bind(&PlatformVerificationFlow::CheckConsent, this, context), Can you just use WeakPtrFactory instead for posting tasks to |this|? And leave the owning Pepper host using scoped_ptr?
https://codereview.chromium.org/50093002/diff/1/chrome/browser/chromeos/attes... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/50093002/diff/1/chrome/browser/chromeos/attes... chrome/browser/chromeos/attestation/platform_verification_flow.cc:131: base::Bind(&PlatformVerificationFlow::CheckConsent, this, context), On 2013/10/29 16:27:13, dmichael wrote: > Can you just use WeakPtrFactory instead for posting tasks to |this|? And leave > the owning Pepper host using scoped_ptr? Would love to (that's how it was) but I need the instance to outlive the call-site. I've added an explanation to the class description. Another option would be a singleton but I like that even less. https://codereview.chromium.org/50093002/diff/1/chrome/browser/chromeos/attes... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/50093002/diff/1/chrome/browser/chromeos/attes... chrome/browser/chromeos/attestation/platform_verification_flow.h:47: : public base::RefCountedThreadSafe<PlatformVerificationFlow> { On 2013/10/29 09:24:29, pastarmovj wrote: > From looking at this CL it is not obvious to me why do you need this class to be > RefCountedThreadSafe please put this explnation in the comment above this class > since this is a rather important detail IMO. Done. It is important -- please let me know if have a better way to solve this.
lgtm. https://codereview.chromium.org/50093002/diff/1/chrome/browser/chromeos/attes... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/50093002/diff/1/chrome/browser/chromeos/attes... chrome/browser/chromeos/attestation/platform_verification_flow.h:47: : public base::RefCountedThreadSafe<PlatformVerificationFlow> { On 2013/10/29 17:23:04, Darren Krahn wrote: > On 2013/10/29 09:24:29, pastarmovj wrote: > > From looking at this CL it is not obvious to me why do you need this class to > be > > RefCountedThreadSafe please put this explnation in the comment above this > class > > since this is a rather important detail IMO. > > Done. It is important -- please let me know if have a better way to solve this. Thanks this looks reasonable to me. I think your choice to use ref-counted is reasonable. Especially the point that having this a singleton is not a good choice because it isn't actually needed all the time so it doesn't have to stay around all the time and pollute the global namespace/memory.
https://chromiumcodereview.appspot.com/50093002/diff/80001/chrome/browser/chr... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://chromiumcodereview.appspot.com/50093002/diff/80001/chrome/browser/chr... chrome/browser/chromeos/attestation/platform_verification_flow.h:158: struct TimeoutStatus : public base::RefCountedThreadSafe<TimeoutStatus> { Why all this instead of base::DelayTimer from base/timer/timer.h? It's self invalidating so you don't need any additional RefCounted or WeakPtr business all over.
pepper stuff lgtm
https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... chrome/browser/chromeos/attestation/platform_verification_flow.h:158: struct TimeoutStatus : public base::RefCountedThreadSafe<TimeoutStatus> { On 2013/10/30 18:06:00, DaleCurtis wrote: > Why all this instead of base::DelayTimer from base/timer/timer.h? It's self > invalidating so you don't need any additional RefCounted or WeakPtr business all > over. AFAICT, DelayTimer offers no real benefit over PostDelayedTask which I'm using. This ref-counted status exists because the class is designed to support multiple ChallengePlatformKey calls in flight simultaneously so I want status tracked with the call, not global to the class instance. When either of the callbacks (timer/actual) return, they need to be aware of whether the other callback has already run, so they both hold a ref to the status. Maybe I'm missing something?
https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... chrome/browser/chromeos/attestation/platform_verification_flow.cc:206: scoped_refptr<TimeoutStatus> timeout_status(new TimeoutStatus); TimeoutStatus() https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... chrome/browser/chromeos/attestation/platform_verification_flow.cc:229: TimeoutStatus* timeout_status, Usage of raw pointers with scoped_refptr seems questionable. scoped_refptr<> instead? https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... chrome/browser/chromeos/attestation/platform_verification_flow.h:158: struct TimeoutStatus : public base::RefCountedThreadSafe<TimeoutStatus> { On 2013/10/30 19:14:30, Darren Krahn wrote: > On 2013/10/30 18:06:00, DaleCurtis wrote: > > Why all this instead of base::DelayTimer from base/timer/timer.h? It's self > > invalidating so you don't need any additional RefCounted or WeakPtr business > all > > over. > > AFAICT, DelayTimer offers no real benefit over PostDelayedTask which I'm using. > This ref-counted status exists because the class is designed to support multiple > ChallengePlatformKey calls in flight simultaneously so I want status tracked > with the call, not global to the class instance. When either of the callbacks > (timer/actual) return, they need to be aware of whether the other callback has > already run, so they both hold a ref to the status. > > Maybe I'm missing something? Ah I forgot there can be multiple calls outstanding. I still think it'd be cleaner in terms of less code and better tested (timer already has extensive tests) than manually managing this TimeoutStatus structure. Up to you though. Also, err, sorry, I meant OneShotTimer. If you instead make ChallengeContext a ref counted structure (which would save on copies anyways), you can stick the OneShotTimer in there. Then you can use Timer::IsRunning() to determine if it has fired or not and Timer::Stop() to prevent it from firing. https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... chrome/browser/chromeos/attestation/platform_verification_flow.h:284: int timeout_delay_; base::TimeDelta
https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... chrome/browser/chromeos/attestation/platform_verification_flow.cc:229: TimeoutStatus* timeout_status, On 2013/10/30 20:11:52, DaleCurtis wrote: > Usage of raw pointers with scoped_refptr seems questionable. scoped_refptr<> > instead? Now using scoped_ptr but this can be done with scoped_refptr to save the extra AddRef/Release. See http://www.chromium.org/developers/design-documents/threading https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... chrome/browser/chromeos/attestation/platform_verification_flow.h:158: struct TimeoutStatus : public base::RefCountedThreadSafe<TimeoutStatus> { On 2013/10/30 20:11:52, DaleCurtis wrote: > On 2013/10/30 19:14:30, Darren Krahn wrote: > > On 2013/10/30 18:06:00, DaleCurtis wrote: > > > Why all this instead of base::DelayTimer from base/timer/timer.h? It's self > > > invalidating so you don't need any additional RefCounted or WeakPtr business > > all > > > over. > > > > AFAICT, DelayTimer offers no real benefit over PostDelayedTask which I'm > using. > > This ref-counted status exists because the class is designed to support > multiple > > ChallengePlatformKey calls in flight simultaneously so I want status tracked > > with the call, not global to the class instance. When either of the callbacks > > (timer/actual) return, they need to be aware of whether the other callback has > > already run, so they both hold a ref to the status. > > > > Maybe I'm missing something? > > Ah I forgot there can be multiple calls outstanding. I still think it'd be > cleaner in terms of less code and better tested (timer already has extensive > tests) than manually managing this TimeoutStatus structure. Up to you though. > > Also, err, sorry, I meant OneShotTimer. If you instead make ChallengeContext a > ref counted structure (which would save on copies anyways), you can stick the > OneShotTimer in there. Then you can use Timer::IsRunning() to determine if it > has fired or not and Timer::Stop() to prevent it from firing. Good suggestion -- updated to use base::Timer and base::Passed. https://codereview.chromium.org/50093002/diff/80001/chrome/browser/chromeos/a... chrome/browser/chromeos/attestation/platform_verification_flow.h:284: int timeout_delay_; On 2013/10/30 20:11:52, DaleCurtis wrote: > base::TimeDelta Done.
lgtm https://codereview.chromium.org/50093002/diff/200001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/50093002/diff/200001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_flow.cc:208: scoped_ptr<base::Timer> timer(new base::Timer(false, // Don't retain. Timer(false, false) is just OneShotTimer(). Why the generic instance?
On 2013/10/30 22:53:17, DaleCurtis wrote: > lgtm > > https://codereview.chromium.org/50093002/diff/200001/chrome/browser/chromeos/... > File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): > > https://codereview.chromium.org/50093002/diff/200001/chrome/browser/chromeos/... > chrome/browser/chromeos/attestation/platform_verification_flow.cc:208: > scoped_ptr<base::Timer> timer(new base::Timer(false, // Don't retain. > Timer(false, false) is just OneShotTimer(). Why the generic instance? OneShotTimer is a template designed to be used as a class member and it hides the base::Bind for you. I want to pass a base::Closure directly and "scoped_ptr<base::OneShotTimer<PlatformVerificationFlow> >" seems a bit messy vs "scoped_ptr<base::Timer>".
On 2013/10/30 23:08:01, Darren Krahn wrote: > On 2013/10/30 22:53:17, DaleCurtis wrote: > > lgtm > > > > > https://codereview.chromium.org/50093002/diff/200001/chrome/browser/chromeos/... > > File chrome/browser/chromeos/attestation/platform_verification_flow.cc > (right): > > > > > https://codereview.chromium.org/50093002/diff/200001/chrome/browser/chromeos/... > > chrome/browser/chromeos/attestation/platform_verification_flow.cc:208: > > scoped_ptr<base::Timer> timer(new base::Timer(false, // Don't retain. > > Timer(false, false) is just OneShotTimer(). Why the generic instance? > > OneShotTimer is a template designed to be used as a class member and it hides > the base::Bind for you. I want to pass a base::Closure directly and > "scoped_ptr<base::OneShotTimer<PlatformVerificationFlow> >" seems a bit messy vs > "scoped_ptr<base::Timer>". Note: this CL will conflict with https://codereview.chromium.org/31043008/ so waiting for that one to land first.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@chromium.org/50093002/300001
Step "update" is always a major failure. Look at the try server FAQ for more details. 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/dkrahn@chromium.org/50093002/300001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@chromium.org/50093002/570001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@chromium.org/50093002/760001
Retried try job too often on win_rel for step(s) content_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@chromium.org/50093002/760001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@chromium.org/50093002/760001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dkrahn@chromium.org/50093002/760001
Message was sent while issue was closed.
Change committed as 232637 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
