Hello playmobil,
I'd like you to do a code review. Please execute
g4 diff -c 8201218
or point your web browser to
http://mondrian/8201218
to review the following code:
Change 8201218 by [EMAIL PROTECTED] on 2008/09/08 17:24:27 *pending*
Break SFHttpRequest <---> ProgressInputStream reference cycle,
which was causing those ref-counted objects to not get deleted.
This is Safari's analog of Firefox's 7834682.
PRESUBMIT=passed
R=playmobil
[EMAIL PROTECTED]
DELTA=34 (27 added, 1 deleted, 6 changed)
OCL=8201218
Affected files ...
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc#22
edit
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.h#3
edit
...
//depot/googleclient/gears/opensource/gears/localserver/safari/http_request_sf.mm#23
edit
...
//depot/googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.h#5
edit
...
//depot/googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.mm#6
edit
34 delta lines: 27 added, 1 deleted, 6 changed
Also consider running:
g4 lint -c 8201218
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 8201218 by [EMAIL PROTECTED] on 2008/09/08 17:24:27 *pending*
Break SFHttpRequest <---> ProgressInputStream reference cycle, which
was causing those ref-counted objects to not get deleted.
This is Safari's analog of Firefox's 7834682.
OCL=8201218
Affected files ...
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc#22
edit
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.cc#6
edit
...
//depot/googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.h#3
edit
...
//depot/googleclient/gears/opensource/gears/localserver/safari/http_request_sf.mm#23
edit
...
//depot/googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.h#5
edit
...
//depot/googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.mm#6
edit
====
//depot/googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc#22
-
/Users/nigeltao/devel/srcmacgears1/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-08 17:25:20.000000000 +1000
+++ googleclient/gears/opensource/gears/localserver/firefox/http_request_ff.cc
2008-09-08 17:20:30.000000000 +1000
@@ -167,7 +167,7 @@
observer_service->RemoveObserver(observer_, kOnModifyRequestTopic);
}
if (post_data_stream_attached_) {
- post_data_stream_->OnFFHttpRequestDestroyed(this);
+ post_data_stream_->OnFFHttpRequestDetached(this);
}
}
====
//depot/googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.cc#6
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.cc
====
# action=edit type=text
---
googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.cc
2008-09-08 17:25:20.000000000 +1000
+++
googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.cc
2008-09-08 17:21:00.000000000 +1000
@@ -100,7 +100,7 @@
return result;
}
-void ProgressInputStream::OnFFHttpRequestDestroyed(FFHttpRequest *request) {
+void ProgressInputStream::OnFFHttpRequestDetached(FFHttpRequest *request) {
assert(request == request_);
request_ = NULL;
}
====
//depot/googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.h#3
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.h
====
# action=edit type=text
---
googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.h
2008-09-08 17:25:20.000000000 +1000
+++
googleclient/gears/opensource/gears/localserver/firefox/progress_input_stream.h
2008-09-08 17:21:57.000000000 +1000
@@ -38,7 +38,7 @@
// If Read() is called, then progress cannot be provided.
//
// This class should only be created by a FFHttpRequest, and that FFHttpRequest
-// should call OnFFHttpRequestDestroyed whenever the FFHttpRequest no longer
+// should call OnFFHttpRequestDetached whenever the FFHttpRequest no longer
// points to this ProgressInputStream (e.g. during FFHttpRequest's destructor).
// This constraint is in order to let the two objects point to each other
// without causing a reference cycle that prohibits deleting them after use.
@@ -52,7 +52,7 @@
NS_DECL_ISUPPORTS
NS_DECL_NSIINPUTSTREAM
- void OnFFHttpRequestDestroyed(FFHttpRequest *request);
+ void OnFFHttpRequestDetached(FFHttpRequest *request);
private:
FFHttpRequest *request_;
====
//depot/googleclient/gears/opensource/gears/localserver/safari/http_request_sf.mm#23
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/localserver/safari/http_request_sf.mm
====
# action=edit type=text
--- googleclient/gears/opensource/gears/localserver/safari/http_request_sf.mm
2008-09-08 17:25:21.000000000 +1000
+++ googleclient/gears/opensource/gears/localserver/safari/http_request_sf.mm
2008-09-08 17:49:45.000000000 +1000
@@ -34,6 +34,7 @@
#import "gears/base/common/atomic_ops.h"
#import "gears/base/common/http_utils.h"
+#import "gears/base/common/leak_counter.h"
#import "gears/base/common/string_utils.h"
#import "gears/base/common/url_utils.h"
#import "gears/base/npapi/browser_utils.h"
@@ -99,10 +100,15 @@
was_redirected_(false),
async_(false) {
+ LEAK_COUNTER_INCREMENT(SFHttpRequest);
delegate_holder_ = new HttpRequestData;
}
SFHttpRequest::~SFHttpRequest() {
+ LEAK_COUNTER_DECREMENT(SFHttpRequest);
+ if (delegate_holder_ && delegate_holder_->post_data_stream) {
+ [delegate_holder_->post_data_stream onSFHttpRequestDetached:this];
+ }
delete delegate_holder_;
}
@@ -511,6 +517,9 @@
// Reset
//------------------------------------------------------------------------------
void SFHttpRequest::Reset() {
+ if (delegate_holder_ && delegate_holder_->post_data_stream) {
+ [delegate_holder_->post_data_stream onSFHttpRequestDetached:this];
+ }
[delegate_holder_->delegate abort];
[delegate_holder_->delegate release];
delegate_holder_->delegate = NULL;
====
//depot/googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.h#5
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.h
====
# action=edit type=text
---
googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.h
2008-09-08 17:25:21.000000000 +1000
+++
googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.h
2008-09-08 17:22:26.000000000 +1000
@@ -33,6 +33,12 @@
// ProgressInputStream is a wrapper around an NSInputStream that provides
// notification to the native HttpRequest object as the stream is read.
+//
+// This class should only be created by a SFHttpRequest, and that SFHttpRequest
+// should call OnSFHttpRequestDetached whenever the SFHttpRequest no longer
+// points to this ProgressInputStream (e.g. during SFHttpRequest's destructor).
+// This constraint is in order to let the two objects point to each other
+// without causing a reference cycle that prohibits deleting them after use.
@interface ProgressInputStream: NSInputStream {
@private
SFHttpRequest *request_;
@@ -49,5 +55,7 @@
request:(SFHttpRequest *)request
total:(int64)total;
+- (void)onSFHttpRequestDetached:(SFHttpRequest *)request;
+
@end
#endif // GEARS_LOCALSERVER_SAFARI_PROGRESS_INPUT_STREAM_H__
====
//depot/googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.mm#6
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.mm
====
# action=edit type=text
---
googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.mm
2008-09-08 17:25:21.000000000 +1000
+++
googleclient/gears/opensource/gears/localserver/safari/progress_input_stream.mm
2008-09-08 17:18:49.000000000 +1000
@@ -25,6 +25,7 @@
#import "gears/localserver/safari/progress_input_stream.h"
+#import "gears/base/common/leak_counter.h"
#import "gears/localserver/common/progress_event.h"
#import "gears/localserver/safari/http_request_sf.h"
@@ -38,18 +39,24 @@
- (id)initFromStream:(NSInputStream *)input_stream
request:(SFHttpRequest *)request
total:(int64)total {
+ assert(request);
self = [super init];
if (self != nil) {
+ LEAK_COUNTER_INCREMENT(ProgressInputStream);
input_stream_ = [input_stream retain];
request_ = request;
- request_->Ref();
total_ = total;
}
return self;
}
+- (void)onSFHttpRequestDetached:(SFHttpRequest *)request {
+ assert(request == request_);
+ request_ = NULL;
+}
+
- (void)dealloc {
- request_->Unref();
+ LEAK_COUNTER_DECREMENT(ProgressInputStream);
[input_stream_ release];
[super dealloc];
}
@@ -60,7 +67,9 @@
NSInteger bytes_read = [input_stream_ read:buffer maxLength:len];
if (bytes_read > 0) {
position_ += bytes_read;
- ProgressEvent::Update(request_, request_, position_, total_);
+ if (request_) {
+ ProgressEvent::Update(request_, request_, position_, total_);
+ }
}
return bytes_read;
}
