diff options
18 files changed, 105 insertions, 508 deletions
diff --git a/chrome/browser/extensions/extension_webrequest_api.cc b/chrome/browser/extensions/extension_webrequest_api.cc index 22102b4..6daf001 100644 --- a/chrome/browser/extensions/extension_webrequest_api.cc +++ b/chrome/browser/extensions/extension_webrequest_api.cc @@ -15,7 +15,6 @@ #include "chrome/browser/extensions/extension_webrequest_api_constants.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/extensions/extension.h" -#include "chrome/common/extensions/extension_error_utils.h" #include "chrome/common/extensions/extension_extent.h" #include "chrome/common/extensions/url_pattern.h" #include "chrome/common/url_constants.h" @@ -129,11 +128,9 @@ static void EventHandledOnIOThread( const std::string& event_name, const std::string& sub_event_name, uint64 request_id, - bool cancel, - const GURL& new_url) { + bool cancel) { ExtensionWebRequestEventRouter::GetInstance()->OnEventHandled( - profile_id, extension_id, event_name, sub_event_name, request_id, - cancel, new_url); + profile_id, extension_id, event_name, sub_event_name, request_id, cancel); } } // namespace @@ -196,13 +193,10 @@ struct ExtensionWebRequestEventRouter::BlockedRequest { // The callback to call when we get a response from all event handlers. net::CompletionCallback* callback; - // If non-empty, this contains the new URL that the request will redirect to. - GURL* new_url; - // Time the request was issued. Used for logging purposes. base::Time request_time; - BlockedRequest() : num_handlers_blocking(0), callback(NULL), new_url(NULL) {} + BlockedRequest() : num_handlers_blocking(0), callback(NULL) {} }; bool ExtensionWebRequestEventRouter::RequestFilter::InitFromValue( @@ -292,8 +286,7 @@ int ExtensionWebRequestEventRouter::OnBeforeRequest( ProfileId profile_id, ExtensionEventRouterForwarder* event_router, net::URLRequest* request, - net::CompletionCallback* callback, - GURL* new_url) { + net::CompletionCallback* callback) { // TODO(jochen): Figure out what to do with events from the system context. if (profile_id == Profile::kInvalidProfileId) return net::OK; @@ -329,10 +322,8 @@ int ExtensionWebRequestEventRouter::OnBeforeRequest( args.Append(dict); if (DispatchEvent(profile_id, event_router, request, callback, listeners, - args)) { - blocked_requests_[request->identifier()].new_url = new_url; + args)) return net::ERR_IO_PENDING; - } return net::OK; } @@ -340,8 +331,8 @@ int ExtensionWebRequestEventRouter::OnBeforeSendHeaders( ProfileId profile_id, ExtensionEventRouterForwarder* event_router, uint64 request_id, - net::CompletionCallback* callback, - net::HttpRequestHeaders* headers) { + net::HttpRequestHeaders* headers, + net::CompletionCallback* callback) { // TODO(jochen): Figure out what to do with events from the system context. if (profile_id == Profile::kInvalidProfileId) return net::OK; @@ -377,7 +368,6 @@ int ExtensionWebRequestEventRouter::OnBeforeSendHeaders( void ExtensionWebRequestEventRouter::OnURLRequestDestroyed( ProfileId profile_id, net::URLRequest* request) { http_requests_.erase(request->identifier()); - blocked_requests_.erase(request->identifier()); } bool ExtensionWebRequestEventRouter::DispatchEvent( @@ -425,8 +415,7 @@ void ExtensionWebRequestEventRouter::OnEventHandled( const std::string& event_name, const std::string& sub_event_name, uint64 request_id, - bool cancel, - const GURL& new_url) { + bool cancel) { EventListener listener; listener.extension_id = extension_id; listener.sub_event_name = sub_event_name; @@ -438,7 +427,7 @@ void ExtensionWebRequestEventRouter::OnEventHandled( if (found != listeners_[profile_id][event_name].end()) found->blocked_requests.erase(request_id); - DecrementBlockCount(request_id, cancel, new_url); + DecrementBlockCount(request_id, cancel); } void ExtensionWebRequestEventRouter::AddEventListener( @@ -484,7 +473,7 @@ void ExtensionWebRequestEventRouter::RemoveEventListener( listeners_[profile_id][event_name].find(listener); for (std::set<uint64>::iterator it = found->blocked_requests.begin(); it != found->blocked_requests.end(); ++it) { - DecrementBlockCount(*it, false, GURL()); + DecrementBlockCount(*it, false); } listeners_[profile_id][event_name].erase(listener); @@ -535,10 +524,9 @@ ExtensionWebRequestEventRouter::GetMatchingListeners( } void ExtensionWebRequestEventRouter::DecrementBlockCount(uint64 request_id, - bool cancel, - const GURL& new_url) { - // It's possible that this request was deleted, or cancelled by a previous - // event handler. If so, ignore this response. + bool cancel) { + // It's possible that this request was already cancelled by a previous event + // handler. If so, ignore this response. if (blocked_requests_.find(request_id) == blocked_requests_.end()) return; @@ -546,15 +534,11 @@ void ExtensionWebRequestEventRouter::DecrementBlockCount(uint64 request_id, int num_handlers_blocking = --blocked_request.num_handlers_blocking; CHECK_GE(num_handlers_blocking, 0); - if (num_handlers_blocking == 0 || cancel || !new_url.is_empty()) { - HISTOGRAM_TIMES("Extensions.NetworkDelay", - base::Time::Now() - blocked_request.request_time); + HISTOGRAM_TIMES("Extensions.NetworkDelay", + base::Time::Now() - blocked_request.request_time); + if (num_handlers_blocking == 0 || cancel) { CHECK(blocked_request.callback); - if (!new_url.is_empty()) { - CHECK(new_url.is_valid()); - *blocked_request.new_url = new_url; - } blocked_request.callback->Run(cancel ? net::ERR_EMPTY_RESPONSE : net::OK); blocked_requests_.erase(request_id); } @@ -609,25 +593,12 @@ bool WebRequestEventHandled::RunImpl() { EXTENSION_FUNCTION_VALIDATE(base::StringToInt64(request_id_str, &request_id)); bool cancel = false; - GURL new_url; if (HasOptionalArgument(3)) { DictionaryValue* value = NULL; EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(3, &value)); if (value->HasKey("cancel")) EXTENSION_FUNCTION_VALIDATE(value->GetBoolean("cancel", &cancel)); - - std::string new_url_str; - if (value->HasKey("redirectUrl")) { - EXTENSION_FUNCTION_VALIDATE(value->GetString("redirectUrl", - &new_url_str)); - new_url = GURL(new_url_str); - if (!new_url.is_valid()) { - error_ = ExtensionErrorUtils::FormatErrorMessage( - keys::kInvalidRedirectUrl, new_url_str); - return false; - } - } } BrowserThread::PostTask( @@ -635,7 +606,7 @@ bool WebRequestEventHandled::RunImpl() { NewRunnableFunction( &EventHandledOnIOThread, profile()->GetRuntimeId(), extension_id(), - event_name, sub_event_name, request_id, cancel, new_url)); + event_name, sub_event_name, request_id, cancel)); return true; } diff --git a/chrome/browser/extensions/extension_webrequest_api.h b/chrome/browser/extensions/extension_webrequest_api.h index 1300900..174ac32 100644 --- a/chrome/browser/extensions/extension_webrequest_api.h +++ b/chrome/browser/extensions/extension_webrequest_api.h @@ -42,8 +42,7 @@ class ExtensionWebRequestEventRouter { int OnBeforeRequest(ProfileId profile_id, ExtensionEventRouterForwarder* event_router, net::URLRequest* request, - net::CompletionCallback* callback, - GURL* new_url); + net::CompletionCallback* callback); // Dispatches the onBeforeSendHeaders event. This is fired for HTTP(s) // requests only, and allows modification of the outgoing request headers. @@ -52,8 +51,8 @@ class ExtensionWebRequestEventRouter { int OnBeforeSendHeaders(ProfileId profile_id, ExtensionEventRouterForwarder* event_router, uint64 request_id, - net::CompletionCallback* callback, - net::HttpRequestHeaders* headers); + net::HttpRequestHeaders* headers, + net::CompletionCallback* callback); void OnURLRequestDestroyed(ProfileId profile_id, net::URLRequest* request); @@ -65,8 +64,7 @@ class ExtensionWebRequestEventRouter { const std::string& event_name, const std::string& sub_event_name, uint64 request_id, - bool cancel, - const GURL& new_url); + bool cancel); // Adds a listener to the given event. |event_name| specifies the event being // listened to. |sub_event_name| is an internal event uniquely generated in @@ -121,11 +119,10 @@ class ExtensionWebRequestEventRouter { ProfileId profile_id, const std::string& event_name, net::URLRequest* request); - // Decrements the count of event handlers blocking the given request. When the // count reaches 0 (or immediately if the request is being cancelled), we // stop blocking the request and either resume or cancel it. - void DecrementBlockCount(uint64 request_id, bool cancel, const GURL& new_url); + void DecrementBlockCount(uint64 request_id, bool cancel); void OnRequestDeleted(net::URLRequest* request); diff --git a/chrome/browser/extensions/extension_webrequest_api_constants.cc b/chrome/browser/extensions/extension_webrequest_api_constants.cc index d6bb17a..32a6f4c 100644 --- a/chrome/browser/extensions/extension_webrequest_api_constants.cc +++ b/chrome/browser/extensions/extension_webrequest_api_constants.cc @@ -26,6 +26,4 @@ const char kOnErrorOccurred[] = "experimental.webRequest.onErrorOccurred"; const char kOnHeadersReceived[] = "experimental.webRequest.onHeadersReceived"; const char kOnRequestSent[] = "experimental.webRequest.onRequestSent"; -const char kInvalidRedirectUrl[] = "redirectUrl '*' is not a valid URL."; - } // namespace extension_webrequest_api_constants diff --git a/chrome/browser/extensions/extension_webrequest_api_constants.h b/chrome/browser/extensions/extension_webrequest_api_constants.h index 56cb449..21c654a 100644 --- a/chrome/browser/extensions/extension_webrequest_api_constants.h +++ b/chrome/browser/extensions/extension_webrequest_api_constants.h @@ -31,9 +31,6 @@ extern const char kOnErrorOccurred[]; extern const char kOnHeadersReceived[]; extern const char kOnRequestSent[]; -// Error messages. -extern const char kInvalidRedirectUrl[]; - } // namespace extension_webrequest_api_constants #endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_WEBREQUEST_API_CONSTANTS_H_ diff --git a/chrome/browser/net/chrome_network_delegate.cc b/chrome/browser/net/chrome_network_delegate.cc index 6cb0321..28669fd 100644 --- a/chrome/browser/net/chrome_network_delegate.cc +++ b/chrome/browser/net/chrome_network_delegate.cc @@ -61,21 +61,19 @@ void ChromeNetworkDelegate::InitializeReferrersEnabled( } int ChromeNetworkDelegate::OnBeforeURLRequest( - net::URLRequest* request, - net::CompletionCallback* callback, - GURL* new_url) { + net::URLRequest* request, net::CompletionCallback* callback) { if (!enable_referrers_->GetValue()) request->set_referrer(std::string()); return ExtensionWebRequestEventRouter::GetInstance()->OnBeforeRequest( - profile_id_, event_router_.get(), request, callback, new_url); + profile_id_, event_router_.get(), request, callback); } int ChromeNetworkDelegate::OnBeforeSendHeaders( uint64 request_id, - net::CompletionCallback* callback, - net::HttpRequestHeaders* headers) { + net::HttpRequestHeaders* headers, + net::CompletionCallback* callback) { return ExtensionWebRequestEventRouter::GetInstance()->OnBeforeSendHeaders( - profile_id_, event_router_.get(), request_id, callback, headers); + profile_id_, event_router_.get(), request_id, headers, callback); } void ChromeNetworkDelegate::OnResponseStarted(net::URLRequest* request) { diff --git a/chrome/browser/net/chrome_network_delegate.h b/chrome/browser/net/chrome_network_delegate.h index 3e414f4..b5c1373 100644 --- a/chrome/browser/net/chrome_network_delegate.h +++ b/chrome/browser/net/chrome_network_delegate.h @@ -40,11 +40,10 @@ class ChromeNetworkDelegate : public net::NetworkDelegate { private: // NetworkDelegate methods: virtual int OnBeforeURLRequest(net::URLRequest* request, - net::CompletionCallback* callback, - GURL* new_url); + net::CompletionCallback* callback); virtual int OnBeforeSendHeaders(uint64 request_id, - net::CompletionCallback* callback, - net::HttpRequestHeaders* headers); + net::HttpRequestHeaders* headers, + net::CompletionCallback* callback); virtual void OnResponseStarted(net::URLRequest* request); virtual void OnReadCompleted(net::URLRequest* request, int bytes_read); virtual void OnURLRequestDestroyed(net::URLRequest* request); diff --git a/chrome/common/extensions/api/extension_api.json b/chrome/common/extensions/api/extension_api.json index 8a098be..fb48df2 100644 --- a/chrome/common/extensions/api/extension_api.json +++ b/chrome/common/extensions/api/extension_api.json @@ -3539,23 +3539,6 @@ "tabId": { "type": "integer", "optional": true }, "windowId": { "type": "integer", "optional": true } } - }, - { - "id": "BlockingResponse", - "type": "object", - "description": "Return value for event handlers that have the 'blocking' extraInfoSpec applied. Allows the event handler to modify network requests.", - "properties": { - "cancel": { - "type": "boolean", - "optional": true, - "description": "If true, the request is cancelled. Used in onBeforeRequest, this prevents the request from being sent." - }, - "redirectUrl": { - "type": "string", - "optional": true, - "description": "If set, the original request is prevented from being sent and is instead redirected to the given URL." - } - } } ], "functions": [ @@ -3596,9 +3579,12 @@ {"type": "string", "name": "subEventName"}, {"type": "string", "name": "requestId"}, { - "$ref": "BlockingResponse", + "type": "object", + "name": "response", "optional": true, - "name": "response" + "properties": { + "cancel": {"type": "boolean", "optional": true} + } } ] } diff --git a/chrome/common/extensions/docs/experimental.webRequest.html b/chrome/common/extensions/docs/experimental.webRequest.html index 65f7ce6..859d2ba 100644 --- a/chrome/common/extensions/docs/experimental.webRequest.html +++ b/chrome/common/extensions/docs/experimental.webRequest.html @@ -311,8 +311,6 @@ <ol> <li> <a href="#type-RequestFilter">RequestFilter</a> - </li><li> - <a href="#type-BlockingResponse">BlockingResponse</a> </li> </ol> </li> @@ -3570,211 +3568,6 @@ unexpected results. </div> - </div><div class="apiItem"> - <a name="type-BlockingResponse"></a> - <h4>BlockingResponse</h4> - - <div> - <dt> - <var style="display: none; ">paramName</var> - <em> - - <!-- TYPE --> - <div style="display:inline"> - ( - <span class="optional" style="display: none; ">optional</span> - <span class="enum" style="display: none; ">enumerated</span> - <span id="typeTemplate"> - <span style="display: none; "> - <a> Type</a> - </span> - <span> - <span style="display: none; "> - array of <span><span></span></span> - </span> - <span>object</span> - <span style="display: none; "></span> - </span> - </span> - ) - </div> - - </em> - </dt> - <dd class="todo" style="display: none; "> - Undocumented. - </dd> - <dd>Return value for event handlers that have the 'blocking' extraInfoSpec applied. Allows the event handler to modify network requests.</dd> - <dd style="display: none; "> - This parameter was added in version - <b><span></span></b>. - You must omit this parameter in earlier versions, - and you may omit it in any version. If you require this - parameter, the manifest key - <a href="manifest.html#minimum_chrome_version">minimum_chrome_version</a> - can ensure that your extension won't be run in an earlier browser version. - </dd> - - <!-- OBJECT PROPERTIES --> - <dd> - <dl> - <div> - <div> - <dt> - <var>cancel</var> - <em> - - <!-- TYPE --> - <div style="display:inline"> - ( - <span class="optional">optional</span> - <span class="enum" style="display: none; ">enumerated</span> - <span id="typeTemplate"> - <span style="display: none; "> - <a> Type</a> - </span> - <span> - <span style="display: none; "> - array of <span><span></span></span> - </span> - <span>boolean</span> - <span style="display: none; "></span> - </span> - </span> - ) - </div> - - </em> - </dt> - <dd class="todo" style="display: none; "> - Undocumented. - </dd> - <dd>If true, the request is cancelled. Used in onBeforeRequest, this prevents the request from being sent.</dd> - <dd style="display: none; "> - This parameter was added in version - <b><span></span></b>. - You must omit this parameter in earlier versions, - and you may omit it in any version. If you require this - parameter, the manifest key - <a href="manifest.html#minimum_chrome_version">minimum_chrome_version</a> - can ensure that your extension won't be run in an earlier browser version. - </dd> - - <!-- OBJECT PROPERTIES --> - <dd style="display: none; "> - <dl> - <div> - <div> - </div> - </div> - </dl> - </dd> - - <!-- OBJECT METHODS --> - <dd style="display: none; "> - <div></div> - </dd> - - <!-- OBJECT EVENT FIELDS --> - <dd style="display: none; "> - <div></div> - </dd> - - <!-- FUNCTION PARAMETERS --> - <dd style="display: none; "> - <div></div> - </dd> - - </div> - </div><div> - <div> - <dt> - <var>redirectUrl</var> - <em> - - <!-- TYPE --> - <div style="display:inline"> - ( - <span class="optional">optional</span> - <span class="enum" style="display: none; ">enumerated</span> - <span id="typeTemplate"> - <span style="display: none; "> - <a> Type</a> - </span> - <span> - <span style="display: none; "> - array of <span><span></span></span> - </span> - <span>string</span> - <span style="display: none; "></span> - </span> - </span> - ) - </div> - - </em> - </dt> - <dd class="todo" style="display: none; "> - Undocumented. - </dd> - <dd>If set, the original request is prevented from being sent and is instead redirected to the given URL.</dd> - <dd style="display: none; "> - This parameter was added in version - <b><span></span></b>. - You must omit this parameter in earlier versions, - and you may omit it in any version. If you require this - parameter, the manifest key - <a href="manifest.html#minimum_chrome_version">minimum_chrome_version</a> - can ensure that your extension won't be run in an earlier browser version. - </dd> - - <!-- OBJECT PROPERTIES --> - <dd style="display: none; "> - <dl> - <div> - <div> - </div> - </div> - </dl> - </dd> - - <!-- OBJECT METHODS --> - <dd style="display: none; "> - <div></div> - </dd> - - <!-- OBJECT EVENT FIELDS --> - <dd style="display: none; "> - <div></div> - </dd> - - <!-- FUNCTION PARAMETERS --> - <dd style="display: none; "> - <div></div> - </dd> - - </div> - </div> - </dl> - </dd> - - <!-- OBJECT METHODS --> - <dd style="display: none; "> - <div></div> - </dd> - - <!-- OBJECT EVENT FIELDS --> - <dd style="display: none; "> - <div></div> - </dd> - - <!-- FUNCTION PARAMETERS --> - <dd style="display: none; "> - <div></div> - </dd> - - </div> - </div> <!-- /apiItem --> </div> <!-- /apiGroup --> diff --git a/chrome/test/data/extensions/api_test/webrequest/events/test.html b/chrome/test/data/extensions/api_test/webrequest/events/test.html index 95b47b4..60eec41 100644 --- a/chrome/test/data/extensions/api_test/webrequest/events/test.html +++ b/chrome/test/data/extensions/api_test/webrequest/events/test.html @@ -46,50 +46,47 @@ function captureEvent(name, details) { details.url.match(/\/favicon.ico$/)) return; - // Pull the extra per-event options out of the expected data. These let - // us specify special return values per event. - var currentIndex = capturedEventData.length; - var extraOptions; - if (expectedEventData.length >= currentIndex) { - extraOptions = expectedEventData[currentIndex][2]; - expectedEventData[currentIndex].splice(2, 1); - } - delete details.requestId; delete details.timeStamp; capturedEventData.push([name, details]); checkExpectations(); - return extraOptions ? extraOptions.retval : undefined; } function initListeners(filter, extraInfoSpec) { chrome.experimental.webRequest.onBeforeRequest.addListener( function(details) { - return captureEvent("onBeforeRequest", details); + captureEvent("onBeforeRequest", details); + return {cancel: true}; // no effect unless event is blocking. }, filter, extraInfoSpec); chrome.experimental.webRequest.onBeforeSendHeaders.addListener( function(details) { - return captureEvent("onBeforeSendHeaders", details); + captureEvent("onBeforeSendHeaders", details); + return {cancel: true}; // no effect unless event is blocking. }, filter, extraInfoSpec); chrome.experimental.webRequest.onRequestSent.addListener( function(details) { - return captureEvent("onRequestSent", details); + captureEvent("onRequestSent", details); + return {cancel: true}; }, filter, extraInfoSpec); chrome.experimental.webRequest.onHeadersReceived.addListener( function(details) { - return captureEvent("onHeadersReceived", details); + captureEvent("onHeadersReceived", details); + return {cancel: true}; }, filter, extraInfoSpec); chrome.experimental.webRequest.onBeforeRedirect.addListener( function(details) { - return captureEvent("onBeforeRedirect", details); + captureEvent("onBeforeRedirect", details); + return {cancel: true}; }, filter, extraInfoSpec); chrome.experimental.webRequest.onCompleted.addListener( function(details) { - return captureEvent("onCompleted", details); + captureEvent("onCompleted", details); + return {cancel: true}; }, filter, extraInfoSpec); chrome.experimental.webRequest.onErrorOccurred.addListener( function(details) { - return captureEvent("onErrorOccurred", details); + captureEvent("onErrorOccurred", details); + return {cancel: true}; }, filter, extraInfoSpec); } @@ -185,7 +182,7 @@ runTests([ // Navigates to a page with subresources, with a blocking handler that // cancels the page request. The page will not load, and we should not // see the subresources. - function complexLoadCancelled() { + function complexLoadBlocking() { expect([ [ "onBeforeRequest", { @@ -193,26 +190,7 @@ runTests([ tabId: tabId, type: "main_frame", url: getURL("complexLoad/a.html") - }, - { retval: {cancel: true} } - ] - ], {}, ["blocking"]); - chrome.tabs.update(tabId, { url: getURL("complexLoad/a.html") }); - }, - - // Navigates to a page with a blocking handler that redirects to a different - // page. - // TODO(mpcomplete): will get an onBeforeRedirect event for the new request. - function complexLoadRedirected() { - expect([ - [ "onBeforeRequest", - { - method: "GET", - tabId: tabId, - type: "main_frame", - url: getURL("complexLoad/a.html") - }, - { retval: {redirectUrl: getURL("simpleLoad/a.html")} } + } ] ], {}, ["blocking"]); chrome.tabs.update(tabId, { url: getURL("complexLoad/a.html") }); diff --git a/net/base/network_delegate.cc b/net/base/network_delegate.cc index d707c44..2ed1ea5 100644 --- a/net/base/network_delegate.cc +++ b/net/base/network_delegate.cc @@ -9,21 +9,20 @@ namespace net { int NetworkDelegate::NotifyBeforeURLRequest(URLRequest* request, - CompletionCallback* callback, - GURL* new_url) { + CompletionCallback* callback) { DCHECK(CalledOnValidThread()); DCHECK(request); DCHECK(callback); - return OnBeforeURLRequest(request, callback, new_url); + return OnBeforeURLRequest(request, callback); } int NetworkDelegate::NotifyBeforeSendHeaders(uint64 request_id, - CompletionCallback* callback, - HttpRequestHeaders* headers) { + HttpRequestHeaders* headers, + CompletionCallback* callback) { DCHECK(CalledOnValidThread()); DCHECK(headers); DCHECK(callback); - return OnBeforeSendHeaders(request_id, callback, headers); + return OnBeforeSendHeaders(request_id, headers, callback); } void NetworkDelegate::NotifyResponseStarted(URLRequest* request) { diff --git a/net/base/network_delegate.h b/net/base/network_delegate.h index eafb212..effc699 100644 --- a/net/base/network_delegate.h +++ b/net/base/network_delegate.h @@ -9,8 +9,6 @@ #include "base/threading/non_thread_safe.h" #include "net/base/completion_callback.h" -class GURL; - namespace net { // NOTE: Layering violations! @@ -36,11 +34,10 @@ class NetworkDelegate : public base::NonThreadSafe { // checking on parameters. See the corresponding virtuals for explanations of // the methods and their arguments. int NotifyBeforeURLRequest(URLRequest* request, - CompletionCallback* callback, - GURL* new_url); + CompletionCallback* callback); int NotifyBeforeSendHeaders(uint64 request_id, - CompletionCallback* callback, - HttpRequestHeaders* headers); + HttpRequestHeaders* headers, + CompletionCallback* callback); void NotifyResponseStarted(URLRequest* request); void NotifyReadCompleted(URLRequest* request, int bytes_read); void NotifyURLRequestDestroyed(URLRequest* request); @@ -58,22 +55,20 @@ class NetworkDelegate : public base::NonThreadSafe { // member functions will be called by the respective public notification // member function, which will perform basic sanity checking. - // Called before a request is sent. Allows the delegate to rewrite the URL - // being fetched by modifying |new_url|. The callback can be called at any - // time, but will have no effect if the request has already been cancelled or + // Called before a request is sent. The callback can be called at any time, + // but will have no effect if the request has already been cancelled or // deleted. Returns a net status code, generally either OK to continue with // the request or ERR_IO_PENDING if the result is not ready yet. virtual int OnBeforeURLRequest(URLRequest* request, - CompletionCallback* callback, - GURL* new_url) = 0; + CompletionCallback* callback) = 0; // Called right before the HTTP headers are sent. Allows the delegate to // read/write |headers| before they get sent out. The callback can be called // at any time, but will have no effect if the transaction handling this // request has been cancelled. Returns a net status code. virtual int OnBeforeSendHeaders(uint64 request_id, - CompletionCallback* callback, - HttpRequestHeaders* headers) = 0; + HttpRequestHeaders* headers, + CompletionCallback* callback) = 0; // This corresponds to URLRequestDelegate::OnResponseStarted. virtual void OnResponseStarted(URLRequest* request) = 0; diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc index a12a64f..c4293d9 100644 --- a/net/http/http_cache_transaction.cc +++ b/net/http/http_cache_transaction.cc @@ -908,7 +908,7 @@ int HttpCache::Transaction::DoNotifyBeforeSendHeaders() { // TODO(mpcomplete): need to be able to modify these headers. HttpRequestHeaders headers = request_->extra_headers; return cache_->GetSession()->network_delegate()->NotifyBeforeSendHeaders( - request_->request_id, cache_callback_, &headers); + request_->request_id, &headers, cache_callback_); } return OK; diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc index 5651a41..f7d3acc 100644 --- a/net/http/http_network_transaction.cc +++ b/net/http/http_network_transaction.cc @@ -771,7 +771,7 @@ int HttpNetworkTransaction::DoBuildRequest() { if (session_->network_delegate()) { return session_->network_delegate()->NotifyBeforeSendHeaders( - request_->request_id, delegate_callback_, &request_headers_); + request_->request_id, &request_headers_, delegate_callback_); } return OK; diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc index 3d60137..5c437c2 100644 --- a/net/url_request/url_request.cc +++ b/net/url_request/url_request.cc @@ -23,7 +23,6 @@ #include "net/url_request/url_request_job.h" #include "net/url_request/url_request_job_manager.h" #include "net/url_request/url_request_netlog_params.h" -#include "net/url_request/url_request_redirect_job.h" using base::Time; using std::string; @@ -121,9 +120,7 @@ URLRequest::URLRequest(const GURL& url, Delegate* delegate) redirect_limit_(kMaxRedirects), final_upload_progress_(0), priority_(LOWEST), - identifier_(GenerateURLRequestIdentifier()), - ALLOW_THIS_IN_INITIALIZER_LIST( - before_request_callback_(this, &URLRequest::BeforeRequestComplete)) { + identifier_(GenerateURLRequestIdentifier()) { SIMPLE_STATS_COUNTER("URLRequestCount"); // Sanity check out environment. @@ -137,6 +134,9 @@ URLRequest::~URLRequest() { if (context_ && context_->network_delegate()) context_->network_delegate()->NotifyURLRequestDestroyed(this); + if (before_request_callback_) + before_request_callback_->Cancel(); + Cancel(); if (job_) @@ -362,41 +362,22 @@ GURL URLRequest::GetSanitizedReferrer() const { void URLRequest::Start() { response_info_.request_time = Time::Now(); - // Only notify the delegate for the initial request. if (context_ && context_->network_delegate()) { + before_request_callback_ = new CancelableCompletionCallback<URLRequest>( + this, &URLRequest::BeforeRequestComplete); if (context_->network_delegate()->NotifyBeforeURLRequest( - this, &before_request_callback_, &delegate_redirect_url_) == - net::ERR_IO_PENDING) { + this, before_request_callback_) == net::ERR_IO_PENDING) { + before_request_callback_->AddRef(); // balanced in BeforeRequestComplete net_log_.BeginEvent(NetLog::TYPE_URL_REQUEST_BLOCKED_ON_EXTENSION, NULL); return; // paused } } - StartInternal(); + StartJob(URLRequestJobManager::GetInstance()->CreateJob(this)); } /////////////////////////////////////////////////////////////////////////////// -void URLRequest::BeforeRequestComplete(int error) { - DCHECK(!job_); - DCHECK_NE(ERR_IO_PENDING, error); - - net_log_.EndEvent(NetLog::TYPE_URL_REQUEST_BLOCKED_ON_EXTENSION, NULL); - if (error != OK) { - StartJob(new URLRequestErrorJob(this, error)); - } else if (!delegate_redirect_url_.is_empty()) { - GURL new_url; - new_url.Swap(&delegate_redirect_url_); - StartJob(new URLRequestRedirectJob(this, new_url)); - } else { - StartInternal(); - } -} - -void URLRequest::StartInternal() { - StartJob(URLRequestJobManager::GetInstance()->CreateJob(this)); -} - void URLRequest::StartJob(URLRequestJob* job) { DCHECK(!is_pending_); DCHECK(!job_); @@ -423,6 +404,18 @@ void URLRequest::StartJob(URLRequestJob* job) { job_->Start(); } +void URLRequest::BeforeRequestComplete(int error) { + DCHECK(!job_); + + net_log_.EndEvent(NetLog::TYPE_URL_REQUEST_BLOCKED_ON_EXTENSION, NULL); + before_request_callback_->Release(); // balanced in Start + if (error != OK) { + StartJob(new URLRequestErrorJob(this, error)); + } else { + StartJob(URLRequestJobManager::GetInstance()->CreateJob(this)); + } +} + void URLRequest::Restart() { // Should only be called if the original job didn't make any progress. DCHECK(job_ && !job_->has_response_started()); @@ -635,7 +628,7 @@ int URLRequest::Redirect(const GURL& location, int http_status_code) { final_upload_progress_ = job_->GetUploadProgress(); PrepareToRestart(); - StartInternal(); + Start(); return OK; } diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h index b25b842..40f7d8a 100644 --- a/net/url_request/url_request.h +++ b/net/url_request/url_request.h @@ -584,15 +584,6 @@ class URLRequest : public base::NonThreadSafe { friend class URLRequestJob; typedef std::map<const void*, linked_ptr<UserData> > UserDataMap; - void StartInternal(); - - // Resumes or blocks a request paused by the NetworkDelegate::OnBeforeRequest - // handler. If |blocked| is true, the request is blocked and an error page is - // returned indicating so. This should only be called after Start is called - // and OnBeforeRequest returns true (signalling that the request should be - // paused). - void BeforeRequestComplete(int error); - void StartJob(URLRequestJob* job); // Restarting involves replacing the current job with a new one such as what @@ -609,6 +600,13 @@ class URLRequest : public base::NonThreadSafe { // passed values. void DoCancel(int os_error, const SSLInfo& ssl_info); + // Resumes or blocks a request paused by the NetworkDelegate::OnBeforeRequest + // handler. If |blocked| is true, the request is blocked and an error page is + // returned indicating so. This should only be called after Start is called + // and OnBeforeRequest returns true (signalling that the request should be + // paused). + void BeforeRequestComplete(int error); + // Contextual information used for this request (can be NULL). This contains // most of the dependencies which are shared between requests (disk cache, // cookie store, socket pool, etc.) @@ -622,7 +620,6 @@ class URLRequest : public base::NonThreadSafe { GURL url_; GURL original_url_; GURL first_party_for_cookies_; - GURL delegate_redirect_url_; std::string method_; // "GET", "POST", etc. Should be all uppercase. std::string referrer_; HttpRequestHeaders extra_request_headers_; @@ -666,7 +663,8 @@ class URLRequest : public base::NonThreadSafe { // Callback passed to the network delegate to notify us when a blocked request // is ready to be resumed or canceled. - CompletionCallbackImpl<URLRequest> before_request_callback_; + scoped_refptr< CancelableCompletionCallback<URLRequest> > + before_request_callback_; DISALLOW_COPY_AND_ASSIGN(URLRequest); }; diff --git a/net/url_request/url_request_test_util.cc b/net/url_request/url_request_test_util.cc index a91460a..aedda63 100644 --- a/net/url_request/url_request_test_util.cc +++ b/net/url_request/url_request_test_util.cc @@ -248,25 +248,20 @@ void TestDelegate::OnResponseCompleted(net::URLRequest* request) { TestNetworkDelegate::TestNetworkDelegate() : last_os_error_(0), - error_count_(0), - created_requests_(0), - destroyed_requests_(0) { + error_count_(0) { } TestNetworkDelegate::~TestNetworkDelegate() {} int TestNetworkDelegate::OnBeforeURLRequest( - net::URLRequest* request, - net::CompletionCallback* callback, - GURL* new_url) { - created_requests_++; + net::URLRequest* request, net::CompletionCallback* callback) { return net::OK; } int TestNetworkDelegate::OnBeforeSendHeaders( uint64 request_id, - net::CompletionCallback* callback, - net::HttpRequestHeaders* headers) { + net::HttpRequestHeaders* headers, + net::CompletionCallback* callback) { return net::OK; } @@ -286,7 +281,6 @@ void TestNetworkDelegate::OnReadCompleted(net::URLRequest* request, } void TestNetworkDelegate::OnURLRequestDestroyed(net::URLRequest* request) { - destroyed_requests_++; } net::URLRequestJob* TestNetworkDelegate::OnMaybeCreateURLRequestJob( diff --git a/net/url_request/url_request_test_util.h b/net/url_request/url_request_test_util.h index cef5449..4d77d80 100644 --- a/net/url_request/url_request_test_util.h +++ b/net/url_request/url_request_test_util.h @@ -190,17 +190,14 @@ class TestNetworkDelegate : public net::NetworkDelegate { int last_os_error() const { return last_os_error_; } int error_count() const { return error_count_; } - int created_requests() const { return created_requests_; } - int destroyed_requests() const { return destroyed_requests_; } - protected: + private: // net::NetworkDelegate: virtual int OnBeforeURLRequest(net::URLRequest* request, - net::CompletionCallback* callback, - GURL* new_url); + net::CompletionCallback* callback); virtual int OnBeforeSendHeaders(uint64 request_id, - net::CompletionCallback* callback, - net::HttpRequestHeaders* headers); + net::HttpRequestHeaders* headers, + net::CompletionCallback* callback); virtual void OnResponseStarted(net::URLRequest* request); virtual void OnReadCompleted(net::URLRequest* request, int bytes_read); virtual void OnURLRequestDestroyed(net::URLRequest* request); @@ -209,8 +206,6 @@ class TestNetworkDelegate : public net::NetworkDelegate { int last_os_error_; int error_count_; - int created_requests_; - int destroyed_requests_; }; #endif // NET_URL_REQUEST_URL_REQUEST_TEST_UTIL_H_ diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc index 7ecc847..394f813 100644 --- a/net/url_request/url_request_unittest.cc +++ b/net/url_request/url_request_unittest.cc @@ -53,13 +53,6 @@ using base::Time; -// We don't need a refcount because we are guaranteed the test will not proceed -// until its task is run. -namespace net { -class BlockingNetworkDelegate; -} // namespace net -DISABLE_RUNNABLE_METHOD_REFCOUNT(net::BlockingNetworkDelegate); - namespace net { namespace { @@ -125,38 +118,6 @@ void CheckSSLInfo(const SSLInfo& ssl_info) { } // namespace -// A network delegate that blocks requests, optionally cancelling or redirecting -// them. -class BlockingNetworkDelegate : public TestNetworkDelegate { - public: - BlockingNetworkDelegate() : callback_retval_(net::OK) {} - - void set_callback_retval(int retval) { callback_retval_ = retval; } - void set_redirect_url(const GURL& url) { redirect_url_ = url; } - - private: - // TestNetworkDelegate: - virtual int OnBeforeURLRequest(net::URLRequest* request, - net::CompletionCallback* callback, - GURL* new_url) { - TestNetworkDelegate::OnBeforeURLRequest(request, callback, new_url); - - if (!redirect_url_.is_empty()) - *new_url = redirect_url_; - MessageLoop::current()->PostTask(FROM_HERE, - NewRunnableMethod(this, &BlockingNetworkDelegate::DoCallback, - callback)); - return net::ERR_IO_PENDING; - } - - void DoCallback(net::CompletionCallback* callback) { - callback->Run(callback_retval_); - } - - int callback_retval_; - GURL redirect_url_; -}; - // Inherit PlatformTest since we require the autorelease pool on Mac OS X.f class URLRequestTest : public PlatformTest { public: @@ -302,61 +263,6 @@ TEST_F(URLRequestTestHTTP, NetworkDelegateTunnelConnectionFailed) { } } -// Tests that the network delegate can block and cancel a request. -TEST_F(URLRequestTestHTTP, NetworkDelegateCancelRequest) { - ASSERT_TRUE(test_server_.Start()); - - TestDelegate d; - BlockingNetworkDelegate network_delegate; - network_delegate.set_callback_retval(ERR_EMPTY_RESPONSE); - - { - TestURLRequest r(test_server_.GetURL(""), &d); - scoped_refptr<TestURLRequestContext> context( - new TestURLRequestContext(test_server_.host_port_pair().ToString())); - context->set_network_delegate(&network_delegate); - r.set_context(context); - - r.Start(); - MessageLoop::current()->Run(); - - EXPECT_EQ(URLRequestStatus::FAILED, r.status().status()); - EXPECT_EQ(ERR_EMPTY_RESPONSE, r.status().os_error()); - EXPECT_EQ(1, network_delegate.created_requests()); - EXPECT_EQ(0, network_delegate.destroyed_requests()); - } - EXPECT_EQ(1, network_delegate.destroyed_requests()); -} - -// Tests that the network delegate can block and redirect a request to a new -// URL. -TEST_F(URLRequestTestHTTP, NetworkDelegateRedirectRequest) { - ASSERT_TRUE(test_server_.Start()); - - TestDelegate d; - BlockingNetworkDelegate network_delegate; - GURL redirect_url(test_server_.GetURL("simple.html")); - network_delegate.set_redirect_url(redirect_url); - - { - TestURLRequest r(test_server_.GetURL("empty.html"), &d); - scoped_refptr<TestURLRequestContext> context( - new TestURLRequestContext(test_server_.host_port_pair().ToString())); - context->set_network_delegate(&network_delegate); - r.set_context(context); - - r.Start(); - MessageLoop::current()->Run(); - - EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status()); - EXPECT_EQ(0, r.status().os_error()); - EXPECT_EQ(redirect_url, r.url()); - EXPECT_EQ(1, network_delegate.created_requests()); - EXPECT_EQ(0, network_delegate.destroyed_requests()); - } - EXPECT_EQ(1, network_delegate.destroyed_requests()); -} - // In this unit test, we're using the HTTPTestServer as a proxy server and // issuing a CONNECT request with the magic host name "www.server-auth.com". // The HTTPTestServer will return a 401 response, which we should balk at. |