ResourceRequestAllowedNotifier didn't work as expected

ResourceRequestAllowedNotifier didn't work as expected

Following problems will be fixed by this change.
1) OnResourceRequestsAllowed() was invoked even if network was offline
2) ResourceRequestsAllowed() returned true even though network was offline
3) OnResourceRequestsAllowed() was not invoked even if network went online
See also http://crbug.com/249607 for each scenario in details.

BUG=249607
TEST=unit_tests --gtest_filter='ResourceRequestAllowedNotifierTest.*'

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207199

[Takashi Toyoshima, 2013-06-14 08:27:29]

Hi Alexei,

Can you take a look this split CL to fix ResourceRequestAllowedNotifier bugs?
I added comments why I modified these tests and which test can reproduce each
crbug problem case.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
File chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
(left):

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:155:
SimulateResourceRequest();
In this order, ResourceRequest always return true internally, and never
notified. We should change network and EULA conditions before calling it.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
File chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
(right):

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:169:
TEST_F(ResourceRequestAllowedNotifierTest, NotifyOnReconnect) {
This test is problem 2) of crbug.
But doesn't work correctly because SetWaitingForNetwork is called after
SimulateResourceRequest().
Revised test doesn't work without my patch.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:199:
EXPECT_FALSE(was_notified());
Network goes online, but not notified because SimulateResourceRequest() returns
true before.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:202:
EXPECT_FALSE(SimulateResourceRequest());
Now, SimulateResourceRequest() returns false and will be notified later.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:206:
EXPECT_TRUE(was_notified());
Yep, this time, it must be notified.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:209:
TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnEulaAfterGoOffline) {
This test is problem 1) of crbug.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:241:
TEST_F(ResourceRequestAllowedNotifierTest, EulaFirst) {
This test is problem 3) of crbug.
Also doesn't work without my patch.

[Alexei Svitkine (slow), 2013-06-14 16:47:59]

Change looks good, here are some comments.

Again, thanks a lot fo noticing the issues and fixing them!

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
File chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
(right):

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:106:
// Simulate a resource request from the test service.
Nit: Update comment to explain return value.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:194:
TEST_F(ResourceRequestAllowedNotifierTest, NotifyOnFlakyConnection) {
This test makes sense.

I think it's different from the old test, which we should also have.

The old test was testing the case where we get true from
SimulateResourceRequest() first, so that we're not expecting a notification. And
then the connection toggles back and forth (without another
SimulateResourceRequest() in between), and this toggling doesn't generate any
notifications (since we weren't waiting for any). Can you re-add such a test?

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:199:
EXPECT_FALSE(was_notified());
On 2013/06/14 08:27:29, Takashi Toyoshima (chromium) wrote:
> Network goes online, but not notified because SimulateResourceRequest()
returns
> true before.

Add it as a comment in the test.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:202:
EXPECT_FALSE(SimulateResourceRequest());
On 2013/06/14 08:27:29, Takashi Toyoshima (chromium) wrote:
> Now, SimulateResourceRequest() returns false and will be notified later.

Add it as a comment in the test.

[Takashi Toyoshima, 2013-06-17 07:04:40]

Thanks.
I fixed my change.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
File chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
(right):

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:106:
// Simulate a resource request from the test service.
On 2013/06/14 16:47:59, Alexei Svitkine wrote:
> Nit: Update comment to explain return value.

Done.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:194:
TEST_F(ResourceRequestAllowedNotifierTest, NotifyOnFlakyConnection) {
Oh, I see.
I'll leave original one separately.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:199:
EXPECT_FALSE(was_notified());
On 2013/06/14 16:47:59, Alexei Svitkine wrote:
> On 2013/06/14 08:27:29, Takashi Toyoshima (chromium) wrote:
> > Network goes online, but not notified because SimulateResourceRequest()
> returns
> > true before.
> 
> Add it as a comment in the test.

Done.

https://codereview.chromium.org/16841020/diff/5001/chrome/browser/web_resourc...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:202:
EXPECT_FALSE(SimulateResourceRequest());
On 2013/06/14 16:47:59, Alexei Svitkine wrote:
> On 2013/06/14 08:27:29, Takashi Toyoshima (chromium) wrote:
> > Now, SimulateResourceRequest() returns false and will be notified later.
> 
> Add it as a comment in the test.

Done.

[Alexei Svitkine (slow), 2013-06-17 13:41:41]

lgtm, thanks

[commit-bot: I haz the power, 2013-06-17 14:51:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/16841020/10001

[commit-bot: I haz the power, 2013-06-17 15:24:42]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-06-17 15:26:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/16841020/10001

[Alexei Svitkine (slow), 2013-06-17 15:31:21]

On 2013/06/17 15:26:55, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/toyoshim%40chromium.org/16841020/10001

Seems there's test failure as a result of this:

VariationsServiceTest.RequestsInitiallyNotAllowed: 
../../chrome/browser/metrics/variations/variations_service_unittest.cc:674:
Failure
Value of: test_service.fetch_attempted()
Actual: false
Expected: true

[commit-bot: I haz the power, 2013-06-17 16:05:21]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Takashi Toyoshima, 2013-06-17 17:02:17]

Oh, it seems like that we need to fix TestRequestAllowedNotifier, too.
I upload fixed one. Can you take a look?

[Alexei Svitkine (slow), 2013-06-17 17:36:13]

https://codereview.chromium.org/16841020/diff/52001/chrome/browser/web_resour...
File chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc
(right):

https://codereview.chromium.org/16841020/diff/52001/chrome/browser/web_resour...
chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc:34:
SetWaitingForNetworkForTesting(false);
So I think ResourceRequestsAllowed() should already be returning true due to
override_requests_allowed_ here.

So I'm guessing the only reason you're calling the base class version is to make
sure observer_requested_permission_ gets set (because with your change below, it
won't get called anymore).

If you revert the change below, then this change won't be necessary.
Alternatively, make a *ForTesting method to set override_requests_allowed_.

https://codereview.chromium.org/16841020/diff/52001/chrome/browser/web_resour...
chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc:41:
// the override flag was set.
With this change, this comment becomes incorrect.

I'm not sure it makes sense to change the semantics of this. given that current
callers may be relying on the underlying one being set?

Could you explain why this is necessary? What happens that causes the test to
fail?

[Takashi Toyoshima, 2013-06-18 07:21:05]

https://codereview.chromium.org/16841020/diff/52001/chrome/browser/web_resour...
File chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc
(right):

https://codereview.chromium.org/16841020/diff/52001/chrome/browser/web_resour...
chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc:34:
SetWaitingForNetworkForTesting(false);
OK, I'll add ForTesting method.

https://codereview.chromium.org/16841020/diff/52001/chrome/browser/web_resour...
chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc:41:
// the override flag was set.
I fixed this function because it had two problems.

1) Even if it calls ResourceRequestsAllowed(), MaybeNotifyObserver() doesn't
notify if the ResourceRequestsAllowed() returns true here.
This is the failed test scenario and was hidden by the bug I fixed in this
change.

2) When both override_requests_allowed_ and  requests_allowed_ are set to true,
and ResourceRequestAllowedNotifier::ResourceRequestsAllowed() returns false
here, it invokes the callback later even though this function returns true. This
was inconsistent and a potential bug.

[Alexei Svitkine (slow), 2013-06-18 15:35:04]

lgtm with a nit

https://codereview.chromium.org/16841020/diff/63001/chrome/browser/web_resour...
File chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc
(right):

https://codereview.chromium.org/16841020/diff/63001/chrome/browser/web_resour...
chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc:40:
// result.
Nit: This comment no longer describes something tricky, just the code below it.
That's redundant, just remove it then.

[Takashi Toyoshima, 2013-06-19 04:35:39]

Agreed.
Thanks!

https://codereview.chromium.org/16841020/diff/63001/chrome/browser/web_resour...
File chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc
(right):

https://codereview.chromium.org/16841020/diff/63001/chrome/browser/web_resour...
chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc:40:
// result.
On 2013/06/18 15:35:04, Alexei Svitkine wrote:
> Nit: This comment no longer describes something tricky, just the code below
it.
> That's redundant, just remove it then.

Done.

[commit-bot: I haz the power, 2013-06-19 04:47:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/16841020/75001

[commit-bot: I haz the power, 2013-06-19 07:16:34]

Change committed as 207199