Fix memory leak in ~GestureRecognizerImpl

~GestureRecognizerImpl() was leaking memory.  Now it doesn't.  An unfortunate side effect of this leak was that GestureSequence objects hold a pointer to the GR's parent window (helper_) that is no longer valid after the dtor is called.  They also hold a callback timer, which fires for a long press...

This CL fixes the crash in issue 139420, but it doesn't fix the full bug.  Specifically, the popup menu is still not responding to touch.

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

[girard, 2012-08-02 17:14:20]

Replaced raw pointer with scoped_ptr.  Ensures that objects will be cleaned up
in a timely manner.

https://chromiumcodereview.appspot.com/10826117/diff/1/ui/base/gestures/gestu...
File ui/base/gestures/gesture_recognizer_impl.cc (right):

https://chromiumcodereview.appspot.com/10826117/diff/1/ui/base/gestures/gestu...
ui/base/gestures/gesture_recognizer_impl.cc:146: map->erase(current_consumer);
Partial template specialization (from generic TransferConsumer, above).

https://chromiumcodereview.appspot.com/10826117/diff/1/ui/base/gestures/gestu...
File ui/base/gestures/gesture_recognizer_impl.h (right):

https://chromiumcodereview.appspot.com/10826117/diff/1/ui/base/gestures/gestu...
ui/base/gestures/gesture_recognizer_impl.h:62: std::map<GestureConsumer*,
scoped_ptr<GestureSequence>> consumer_sequence_;
Changed to scoped_ptr.  This ensures that during  ~GestureRecognizerImpl all of
these GestureSequence objects will be deleted.  The GestureSequence contains a
timed callback, and a pointer to a helper.  The helper pointer is not valid
after the GR's destruction.

[sadrul, 2012-08-02 18:29:55]

http://codereview.chromium.org/10826117/diff/1/ui/base/gestures/gesture_recog...
File ui/base/gestures/gesture_recognizer_impl.cc (right):

http://codereview.chromium.org/10826117/diff/1/ui/base/gestures/gesture_recog...
ui/base/gestures/gesture_recognizer_impl.cc:345: event_queue_.erase(consumer);
Perhaps we need to do the same for event_queue_ too? (we can probably also get
rid of the old TransferConsumer function.

[girard, 2012-08-02 19:33:53]

Refactored previous CL to deal with more raw pointers.  scoped_ptr's FTW.

http://codereview.chromium.org/10826117/diff/1/ui/base/gestures/gesture_recog...
File ui/base/gestures/gesture_recognizer_impl.cc (right):

http://codereview.chromium.org/10826117/diff/1/ui/base/gestures/gesture_recog...
ui/base/gestures/gesture_recognizer_impl.cc:345: event_queue_.erase(consumer);
On 2012/08/02 18:29:55, sadrul wrote:
> Perhaps we need to do the same for event_queue_ too? (we can probably also get
> rid of the old TransferConsumer function.

That makes sense.  I've updated the CL (second patch) to do this, and then added
a third patch that also changes TouchEventQueue.  I think this last patch cleans
up everything.

[sadrul, 2012-08-02 21:01:12]

The patch doesn't seem to compile on linux-chromeos. Would you please update the
patch with build fixes?

[girard, 2012-08-03 14:55:34]

On 2012/08/02 21:01:12, sadrul wrote:
> The patch doesn't seem to compile on linux-chromeos. Would you please update
the
> patch with build fixes?

The problem appears to be that std::map's iterator creates a pair<K,T>, and that
requires that T be copiable.  In our case, T is a scoped_ptr<TouchEvent>, which
has no public copy c'tor.

I'm surprised that this compiled under so many platforms.... I guess they have
different implementations of the standard library.

I'll fix this by going back to raw pointers, and adding manual cleanup in
~GestureRecognizerImpl for any remaining contents.

[girard, 2012-08-03 18:41:51]

This CL is much simpler.... we now add clean-up logic into
~GestureRecognizerImpl().

[sadrul, 2012-08-03 19:58:52]

I think this looks good. +sky in case there is a better way of doing this.

[sky, 2012-08-03 20:05:06]

http://codereview.chromium.org/10826117/diff/8002/ui/base/gestures/gesture_re...
File ui/base/gestures/gesture_recognizer_impl.cc (right):

http://codereview.chromium.org/10826117/diff/8002/ui/base/gestures/gesture_re...
ui/base/gestures/gesture_recognizer_impl.cc:174: for (i =
consumer_sequence_.begin(); i != consumer_sequence_.end(); ++i)
Move iterators inside the for loop, that way you don't end up with i2. That
said, I believe STLDeleteValues does what you want (in base/stl_util.h).

[girard, 2012-08-03 20:14:45]

Updated as per sky's review.  Much cleaner, and shorter.

http://codereview.chromium.org/10826117/diff/8002/ui/base/gestures/gesture_re...
File ui/base/gestures/gesture_recognizer_impl.cc (right):

http://codereview.chromium.org/10826117/diff/8002/ui/base/gestures/gesture_re...
ui/base/gestures/gesture_recognizer_impl.cc:174: for (i =
consumer_sequence_.begin(); i != consumer_sequence_.end(); ++i)
On 2012/08/03 20:05:07, sky wrote:
> Move iterators inside the for loop, that way you don't end up with i2. That
> said, I believe STLDeleteValues does what you want (in base/stl_util.h).
Nice!  I was hoping there might be a utility to take care of this.

[sadrul, 2012-08-03 20:16:42]

LGTM

http://codereview.chromium.org/10826117/diff/8002/ui/base/gestures/gesture_re...
File ui/base/gestures/gesture_recognizer_impl.cc (right):

http://codereview.chromium.org/10826117/diff/8002/ui/base/gestures/gesture_re...
ui/base/gestures/gesture_recognizer_impl.cc:174: for (i =
consumer_sequence_.begin(); i != consumer_sequence_.end(); ++i)
On 2012/08/03 20:05:07, sky wrote:
> Move iterators inside the for loop, that way you don't end up with i2. That
> said, I believe STLDeleteValues does what you want (in base/stl_util.h).

Nice! Thanks!

[commit-bot: I haz the power, 2012-08-03 20:28:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/10826117/7

[commit-bot: I haz the power, 2012-08-03 23:02:01]

Change committed as 149957