Hello nigeltao,
I'd like you to do a code review. Please execute
g4 diff -c 8148979
or point your web browser to
http://mondrian/8148979
to review the following code:
Change 8148979 by [EMAIL PROTECTED] on 2008/09/01 14:41:32 *pending*
Updates implementation of disable_browser_cookies in FFHttpRequest now
that this object is correctly ref-counted. We use a new object as the observer,
rather than the FFHttpRequest object itself, to avoid circular references.
Alos makes sure that the http channel is always valid when we clear the
Cookie header.
R=nigeltao
[EMAIL PROTECTED]
DELTA=80 (47 added, 29 deleted, 4 changed)
OCL=8148979
Affected files ...
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc#21
edit
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.h#17
edit
80 delta lines: 47 added, 29 deleted, 4 changed
Also consider running:
g4 lint -c 8148979
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 8148979 by [EMAIL PROTECTED] on 2008/09/01 14:41:32 *pending*
Updates implementation of disable_browser_cookies in FFHttpRequest now
that this object is correctly ref-counted. We use a new object as the observer,
rather than the FFHttpRequest object itself, to avoid circular references.
Alos makes sure that the http channel is always valid when we clear the
Cookie header.
Affected files ...
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc#21
edit
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.h#17
edit
====
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc#21
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc
2008-09-01 17:46:28.000000000 +0100
+++ googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc
2008-09-01 17:44:49.000000000 +0100
@@ -33,6 +33,7 @@
#include <gecko_sdk/include/nsIInputStream.h>
#include <gecko_sdk/include/nsIHttpHeaderVisitor.h>
#include <gecko_sdk/include/nsIHttpChannel.h>
+#include <gecko_sdk/include/nsIObserver.h>
#include <gecko_sdk/include/nsIObserverService.h>
#include <gecko_internal/nsICachingChannel.h>
#include <gecko_internal/nsIEncodedChannel.h>
@@ -65,12 +66,11 @@
"@mozilla.org/observer-service;1";
static const char *kOnModifyRequestTopic = "http-on-modify-request";
-NS_IMPL_THREADSAFE_ISUPPORTS6(FFHttpRequest,
+NS_IMPL_THREADSAFE_ISUPPORTS5(FFHttpRequest,
nsIRequestObserver,
nsIStreamListener,
nsIChannelEventSink,
nsIInterfaceRequestor,
- nsIObserver,
SpecialHttpRequestInterface)
#if BROWSER_FF3
NS_IMPL_THREADSAFE_ISUPPORTS5(GearsLoadGroup,
@@ -81,6 +81,46 @@
nsIInterfaceRequestor)
#endif
+//------------------------------------------------------------------------------
+// HttpRequestObserver
+//------------------------------------------------------------------------------
+class HttpRequestObserver : public nsIObserver {
+ public:
+ NS_DECL_ISUPPORTS
+
+ HttpRequestObserver(nsIChannel *channel) : channel_(channel) {}
+
+ private:
+ // nsIObserver implementation
+ NS_IMETHODIMP Observe(nsISupports *channel,
+ const char *topic,
+ const PRUnichar * /* data */) {
+ // Check that this is for the right topic.
+ assert(strcmp(topic, kOnModifyRequestTopic) == 0);
+
+ // Note that we will observe this event for all HTTP requests while the
+ // observer is registered. However, we only make changes to the header for
+ // this HTTP request. See
+ // http://developer.mozilla.org/En/Creating_Sandboxed_HTTP_Connections.
+ if (channel_ == channel) {
+ nsIHttpChannel *http_channel = nsnull;
+ CallQueryInterface(channel_, &http_channel);
+ assert(http_channel);
+ nsresult rv = http_channel->SetRequestHeader(nsCString("Cookie"),
+ nsCString(""),
+ PR_FALSE); // Replace
header
+ NS_ENSURE_SUCCESS(rv, false);
+ // Note that we can't unregister the observer from this callback, so we
do
+ // so from the FFHttpRequest destructor.
+ }
+ return NS_OK;
+ }
+
+ nsIChannel *channel_;
+};
+
+NS_IMPL_THREADSAFE_ISUPPORTS1(HttpRequestObserver,
+ nsIObserver);
//------------------------------------------------------------------------------
// HttpRequest::Create
@@ -110,6 +150,7 @@
caching_behavior_(USE_ALL_CACHES),
redirect_behavior_(FOLLOW_ALL),
cookie_behavior_(SEND_BROWSER_COOKIES),
+ observer_(NULL),
was_sent_(false),
was_aborted_(false),
was_redirected_(false),
@@ -120,12 +161,10 @@
FFHttpRequest::~FFHttpRequest() {
LEAK_COUNTER_DECREMENT(FFHttpRequest);
- // TODO(steveblock): It seems that the FFHttpRequest object is leaked by
- // AsyncTask, so the observer is never removed. Investigate this further.
nsCOMPtr<nsIObserverService> observer_service(
do_GetService(kObserverServiceContractId));
- if (observer_service) {
- observer_service->RemoveObserver(this, kOnModifyRequestTopic);
+ if (observer_service && observer_) {
+ observer_service->RemoveObserver(observer_, kOnModifyRequestTopic);
}
if (post_data_stream_attached_) {
post_data_stream_->OnFFHttpRequestDestroyed(this);
@@ -391,7 +430,8 @@
LOG(("FFHttpRequest::Send(): Could not get observer service.\n"));
return false;
}
- observer_service->AddObserver(this, kOnModifyRequestTopic, false);
+ observer_ = new HttpRequestObserver(channel_);
+ observer_service->AddObserver(observer_, kOnModifyRequestTopic, false);
}
if (!IsFileGet()) {
@@ -812,29 +852,6 @@
}
//-----------------------------------------------------------------------------
-// nsIObserver::Observe
-//-----------------------------------------------------------------------------
-NS_IMETHODIMP FFHttpRequest::Observe(nsISupports * /* subject */,
- const char *topic,
- const PRUnichar * /* data */) {
- // Check that this is for the right topic.
- assert(strcmp(topic, kOnModifyRequestTopic) == 0);
-
- assert(cookie_behavior_ == DO_NOT_SEND_BROWSER_COOKIES);
-
- // Note that we will observe this event for all HTTP requests while the
- // observer is registered. However, we only make changes to the header for
- // this HTTP request.
- nsCOMPtr<nsIHttpChannel> http_channel = GetCurrentHttpChannel();
- assert(http_channel);
- nsresult rv = http_channel->SetRequestHeader(nsCString("Cookie"),
- nsCString(""),
- PR_FALSE); // Replace header
- NS_ENSURE_SUCCESS(rv, false);
- return NS_OK;
-}
-
-//-----------------------------------------------------------------------------
// SpecialHttpRequestInterface::GetNativeHttpRequest
//-----------------------------------------------------------------------------
NS_IMETHODIMP FFHttpRequest::GetNativeHttpRequest(FFHttpRequest **retval) {
====
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.h#17
-
c:\MyDocs\Gears3/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.h
2008-09-01 17:46:28.000000000 +0100
+++ googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.h
2008-09-01 17:44:55.000000000 +0100
@@ -30,7 +30,6 @@
#include <vector>
#include "gecko_sdk/include/nsIInterfaceRequestor.h"
#include "gecko_sdk/include/nsILoadGroup.h"
-#include <gecko_sdk/include/nsIObserver.h>
#include "gecko_sdk/include/nsIStreamListener.h"
#include "gecko_internal/nsIChannelEventSink.h"
#include "gecko_internal/nsIDocShellTreeItem.h"
@@ -50,6 +49,9 @@
class nsIHttpChannel;
class nsIRequest;
+// The HttpRequestObserver class is an implementation detail of FFHttpRequest,
+// used to remove browser cookies from outgoing requests.
+class HttpRequestObserver;
#if BROWSER_FF3
class GearsLoadGroup : public nsILoadGroup,
@@ -76,7 +78,6 @@
public nsIStreamListener,
public nsIChannelEventSink,
public nsIInterfaceRequestor,
- public nsIObserver,
public SpecialHttpRequestInterface,
public ProgressEvent::Listener {
public:
@@ -85,7 +86,6 @@
NS_DECL_NSIREQUESTOBSERVER
NS_DECL_NSICHANNELEVENTSINK
NS_DECL_NSIINTERFACEREQUESTOR
- NS_DECL_NSIOBSERVER
NS_DECL_SPECIALHTTPREQUESTINTERFACE // see localserver.idl.m4
// Our C++ HttpRequest interface
@@ -203,6 +203,7 @@
CachingBehavior caching_behavior_;
RedirectBehavior redirect_behavior_;
CookieBehavior cookie_behavior_;
+ nsCOMPtr<HttpRequestObserver> observer_;
bool was_sent_;
bool was_aborted_;
bool was_redirected_;
