Support redirectUrl at onHeadersReceived in WebRequest / DWR API

net/url_request/url_request_http_job.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/url_request_http_job.h"
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 271 matching lines @@
     return;
 
   weak_factory_.InvalidateWeakPtrs();
   DestroyTransaction();
   URLRequestJob::Kill();
 }
 
 void URLRequestHttpJob::NotifyHeadersComplete() {
   DCHECK(!response_info_);
 
+  if (GetResponseHeaders()->HasSafeRedirect()) {
+    // Do not store non-server-issued redirects in cache.
+    transaction_->StopCaching();

[mmenke, 2014/03/20 15:22:25]

I don't think we actually need this.  My cache comment was because I got this
confused with Done(), which would make us finalize the partial cache entry, so
we could reuse it.

If we just delete the URLRequestJob, the HttpCache::Transaction will treat it
like a cancelled request, which we handle fine.

[robwu, 2014/03/20 16:21:11]

DoneReading() just calls DoneWritingToEntry(true); when the cache transaction is
in writing mode. If we end a request via an overridden redirect, then the cache
transaction ends "successfully". Consequently, the original response headers are
written to the cache, but not the response body.

When I tested my initial CL (before 7) as follows, I ended up with a bad state
(see step 6):
1. Load extension that sets up a redirect for http://example.com/
2. Visit http://example.com/ (note: example.com replies with caching directives)
3. Be redirected to data:text/plain,xxx via the extension.
4. Disable extension.
5. Visit http://localhost/.
6. Observe that the page is load from cache and completely blank. The response
without body also shows up in chrome://cache.

[mmenke, 2014/03/20 16:40:28]

We should have a test for that, too, then.

Does this mean (without the patch) if anything overrides the headers of a
non-redirect to be a "safe" redirect, we end up with a bad cache entry?

[mmenke, 2014/03/20 17:22:38]

Turns out this is a bug added back in September by
https://chromiumcodereview.appspot.com/23710059 (Which I reviewed...Hrm...). 
Don't worry about doing anything with the cache, I'll fix that issue today or
tomorrow, which should make this problem go away.

[robwu, 2014/03/20 17:28:14]

Okay, great!
Do you think whether it's acceptable to move the processing of the caching
directives after handling the headers? That would also have fixed this issue,
and furthermore allow extensions to control the caching behavior of sites (by
modifying the response headers).

+  }
+
   response_info_ = transaction_->GetResponseInfo();
 
   // Save boolean, as we'll need this info at destruction time, and filters may
   // also need this info.
   is_cached_content_ = response_info_->was_cached;
 
   if (!is_cached_content_ && throttling_entry_.get()) {
     URLRequestThrottlerHeaderAdapter response_adapter(GetResponseHeaders());
     throttling_entry_->UpdateWithResponse(request_info_.url.host(),
                                           &response_adapter);
@@ skipping 728 matching lines @@
       ? Filter::Factory(encoding_types, *filter_context_) : NULL;
 }
 
 bool URLRequestHttpJob::IsSafeRedirect(const GURL& location) {
   // HTTP is always safe.
   // TODO(pauljensen): Remove once crbug.com/146591 is fixed.
   if (location.is_valid() &&
       (location.scheme() == "http" || location.scheme() == "https")) {
     return true;
   }
+  if (GetResponseHeaders()->IsSafeRedirect(location)) {

[mmenke, 2014/03/20 15:22:25]

I don't think this should be part of the headers, but rather a value in the
URLRequestHttpJob.  We'll need to pass another GURL parameter to the delegate,
which is a bit of a pain, but I consider this a property of the job, rather than
a property of the overridden headers.  Know it's a bit of a pain, but being a
safe redirect isn't treated as a fundamental property of the headers.

[robwu, 2014/03/20 16:21:11]

So, basically the situation from patch set 1?
Then I prefer to use a struct over two parameters, because it is easier to
extend (e.g. with a toggle for status code, http 302 vs 307).

[mmenke, 2014/03/20 16:40:28]

We allow overriding the entire headers, so I don't think we'd need the 302 vs
307 (Overriding the whole headers for that may be overkill, but I prefer that to
adding more cruft to the interface for things the embedder of the stack can
already do, and again, if you override both, that gives rise to ambiguities).

I'm weakly opposed to this largely on style grounds - it's not the style the
network stack currently uses, and this interface does not change frequently
enough that I think future changes are a big concern.

[robwu, 2014/03/20 17:28:14]

After re-reading your previous comment, I see that you meant to pass the safe
redirect target via the delegate instead of the headers, and keep the redirect
override on the response headers. Makes sense.

[mmenke, 2014/03/20 17:31:59]

Not sure you're completely following me...  Here's what I'm thinking:

When calling into the network delegate, just have:

network_delegate()->OnHeadersReceived(... &override_headers_,
&allow_unsafe_redirect_url_);

And then here, just have:
if (allow_unsafe_redirect_url_.is_valid() && allow_unsafe_redirect_url_ ==
location)
  return true;

(Doing all the ref clearing that's needed, of course).

+    return true;
+  }
   // Query URLRequestJobFactory as to whether |location| would be safe to
   // redirect to.
   return request_->context()->job_factory() &&
       request_->context()->job_factory()->IsSafeRedirectTarget(location);
 }
 
 bool URLRequestHttpJob::NeedsAuth() {
   int code = GetResponseCode();
   if (code == -1)
     return false;
@@ skipping 416 matching lines @@
   return override_response_headers_.get() ?
              override_response_headers_.get() :
              transaction_->GetResponseInfo()->headers.get();
 }
 
 void URLRequestHttpJob::NotifyURLRequestDestroyed() {
   awaiting_callback_ = false;
 }
 
 }  // namespace net