Add a lock to the cookie accountant to prevent race conditions in patching...

ceee/ie/plugin/bho/cookie_accountant.cc

 // Copyright (c) 2010 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.
 //
 // CookieAccountant implementation.
 #include "ceee/ie/plugin/bho/cookie_accountant.h"
 
 #include <atlbase.h>
 #include <wininet.h>
 
@@ skipping 17 matching lines @@
 static const wchar_t kMsHtmlModuleName[] = L"mshtml.dll";
 static const char kWinInetModuleName[] = "wininet.dll";
 static const char kInternetSetCookieExAFunctionName[] = "InternetSetCookieExA";
 static const char kInternetSetCookieExWFunctionName[] = "InternetSetCookieExW";
 
 }  // namespace
 
 CookieAccountant* CookieAccountant::singleton_instance_ = NULL;
 
 CookieAccountant::~CookieAccountant() {
+  AutoLock lock(lock_);

[MAD, 2010/11/30 22:35:55]

I don't think a lock is necessary in the destructor since nobody should be
making any calls on us while we are being destroyed otherwise we would have even
more problems...

[Cindy Lau, 2010/12/01 01:33:15]

Done.

   if (internet_set_cookie_ex_a_patch_.is_patched())
     internet_set_cookie_ex_a_patch_.Unpatch();
   if (internet_set_cookie_ex_w_patch_.is_patched())
     internet_set_cookie_ex_w_patch_.Unpatch();
 }
 
 ProductionCookieAccountant::ProductionCookieAccountant() {
 }
 
 CookieAccountant* CookieAccountant::GetInstance() {
@@ skipping 167 matching lines @@
       continue;  // Skip non-cookie headers.
     std::string cookie_data = header_line.substr(
         name_pos + std::string(kSetCookieHeaderName).size());
     // TODO(cindylau@chromium.org): Get the URL for the HTTP request from
     // IHttpNegotiate::BeginningTransaction.
     RecordCookie(std::string(), cookie_data, current_time);
   }
 }
 
 void CookieAccountant::PatchWininetFunctions() {
+  AutoLock lock(lock_);

[MAD, 2010/11/30 22:35:55]

I don't think it's a good idea to call Patch from within a lock since it can do
a LoadLibrary and it may depend on another thread that may depend on us...

I think we need to do a bit more work here... For example, use a member variable
that we protect with the lock that identifies whether we are going to patch or
if someone else is responsible for it...

[Cindy Lau, 2010/12/01 01:33:15]

Okay, I rewrote it not to block other threads coming in while patching. However,
this will mean that some threads may exit this function without the patches
being completed, which could mean we lose some initial cookie events on startup.
 I'm not sure how that problem could be solved without forcing threads to block.
 What do you think?

[MAD, 2010/12/01 02:14:43]

I think the chances are pretty slim since IE still has to setup the BHO before
doing anything in the BHO thread so it's unlikely that thread will make cooki
changes... If other threads make cookie changes, we'll not be worse then we
would be with a lock, right?

One thing that might minimize that chance and also improve the speed a bit would
be to not rely on !internet_set_cookie_ex_a_patch_.is_patched()to decide if we
will patch or not, but to keep the boolean member set to true forever, or maybe
reset it to false if we fail... so we could keep two, one for A and one for W,
what do you think?

This way, we could even get the two racing threads working in parallel, with
even less chances of missing anything... :-)

[Sigurður Ásgeirsson, 2010/12/01 14:36:40]

is_patched() is an inline accessor to a pointer, which is going to be just as
fast as a boolean check. Adding more state will not improve speed - which is not
a concern in practice here - and will only complicate the code. 

If we're seeing crashes here, it's likely because other parties are messing with
the same page protections concurrently to us, and a fix like this:

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome_frame/...

is what we really need. I rewrote that particular file to get rid of races that
were causing crashes with GCF, so this is not a hypothetical problem.

   if (!internet_set_cookie_ex_a_patch_.is_patched()) {
     DWORD error = internet_set_cookie_ex_a_patch_.Patch(
         kMsHtmlModuleName, kWinInetModuleName,
         kInternetSetCookieExAFunctionName, InternetSetCookieExAPatch);
     DCHECK(error == NO_ERROR || !internet_set_cookie_ex_a_patch_.is_patched());
   }
   if (!internet_set_cookie_ex_w_patch_.is_patched()) {
     DWORD error = internet_set_cookie_ex_w_patch_.Patch(
         kMsHtmlModuleName, kWinInetModuleName,
         kInternetSetCookieExWFunctionName, InternetSetCookieExWPatch);
     DCHECK(error == NO_ERROR || !internet_set_cookie_ex_w_patch_.is_patched());
   }
   DCHECK(internet_set_cookie_ex_a_patch_.is_patched() ||
          internet_set_cookie_ex_w_patch_.is_patched());
 }