Third-party appcache blocking.

Third-party appcache blocking.

BUG=72586
TEST=AppCacheHostTest.SelectCacheAllowed, AppCacheHostTest.SelectCacheBlocked, AppCacheRequestHandlerTest.MainResource_Blocked


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100131

[michaeln, 2011-08-31 23:42:08]

http://codereview.chromium.org/7720022/diff/16001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/7720022/diff/16001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/offline_resource_handler.cc:115: url, GURL(),
appcache_completion_callback_);
is it possible to get the actual first_party_url value from the request?

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:98: (manifest_url.GetOrigin() ==
document_url.GetOrigin())) {
maybe DCHECK(!first_party_url_.empty());

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:101: !policy->CanCreateAppCache(manifest_url,
first_party_url_)) {
We need to call FinishCacheSelection(NULL,NULL) in this clause.

Also, i think we'll want to synthesize the correct sequence of appcache events
and message at this point. These are things the update job used to do.

frontend_->OnEventRaised(CHECKING);
frontend_->OnErrorEventRaised("Cache creation was blocked by the content
policy");
frontend_->OnContentBlocked(manifest_url);
return;

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:272: first_party_url_ =
request->first_party_for_cookies();
Can this be moved down into the if (IsMainResourceType()) block? Where it is, we
reset the value for every subresource request but we only need the value for the
main resource request.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:436: void
AppCacheHost::OnContentBlocked(AppCacheGroup* group) {
i think this method can be removed now that the update job doesn't call it
anymore

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:135: const GURL& first_party_url() { return
first_party_url_; }
method could be const too

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:155: virtual void
OnContentBlocked(AppCacheGroup* group);
Looks like this OnContentBlocked() observer method and the related
Group::NotifyContentBlocked() are no longer needed.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:240:
host_->LoadMainResourceCache(cache_id);
we shouldn't do these things if 'blocked'

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:251: found_entry_ = entry;
if was_blocked, these data members cannot be set to these values... doing so
will cause the 'blocked' resource to be used

not sure, you may want to make an early exit in the blocked case which calls
DeliverNetworkResponse() prior to exit

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_up...
File webkit/appcache/appcache_update_job_unittest.cc (left):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_up...
webkit/appcache/appcache_update_job_unittest.cc:3338:
TEST_F(AppCacheUpdateJobTest, ImmediatelyBlockCacheAttemptTest) {
Since these 'blocking' functions are now being performed by the AppCacheHost
class, can you add unittest for these functions in that class.

[marja, 2011-09-02 13:40:23]

Thanks for the comments. The smaller ones are done (I added some open questions
to the comments, though), and I'll continue with adding an AppCacheHost test and
getting the first_party_url in the OfflineResourceHandler.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:98: (manifest_url.GetOrigin() ==
document_url.GetOrigin())) {
On 2011/08/31 23:42:08, michaeln wrote:
> maybe DCHECK(!first_party_url_.empty());

Done. For this, I also needed to add a call to CreateRequestHandler in
MasterEntryFailStoreNewestCacheTest.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:101: !policy->CanCreateAppCache(manifest_url,
first_party_url_)) {
On 2011/08/31 23:42:08, michaeln wrote:
> We need to call FinishCacheSelection(NULL,NULL) in this clause.
> 
> Also, i think we'll want to synthesize the correct sequence of appcache events
> and message at this point. These are things the update job used to do.
> 
> frontend_->OnEventRaised(CHECKING);
> frontend_->OnErrorEventRaised("Cache creation was blocked by the content
> policy");
> frontend_->OnContentBlocked(manifest_url);
> return;

Done.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:272: first_party_url_ =
request->first_party_for_cookies();
On 2011/08/31 23:42:08, michaeln wrote:
> Can this be moved down into the if (IsMainResourceType()) block? Where it is,
we
> reset the value for every subresource request but we only need the value for
the
> main resource request.

Done.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:436: void
AppCacheHost::OnContentBlocked(AppCacheGroup* group) {
On 2011/08/31 23:42:08, michaeln wrote:
> i think this method can be removed now that the update job doesn't call it
> anymore

Done.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.h (right):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:135: const GURL& first_party_url() { return
first_party_url_; }
On 2011/08/31 23:42:08, michaeln wrote:
> method could be const too

Done.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.h:155: virtual void
OnContentBlocked(AppCacheGroup* group);
On 2011/08/31 23:42:08, michaeln wrote:
> Looks like this OnContentBlocked() observer method and the related
> Group::NotifyContentBlocked() are no longer needed.

Done.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:240:
host_->LoadMainResourceCache(cache_id);
On 2011/08/31 23:42:08, michaeln wrote:
> we shouldn't do these things if 'blocked'

Hmm, I don't understand. They were done when the was_blocked_by_policy was
passed to this func as a param, why shouldn't they be done now?

I changed this to "else if" though.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:251: found_entry_ = entry;
On 2011/08/31 23:42:08, michaeln wrote:
> if was_blocked, these data members cannot be set to these values... doing so
> will cause the 'blocked' resource to be used
> 
> not sure, you may want to make an early exit in the blocked case which calls
> DeliverNetworkResponse() prior to exit

Done.

[michaeln, 2011-09-02 18:47:47]

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:229:
!policy->CanLoadAppCache(manifest_url, host_->first_party_url());
in most cases (the "nothing was found" case), manifest_url will be empty

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:240:
host_->LoadMainResourceCache(cache_id);
On 2011/09/02 13:40:24, marja wrote:
> On 2011/08/31 23:42:08, michaeln wrote:
> > we shouldn't do these things if 'blocked'
> 
> Hmm, I don't understand. They were done when the was_blocked_by_policy was
> passed to this func as a param, why shouldn't they be done now?

Here's the current callsite for OnMainResponseFound in the blocked case, notice
the values being passed in.

  if (!manifest_url.is_empty()) {
    // Check the policy prior to returning a main resource from the appcache.
    AppCachePolicy* policy = service()->appcache_policy();
    if (policy && !policy->CanLoadAppCache(manifest_url)) {
      FOR_EACH_DELEGATE(
          (*delegates),
          OnMainResponseFound(url, AppCacheEntry(),
                              GURL(), AppCacheEntry(),
                              kNoCacheId, manifest_url, true));
      return;
    }
  }

[michaeln, 2011-09-02 19:27:20]

On 2011/09/02 13:40:23, marja wrote:
> Thanks for the comments. The smaller ones are done (I added some open
questions
> to the comments, though), and I'll continue with adding an AppCacheHost test
and
> getting the first_party_url in the OfflineResourceHandler.

I guess the unittests for CanLoad should go in
appcache_request_handler_unittest.cc since that's the class that contains that
logic. And the tests for CanCreate should go in appcache_host_unittest.cc.

> > maybe DCHECK(!first_party_url_.empty());
> 
> Done. For this, I also needed to add a call to CreateRequestHandler in
> MasterEntryFailStoreNewestCacheTest.

For the unittest, you could just set the first_party_url_ value directly
(AppCacheUpdateJobTest is a friend of AppCacheHost) instead of indirectly
setting that up via the call to CreateRequestHandler.

[michaeln, 2011-09-02 19:32:04]

http://codereview.chromium.org/7720022/diff/24001/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/7720022/diff/24001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:105: host_ids.push_back(host_id_);
there's a ctor for this single element vector case
std::vector<int> ids(1, host_id_);

[marja, 2011-09-05 10:42:24]

Thanks again for the comments!

http://codereview.chromium.org/7720022/diff/16001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/7720022/diff/16001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/offline_resource_handler.cc:115: url, GURL(),
appcache_completion_callback_);
On 2011/08/31 23:42:08, michaeln wrote:
> is it possible to get the actual first_party_url value from the request?

Done.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:229:
!policy->CanLoadAppCache(manifest_url, host_->first_party_url());
On 2011/09/02 18:47:47, michaeln wrote:
> in most cases (the "nothing was found" case), manifest_url will be empty

Ok, in the current code in
AppCacheStorageImpl::CheckPolicyAndCallOnMainResponseFound, when manifest_url is
empty, the policy check is not done and OnMainResponseFound is called with
non-nullified values. So here I added !manifest_url.is_empty() to the policy
check.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:240:
host_->LoadMainResourceCache(cache_id);
On 2011/09/02 18:47:47, michaeln wrote:
> On 2011/09/02 13:40:24, marja wrote:
> > On 2011/08/31 23:42:08, michaeln wrote:
> > > we shouldn't do these things if 'blocked'
> > 
> > Hmm, I don't understand. They were done when the was_blocked_by_policy was
> > passed to this func as a param, why shouldn't they be done now?
> 
> Here's the current callsite for OnMainResponseFound in the blocked case,
notice
> the values being passed in.
> 
>   if (!manifest_url.is_empty()) {
>     // Check the policy prior to returning a main resource from the appcache.
>     AppCachePolicy* policy = service()->appcache_policy();
>     if (policy && !policy->CanLoadAppCache(manifest_url)) {
>       FOR_EACH_DELEGATE(
>           (*delegates),
>           OnMainResponseFound(url, AppCacheEntry(),
>                               GURL(), AppCacheEntry(),
>                               kNoCacheId, manifest_url, true));
>       return;
>     }
>   }

Ah, of course, thanks.

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_up...
File webkit/appcache/appcache_update_job_unittest.cc (left):

http://codereview.chromium.org/7720022/diff/16001/webkit/appcache/appcache_up...
webkit/appcache/appcache_update_job_unittest.cc:3338:
TEST_F(AppCacheUpdateJobTest, ImmediatelyBlockCacheAttemptTest) {
On 2011/08/31 23:42:08, michaeln wrote:
> Since these 'blocking' functions are now being performed by the AppCacheHost
> class, can you add unittest for these functions in that class.

Done. (Added tests for AppCacheHost and AppCacheRequestHandler.)

http://codereview.chromium.org/7720022/diff/24001/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host.cc (right):

http://codereview.chromium.org/7720022/diff/24001/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host.cc:105: host_ids.push_back(host_id_);
On 2011/09/02 19:32:04, michaeln wrote:
> there's a ctor for this single element vector case
> std::vector<int> ids(1, host_id_);

Done.

[michaeln, 2011-09-05 19:49:31]

thnx for adding the tests... looks really nice and very close to being wrapped
up... i just have a few comments

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host_unittest.cc (right):

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host_unittest.cc:464: EXPECT_EQ(OBSOLETE_EVENT,
mock_frontend_.last_event_id_);
does it make sense to also verify that frontend.OnContentBlocked has not been
called in this case

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host_unittest.cc:500: EXPECT_EQ(ERROR_EVENT,
mock_frontend_.last_event_id_);
similarly verify that frontend.OnContentBlocked has been called in this case

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:230: 
it may be more readable to hoist all of this was_blocked case handling into a
single block with an early return instead of interspersing the logic with the
usual case handling...

if (was_blocked_by_policy) {
  if (ResourceType::IsFrame(resource_type_))
    host_->NotifyMainResourceBlocked(manifest_url);
  else
    host_->frontend()->OnContentBlocked(host_->host_id(), manifest_url);
  DeliverNetworkResponse();
  return;
}

if (ResourceType::IsFrame(resource_type_) && cache_id != kNoCacheId) {
  // AppCacheHost loads and holds a reference to ...
  host_->LoadMainResourceCache(cache_id);
  host_->set_preferred_manifest_url(manifest_url);
}

wdyt?

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:232: if (was_blocked_by_policy)
style: since the else clause needs {'s, this if clause should also be  bracketed

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler_unittest.cc (right):

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler_unittest.cc:775: 
does it make sense to also verify these additional host data members?

host_->main_resource_blocked_;
host_->blocked_manifest_url_;

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service.cc (right):

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_se...
webkit/appcache/appcache_service.cc:88: CallCallback(net::ERR_FAILED);
i think you need to 'delete this' here to avoid leaking the helper object

[marja, 2011-09-06 09:09:28]

Hi,

Here are some more modifications. I had indeed totally missed the deletion of
CanHandleOfflineHelper.

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_ho...
File webkit/appcache/appcache_host_unittest.cc (right):

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host_unittest.cc:464: EXPECT_EQ(OBSOLETE_EVENT,
mock_frontend_.last_event_id_);
On 2011/09/05 19:49:31, michaeln wrote:
> does it make sense to also verify that frontend.OnContentBlocked has not been
> called in this case

Done.

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_ho...
webkit/appcache/appcache_host_unittest.cc:500: EXPECT_EQ(ERROR_EVENT,
mock_frontend_.last_event_id_);
On 2011/09/05 19:49:31, michaeln wrote:
> similarly verify that frontend.OnContentBlocked has been called in this case

Done.

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler.cc (right):

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:230: 
On 2011/09/05 19:49:31, michaeln wrote:
> it may be more readable to hoist all of this was_blocked case handling into a
> single block with an early return instead of interspersing the logic with the
> usual case handling...
> 
> if (was_blocked_by_policy) {
>   if (ResourceType::IsFrame(resource_type_))
>     host_->NotifyMainResourceBlocked(manifest_url);
>   else
>     host_->frontend()->OnContentBlocked(host_->host_id(), manifest_url);
>   DeliverNetworkResponse();
>   return;
> }
> 
> if (ResourceType::IsFrame(resource_type_) && cache_id != kNoCacheId) {
>   // AppCacheHost loads and holds a reference to ...
>   host_->LoadMainResourceCache(cache_id);
>   host_->set_preferred_manifest_url(manifest_url);
> }
> 
> wdyt?

Done.

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler.cc:232: if (was_blocked_by_policy)
On 2011/09/05 19:49:31, michaeln wrote:
> style: since the else clause needs {'s, this if clause should also be 
bracketed

Gone.

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
File webkit/appcache/appcache_request_handler_unittest.cc (right):

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_re...
webkit/appcache/appcache_request_handler_unittest.cc:775: 
On 2011/09/05 19:49:31, michaeln wrote:
> does it make sense to also verify these additional host data members?
> 
> host_->main_resource_blocked_;
> host_->blocked_manifest_url_;

Done.

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service.cc (right):

http://codereview.chromium.org/7720022/diff/34002/webkit/appcache/appcache_se...
webkit/appcache/appcache_service.cc:88: CallCallback(net::ERR_FAILED);
On 2011/09/05 19:49:31, michaeln wrote:
> i think you need to 'delete this' here to avoid leaking the helper object

Done.

[marja, 2011-09-06 09:25:13]

Hi avi & tony,

Could you review the following parts (you're OWNERS there):
avi:
content/browser/content_browser_client.h,
content/browser/mock_content_browser_client.cc & .h

tony:
webkit/tools/test_shell/test_shell.gypi

[tony, 2011-09-06 18:29:56]

test_shell LGTM

[michaeln, 2011-09-06 19:12:53]

LGTM

[Avi (use Gerrit), 2011-09-07 14:38:41]

content part LGTM

[commit-bot: I haz the power, 2011-09-08 07:27:05]

Can't apply patch for file webkit/appcache/appcache_request_handler.cc.
While running patch -p1 --forward --force;
patching file webkit/appcache/appcache_request_handler.cc
Hunk #2 FAILED at 217.
1 out of 2 hunks FAILED -- saving rejects to file
webkit/appcache/appcache_request_handler.cc.rej

[commit-bot: I haz the power, 2011-09-08 10:55:25]

Change committed as 100131