Combine ChannelIDService::RequestHandle and ChannelIDServiceRequest classes

Combine ChannelIDService::RequestHandle and ChannelIDServiceRequest classes

BUG=486265
Committed: https://crrev.com/75ade89a5463673304e182c1e19f3e9288fb200d
Cr-Commit-Position: refs/heads/master@{#333770}

[nharper, 2015-06-04 23:06:27]

Fix order of variables

[nharper, 2015-06-04 23:09:08]

nharper@chromium.org changed reviewers:
+ mattm@chromium.org

[mattm, 2015-06-06 00:27:15]

https://codereview.chromium.org/1149083013/diff/20001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1149083013/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:214: Cancel();
How does this work? The service will still have a pointer to the Request that is
being deleted won't it?

[nharper, 2015-06-06 00:47:12]

https://codereview.chromium.org/1149083013/diff/20001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1149083013/diff/20001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:214: Cancel();
On 2015/06/06 00:27:14, mattm wrote:
> How does this work?
It doesn't (if the Request object is deleted before the request finishes)

> The service will still have a pointer to the Request that is
> being deleted won't it?
Yes. Before combining the classes here, the ChannelIDServiceRequest was owned by
the ChannelIDServiceJob. I need to figure out how to rework this so that
Request::Cancel() (and Request::~Request()) remove the Request object from the
ChannelIDServiceJob (so that the service no longer has a pointer to the deleted
object).

[nharper, 2015-06-08 19:10:24]

ChannelIDService::Request::Cancel() now deletes ptr to Request from
ChannelIDServiceJob

[nharper, 2015-06-08 19:20:20]

I've updated ChannelIDService::Request to delete the pointer to it in
ChannelIDServiceJob when Request::Cancel() is called.

I still want to look at what happens (or what should happen) when
ChannelIDServiceJob has 0 requests left - it seems like that should maybe cancel
the job.

[mattm, 2015-06-08 23:59:30]

On 2015/06/08 19:20:20, nharper wrote:
> I've updated ChannelIDService::Request to delete the pointer to it in
> ChannelIDServiceJob when Request::Cancel() is called.
> 
> I still want to look at what happens (or what should happen) when
> ChannelIDServiceJob has 0 requests left - it seems like that should maybe
cancel
> the job.

Dunno if cancelling the job is worth the effort. If the posted worker task
already started then there's not much you could do. Even if it hasn't started
yet you'd have to have a thread-safe way of signalling the task to not actually
run.

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (left):

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:235: ~ChannelIDServiceJob() {
Maybe keep destructor and DCHECK that requests_ is empty? (I think that a job
should only get deleted if it has finished, which clears requests_, or if all
requests have been cancelled first, which should remove them all.)

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:190: for
(std::vector<ChannelIDService::Request*>::iterator i =
maybe use std::find?

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:243: const std::string& server_identifier) {
Could just pass in a pointer to the Job and avoid having to do the
server_identifier search when cancelling. (Assuming I'm correct that a job never
gets deleted while it still has non-cancelled requests.)

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:279: if (!callback_.is_null()) {
Should we change this to a DCHECK? It shouldn't be null if Post is getting
called, right?

[nharper, 2015-06-09 19:16:59]

Address comments

[nharper, 2015-06-09 19:19:21]

On 2015/06/08 23:59:30, mattm wrote:
> Dunno if cancelling the job is worth the effort. If the posted worker task
> already started then there's not much you could do. Even if it hasn't started
> yet you'd have to have a thread-safe way of signalling the task to not
actually
> run.
> 

I looked into what the Worker task is doing (all it does is generate the
crypto::ECPrivateKey keypair), and I agree that it would be a lot of work for
little gain to try to cancel the task.

[nharper, 2015-06-09 19:19:32]

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (left):

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:235: ~ChannelIDServiceJob() {
On 2015/06/08 23:59:30, mattm wrote:
> Maybe keep destructor and DCHECK that requests_ is empty? (I think that a job
> should only get deleted if it has finished, which clears requests_, or if all
> requests have been cancelled first, which should remove them all.)

Yes, that seems reasonable. I had to re-read the code to see what is going on,
but I agree with the analysis that requests_ should be empty when the d'tor is
called: ChannelIDServiceJobs are only destroyed in
ChannelIDService::HandleResult, which first calls
ChannelIDServiceJob::HandleResult which clears requests_.

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:190: for
(std::vector<ChannelIDService::Request*>::iterator i =
On 2015/06/08 23:59:30, mattm wrote:
> maybe use std::find?

Done.

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:243: const std::string& server_identifier) {
On 2015/06/08 23:59:30, mattm wrote:
> Could just pass in a pointer to the Job and avoid having to do the
> server_identifier search when cancelling. (Assuming I'm correct that a job
never
> gets deleted while it still has non-cancelled requests.)

I believe the assumption that a job never gets deleted while it still has
non-cancelled requests is correct. The Request object outlives the actual
request (and the Job object that has the request), which made me concerned that
Request::Cancel() would crash trying to access the Job when called in Request's
destructor. However, the appropriate logic is in place in Request::Post to mark
the Request as no longer active, so this isn't actually a problem.

I switched it to using a pointer to the Job so we can skip searching by
server_identifier.

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:279: if (!callback_.is_null()) {
On 2015/06/08 23:59:30, mattm wrote:
> Should we change this to a DCHECK? It shouldn't be null if Post is getting
> called, right?

No, this shouldn't be changed to a DCHECK. It's valid for a null callback to be
set (e.g. only caring for synchronous responses), and there are tests that fail
if a DCHECK is added.

[mattm, 2015-06-09 19:32:24]

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:279: if (!callback_.is_null()) {
On 2015/06/09 19:19:32, nharper wrote:
> On 2015/06/08 23:59:30, mattm wrote:
> > Should we change this to a DCHECK? It shouldn't be null if Post is getting
> > called, right?
> 
> No, this shouldn't be changed to a DCHECK. It's valid for a null callback to
be
> set (e.g. only caring for synchronous responses), and there are tests that
fail
> if a DCHECK is added.

Hm, GetChannelID and GetOrCreateChannelID both have an "if (callback.is_null()
|| ...)" test that errors out immediately. If it does fail with that dcheck it
must be for a different reason.

[mattm, 2015-06-09 20:06:39]

https://codereview.chromium.org/1149083013/diff/60001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1149083013/diff/60001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:477: void ChannelIDService::CancelRequest(Request*
req) {
Is this still used?

[nharper, 2015-06-09 21:30:13]

Remove unused ChannelIDService::CancelRequest; add DCHECK to
ChannelIDService::Request::Post

[nharper, 2015-06-09 21:30:16]

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1149083013/diff/40001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:279: if (!callback_.is_null()) {
On 2015/06/09 19:32:24, mattm wrote:
> On 2015/06/09 19:19:32, nharper wrote:
> > On 2015/06/08 23:59:30, mattm wrote:
> > > Should we change this to a DCHECK? It shouldn't be null if Post is getting
> > > called, right?
> > 
> > No, this shouldn't be changed to a DCHECK. It's valid for a null callback to
> be
> > set (e.g. only caring for synchronous responses), and there are tests that
> fail
> > if a DCHECK is added.
> 
> Hm, GetChannelID and GetOrCreateChannelID both have an "if (callback.is_null()
> || ...)" test that errors out immediately. If it does fail with that dcheck it
> must be for a different reason.

The DCHECK failed because I got the condition backwards.

Since I'm adding a DCHECK, I'm also removing the if statement, as the style
guide says not to handle DCHECK failures.

https://codereview.chromium.org/1149083013/diff/60001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service.cc (right):

https://codereview.chromium.org/1149083013/diff/60001/net/ssl/channel_id_serv...
net/ssl/channel_id_service.cc:477: void ChannelIDService::CancelRequest(Request*
req) {
On 2015/06/09 20:06:39, mattm wrote:
> Is this still used?

No. Removed.

[mattm, 2015-06-09 23:08:36]

https://codereview.chromium.org/1149083013/diff/80001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1149083013/diff/80001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:372: TEST_F(ChannelIDServiceTest,
DestructionOfChannelIDServiceRequest) {
Is this test different than
ChannelIDServiceTest.CancelRequestByHandleDestruction?

[nharper, 2015-06-09 23:43:49]

Remove new test case and combine it with existing one

[nharper, 2015-06-09 23:45:03]

https://codereview.chromium.org/1149083013/diff/80001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1149083013/diff/80001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:372: TEST_F(ChannelIDServiceTest,
DestructionOfChannelIDServiceRequest) {
On 2015/06/09 23:08:35, mattm wrote:
> Is this test different than
> ChannelIDServiceTest.CancelRequestByHandleDestruction?

It looks like these tests should do the same thing, but came from different
intentions. That test (CancelRequestByHandleDestruction) appears to be testing
that once the Request is Cancel()ed, its callback isn't called, whereas this one
tests that a Request object isn't read from/written to after it is destroyed.
That test also doesn't fail the way this test does, because its Request object
is stack allocated, so reading/writing to it doesn't cause a crash or asan/lsan
failure like in this test where the Request is on the heap.

Even though there are these differences, they're so similar that they can be
combined by changing the stack-allocated Request in that test to be heap
allocated, and getting rid of this test.

[mattm, 2015-06-10 00:14:24]

lgtm

https://codereview.chromium.org/1149083013/diff/80001/net/ssl/channel_id_serv...
File net/ssl/channel_id_service_unittest.cc (right):

https://codereview.chromium.org/1149083013/diff/80001/net/ssl/channel_id_serv...
net/ssl/channel_id_service_unittest.cc:372: TEST_F(ChannelIDServiceTest,
DestructionOfChannelIDServiceRequest) {
On 2015/06/09 23:45:03, nharper wrote:
> On 2015/06/09 23:08:35, mattm wrote:
> > Is this test different than
> > ChannelIDServiceTest.CancelRequestByHandleDestruction?
> 
> It looks like these tests should do the same thing, but came from different
> intentions. That test (CancelRequestByHandleDestruction) appears to be testing
> that once the Request is Cancel()ed, its callback isn't called, whereas this
one
> tests that a Request object isn't read from/written to after it is destroyed.
> That test also doesn't fail the way this test does, because its Request object
> is stack allocated, so reading/writing to it doesn't cause a crash or
asan/lsan
> failure like in this test where the Request is on the heap.

Oh, that's a good point. I wasn't thinking about that difference between stack
and heap allocated objects.

> Even though there are these differences, they're so similar that they can be
> combined by changing the stack-allocated Request in that test to be heap
> allocated, and getting rid of this test.

sgtm

[nharper, 2015-06-10 00:30:23]

nharper@chromium.org changed reviewers:
+ juanlang@chromium.org, kalman@chromium.org

[nharper, 2015-06-10 00:30:24]

+kalman for //chrome/browser/extensions

[juanlang (chromium.org), 2015-06-10 03:15:48]

I'm happy to lgtm for form's sake, but mattm@'s and kalman@'s are worth more
than mine here.

[not at google - send to devlin, 2015-06-10 18:27:24]

extensions lgtm (rubber stamp)

[nharper, 2015-06-10 18:27:48]

The CQ bit was checked by nharper@chromium.org

[commit-bot: I haz the power, 2015-06-10 18:27:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083013/100001

[commit-bot: I haz the power, 2015-06-10 19:05:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-10 19:06:42]

Patchset 6 (id:??) landed as
https://crrev.com/75ade89a5463673304e182c1e19f3e9288fb200d
Cr-Commit-Position: refs/heads/master@{#333770}