On Mon, Sep 15, 2008 at 7:26 PM, Nigel Tao <[EMAIL PROTECTED]> wrote:
> ======================================================================== > > http://mondrian.corp.google.com/file/8201059///depot/googleclient/gears/opensource/gears/localserver/ie/http_request_ie.cc?a=1 > File > //depot/googleclient/gears/opensource/gears/localserver/ie/http_request_ie.cc > (snapshot 1) > ------------------------------------ > Line 567: info->stgmedData.pstm = static_cast<IStream*>(stream); > On Mon Sep 15 12:17:51 2008 PDT, michaeln wrote: > > Two things... > > > > 1) Now that we've added the ReleaseBindInfo call to the handler, I > question > > whether we're returning the stream properly here? > > > > If the caller is expected to call ReleaseBindInfo which is expected to > release > > the stream, that leads me to believe that we should AddRef the stream we > return > > here. > > Don't we implicitly AddRef the stream in the next line down, > "post_data_stream_ > = stream", since post_data_stream_ is a CComPtr, which implicitly AddRef's > and > Release's? Yes, and then the post_data_stream_ smartptr releases that reference. But now we've added an additional call to Release in the system (via ReleaseBindInfo) that we should balance with an AddRef. Some history... When the YahooToolbar is installed, the HttpHandler is effectively not registered as a protocol handler so is not called. When GHR was used to POST data in this scenario, it crashed. The data stream was being released on too many times. GHR was modified to be resilient to this. The GHR.post_data_stream_ smartptr was put in place and the PostDataStreams dtor was modified to drop that reference w/o calling Release if the object was being destructed while we still held that reference. This quick fix works when our HttpHandler is in place or has been overriden by the YT. But I think the quick fix was not correct. It looks like the implementation of GetBindInfo should AddRef the stream being returned, and the caller of GetBindInfo should ReleaseBindInfo when done. Without the AddRef, we're depending on our quick fix to work around the extra release. I think the post_data_stream_ smartptr, and ~ProgressInputStream logic to drop it, don't need to be there. > > > > > 2) With YahooToolbar installed, our handler is NOT registered, and things > are > > not well behaved. Can you also run thru the unit tests with YT installed > and > > check for leaks/crashes in that case. > > I'm on IE6 / WinXP, and installing the YahooToolbar causes the localserver > tests > to fail quite spectacularly (the other tests, including database and > workerpool, > seem fine), both with and without my ReleaseBindInfo patch. And, > occasionally > Gears + YahooToolbar will trigger an assertion failure (that > downloading_version > == completed_version, on line 136 of localserver/common/update_task.cc). The yahoo toolbar completely breaks the Gears localserver, a known bug that we have no answer for at this time. I don't think we can solve this problem w/o working with others that plug the HTTP protocol in IE. So long as GHR functions properly with YT installed we OK for now. > > So, Gears + YahooToolbar + LocalServer usage = test failure and occasional > crashes, but that was happening anyway, before this CL... > ======================================================================== > > -- > To respond, reply to this email or visit > http://mondrian.corp.google.com/8201059 >
