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

Add a lock to the cookie accountant to prevent race conditions in patching
functions from different threads.

BUG=64844
TEST=Run integration tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67926

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

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
File ceee/ie/plugin/bho/cookie_accountant.cc (right):

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
ceee/ie/plugin/bho/cookie_accountant.cc:38: AutoLock lock(lock_);
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...

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
ceee/ie/plugin/bho/cookie_accountant.cc:226: AutoLock lock(lock_);
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]

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
File ceee/ie/plugin/bho/cookie_accountant.cc (right):

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
ceee/ie/plugin/bho/cookie_accountant.cc:38: AutoLock lock(lock_);
On 2010/11/30 22:35:55, MAD wrote:
> 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...

Done.

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
ceee/ie/plugin/bho/cookie_accountant.cc:226: AutoLock lock(lock_);
On 2010/11/30 22:35:55, MAD wrote:
> 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...

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]

Getting closer... :-)

A suggestion below...

BYE
MAD

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
File ceee/ie/plugin/bho/cookie_accountant.cc (right):

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
ceee/ie/plugin/bho/cookie_accountant.cc:226: AutoLock lock(lock_);
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]

drive-by

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
File ceee/ie/plugin/bho/cookie_accountant.cc (right):

http://codereview.chromium.org/5290011/diff/1/ceee/ie/plugin/bho/cookie_accou...
ceee/ie/plugin/bho/cookie_accountant.cc:226: AutoLock lock(lock_);
On 2010/12/01 02:14:43, MAD wrote:
> 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

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.

> 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... :-)

[MAD, 2010-12-01 14:55:38]

OK, then, based on Siggi's comments... LGTM!

We can keep it as is...

Thanks!

BYE
MAD

[Sigurður Ásgeirsson, 2010-12-01 15:01:11]

Also, after re-reading the rewritten vtable patching code and a discussion
with MAD, we've come to the realization that increasing concurrency here is
really bad, because there's a race on the VM operations (which happen to be
serializing).

On Wed, Dec 1, 2010 at 9:55 AM, <mad@chromium.org> wrote:

> OK, then, based on Siggi's comments... LGTM!
>
> We can keep it as is...
>
> Thanks!
>
> BYE
> MAD
>
>
>
> http://codereview.chromium.org/5290011/
>