Issue 23702008: Adds the UserInputMonitor implementation for Windows.

jiayl

PTAL. Thanks!

7 years, 3 months ago (2013-08-28 23:02:01 UTC) #1

DaleCurtis

https://codereview.chromium.org/23702008/diff/1/media/base/user_input_monitor_unittest.cc File media/base/user_input_monitor_unittest.cc (right): https://codereview.chromium.org/23702008/diff/1/media/base/user_input_monitor_unittest.cc#newcode88 media/base/user_input_monitor_unittest.cc:88: base::Thread io_thread("UserInputMonitorTestIOThread"); Do you actually need these threads? Can ...

7 years, 3 months ago (2013-08-29 19:02:41 UTC) #2

jiayl

https://codereview.chromium.org/23702008/diff/1/media/base/user_input_monitor_unittest.cc File media/base/user_input_monitor_unittest.cc (right): https://codereview.chromium.org/23702008/diff/1/media/base/user_input_monitor_unittest.cc#newcode88 media/base/user_input_monitor_unittest.cc:88: base::Thread io_thread("UserInputMonitorTestIOThread"); On 2013/08/29 19:02:42, DaleCurtis wrote: > Do ...

7 years, 3 months ago (2013-08-29 21:05:39 UTC) #3

jiayl

Sergey/James, could you review the Windows specific code? On Thu, Aug 29, 2013 at 2:05 ...

7 years, 3 months ago (2013-09-03 18:36:54 UTC) #4

Wez

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc#newcode31 media/base/user_input_monitor_win.cc:31: virtual size_t GetKeyPressCount() const OVERRIDE; nit: Suggest comment "UserInputMonitor ...

7 years, 3 months ago (2013-09-03 20:07:04 UTC) #5

jiayl

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc#newcode101 media/base/user_input_monitor_win.cc:101: base::Unretained(this), On 2013/09/03 20:07:04, Wez wrote: > This will ...

7 years, 3 months ago (2013-09-03 21:41:28 UTC) #6

DaleCurtis

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc#newcode101 media/base/user_input_monitor_win.cc:101: base::Unretained(this), On 2013/09/03 21:41:28, jiayl wrote: > On 2013/09/03 ...

7 years, 3 months ago (2013-09-03 22:59:32 UTC) #7

jiayl

On 2013/09/03 22:59:32, DaleCurtis wrote: > https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc > File media/base/user_input_monitor_win.cc (right): > > https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc#newcode101 > ...

7 years, 3 months ago (2013-09-03 23:01:51 UTC) #8

jiayl

PTAL. sky, could you review the change in browser_main_loop.h? https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc#newcode31 media/base/user_input_monitor_win.cc:31: ...

7 years, 3 months ago (2013-09-03 23:44:12 UTC) #9

PTAL. 

sky, could you review the change in browser_main_loop.h?

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
File media/base/user_input_monitor_win.cc (right):

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:31: virtual size_t GetKeyPressCount() const
OVERRIDE;
On 2013/09/03 20:07:04, Wez wrote:
> nit: Suggest comment "UserInputMonitor public interface"

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:38: static int
NumberOfRawInputDevices(uint8 target_events);
On 2013/09/03 20:07:04, Wez wrote:
> nit: |target_events| -> |event_mask| to make it clearer that this should be a
> combination of EventBitMask values? Or add a comment to explain the method's
> usage.

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:40: virtual void StartMouseMonitoring()
OVERRIDE;
On 2013/09/03 20:07:04, Wez wrote:
> nit: Suggest comment "UserInputMonitor private interface" to introduce this
> block.

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:40: virtual void StartMouseMonitoring()
OVERRIDE;
On 2013/09/03 20:07:04, Wez wrote:
> nit: Suggest comment "UserInputMonitor private interface" to introduce this
> block.

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:59: RAWINPUTDEVICE*
GetRawInputDevices(uint8 target_events, DWORD flags);
On 2013/09/03 20:07:04, Wez wrote:
> If you make this return std::vector<RAWINPUTDEVICE> then you don't end up
> returning a bare pointer, and you fold NummberOfRawInputDevices() into
> GetRawInputDevices.
> 
> (If you're worried about the vector copying overhead it could return a
> scoped_ptr<std::vector<...> >)

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:65: // Members only accessed on the UI
thread.
On 2013/09/03 20:07:04, Wez wrote:
> If all the methods above are called only on the UI message loop, and all these
> members are called only on the UI thread, why do you need these comments?

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:101: base::Unretained(this),
On 2013/09/03 20:07:04, Wez wrote:
> This will use-after-free if the StartMouseMonitoring() is called, and the
caller
> then deletes the UserInputMonitor immediately on the calling thread, since the
> task will still be processed on the UI thread.

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:105: StartMonitor(MOUSE_EVENT_MASK);
On 2013/09/03 20:07:04, Wez wrote:
> This path will never be hit unless the caller is also on the UI thread. Better
> to have the PostTask, above, re-post StartMouseMonitoring().

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:142:
DCHECK(ui_task_runner_->BelongsToCurrentThread());
On 2013/09/03 20:07:04, Wez wrote:
> nit: I prefer pre-condition DCHECKs to be followed by a blank line before the
> rest of the method logic, for ease of readability.

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:151: window_.reset(new
base::win::MessageWindow());
On 2013/09/03 20:07:04, Wez wrote:
> Why re-create the window every time a new event is requested? Can't you just
> create the window if |events_monitored_| was empty, and call
> RegisterRawInputDevices() to start it receiving events?
> 
> You're also never un-registering for notifications to the old window.

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:165: if (window_) {
On 2013/09/03 20:07:04, Wez wrote:
> Clarify that |window_| may be NULL if we failed to monitor events.

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:169: // The error is harmless, ignore it.
On 2013/09/03 20:07:04, Wez wrote:
> Which error? Why not trap & at least LOG_SYSERR(WARNING) it?

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:183: input_handle, RID_INPUT, NULL, &size,
sizeof(RAWINPUTHEADER));
On 2013/09/03 20:07:04, Wez wrote:
> nit: Verify that |result| is not greater than zero here, or some other -ve
value
> than -1?

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:194: if (result == -1) {
On 2013/09/03 20:07:04, Wez wrote:
> nit: See above re verifying |result|

Done.

https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:230: if
(RegisterRawInputDevices(devices.get(),
On 2013/09/03 20:07:04, Wez wrote:
> Have you checked the semantics if e.g. two separate calls are made to
> RegisterRawInputDevices() that direct the same type of input to different
> windows, to know how this will interact w/ other users of the API in Chrome?
Hmm...I verified that only one window will receive the event if multiple ones
are registered. Chrome has no other callers though.

Wez

On 2013/09/03 23:01:51, jiayl wrote: > On 2013/09/03 22:59:32, DaleCurtis wrote: > > > https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc ...

7 years, 3 months ago (2013-09-04 00:07:51 UTC) #10

Wez

On 2013/09/03 23:44:12, jiayl wrote: > PTAL. > > sky, could you review the change ...

7 years, 3 months ago (2013-09-04 00:08:51 UTC) #11

On 2013/09/03 23:44:12, jiayl wrote:
> PTAL. 
> 
> sky, could you review the change in browser_main_loop.h?
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> File media/base/user_input_monitor_win.cc (right):
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:31: virtual size_t GetKeyPressCount()
const
> OVERRIDE;
> On 2013/09/03 20:07:04, Wez wrote:
> > nit: Suggest comment "UserInputMonitor public interface"
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:38: static int
> NumberOfRawInputDevices(uint8 target_events);
> On 2013/09/03 20:07:04, Wez wrote:
> > nit: |target_events| -> |event_mask| to make it clearer that this should be
a
> > combination of EventBitMask values? Or add a comment to explain the method's
> > usage.
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:40: virtual void StartMouseMonitoring()
> OVERRIDE;
> On 2013/09/03 20:07:04, Wez wrote:
> > nit: Suggest comment "UserInputMonitor private interface" to introduce this
> > block.
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:40: virtual void StartMouseMonitoring()
> OVERRIDE;
> On 2013/09/03 20:07:04, Wez wrote:
> > nit: Suggest comment "UserInputMonitor private interface" to introduce this
> > block.
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:59: RAWINPUTDEVICE*
> GetRawInputDevices(uint8 target_events, DWORD flags);
> On 2013/09/03 20:07:04, Wez wrote:
> > If you make this return std::vector<RAWINPUTDEVICE> then you don't end up
> > returning a bare pointer, and you fold NummberOfRawInputDevices() into
> > GetRawInputDevices.
> > 
> > (If you're worried about the vector copying overhead it could return a
> > scoped_ptr<std::vector<...> >)
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:65: // Members only accessed on the UI
> thread.
> On 2013/09/03 20:07:04, Wez wrote:
> > If all the methods above are called only on the UI message loop, and all
these
> > members are called only on the UI thread, why do you need these comments?
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:101: base::Unretained(this),
> On 2013/09/03 20:07:04, Wez wrote:
> > This will use-after-free if the StartMouseMonitoring() is called, and the
> caller
> > then deletes the UserInputMonitor immediately on the calling thread, since
the
> > task will still be processed on the UI thread.
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:105: StartMonitor(MOUSE_EVENT_MASK);
> On 2013/09/03 20:07:04, Wez wrote:
> > This path will never be hit unless the caller is also on the UI thread.
Better
> > to have the PostTask, above, re-post StartMouseMonitoring().
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:142:
> DCHECK(ui_task_runner_->BelongsToCurrentThread());
> On 2013/09/03 20:07:04, Wez wrote:
> > nit: I prefer pre-condition DCHECKs to be followed by a blank line before
the
> > rest of the method logic, for ease of readability.
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:151: window_.reset(new
> base::win::MessageWindow());
> On 2013/09/03 20:07:04, Wez wrote:
> > Why re-create the window every time a new event is requested? Can't you just
> > create the window if |events_monitored_| was empty, and call
> > RegisterRawInputDevices() to start it receiving events?
> > 
> > You're also never un-registering for notifications to the old window.
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:165: if (window_) {
> On 2013/09/03 20:07:04, Wez wrote:
> > Clarify that |window_| may be NULL if we failed to monitor events.
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:169: // The error is harmless, ignore it.
> On 2013/09/03 20:07:04, Wez wrote:
> > Which error? Why not trap & at least LOG_SYSERR(WARNING) it?
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:183: input_handle, RID_INPUT, NULL,
&size,
> sizeof(RAWINPUTHEADER));
> On 2013/09/03 20:07:04, Wez wrote:
> > nit: Verify that |result| is not greater than zero here, or some other -ve
> value
> > than -1?
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:194: if (result == -1) {
> On 2013/09/03 20:07:04, Wez wrote:
> > nit: See above re verifying |result|
> 
> Done.
> 
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> media/base/user_input_monitor_win.cc:230: if
> (RegisterRawInputDevices(devices.get(),
> On 2013/09/03 20:07:04, Wez wrote:
> > Have you checked the semantics if e.g. two separate calls are made to
> > RegisterRawInputDevices() that direct the same type of input to different
> > windows, to know how this will interact w/ other users of the API in Chrome?
> Hmm...I verified that only one window will receive the event if multiple ones
> are registered. Chrome has no other callers though.

Do you only create a single UserInputMonitor no matter how many capture streams
are active, then?

jiayl

On 2013/09/04 00:08:51, Wez wrote: > On 2013/09/03 23:44:12, jiayl wrote: > > PTAL. > ...

7 years, 3 months ago (2013-09-04 00:10:28 UTC) #12

On 2013/09/04 00:08:51, Wez wrote:
> On 2013/09/03 23:44:12, jiayl wrote:
> > PTAL. 
> > 
> > sky, could you review the change in browser_main_loop.h?
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > File media/base/user_input_monitor_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:31: virtual size_t GetKeyPressCount()
> const
> > OVERRIDE;
> > On 2013/09/03 20:07:04, Wez wrote:
> > > nit: Suggest comment "UserInputMonitor public interface"
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:38: static int
> > NumberOfRawInputDevices(uint8 target_events);
> > On 2013/09/03 20:07:04, Wez wrote:
> > > nit: |target_events| -> |event_mask| to make it clearer that this should
be
> a
> > > combination of EventBitMask values? Or add a comment to explain the
method's
> > > usage.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:40: virtual void StartMouseMonitoring()
> > OVERRIDE;
> > On 2013/09/03 20:07:04, Wez wrote:
> > > nit: Suggest comment "UserInputMonitor private interface" to introduce
this
> > > block.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:40: virtual void StartMouseMonitoring()
> > OVERRIDE;
> > On 2013/09/03 20:07:04, Wez wrote:
> > > nit: Suggest comment "UserInputMonitor private interface" to introduce
this
> > > block.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:59: RAWINPUTDEVICE*
> > GetRawInputDevices(uint8 target_events, DWORD flags);
> > On 2013/09/03 20:07:04, Wez wrote:
> > > If you make this return std::vector<RAWINPUTDEVICE> then you don't end up
> > > returning a bare pointer, and you fold NummberOfRawInputDevices() into
> > > GetRawInputDevices.
> > > 
> > > (If you're worried about the vector copying overhead it could return a
> > > scoped_ptr<std::vector<...> >)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:65: // Members only accessed on the UI
> > thread.
> > On 2013/09/03 20:07:04, Wez wrote:
> > > If all the methods above are called only on the UI message loop, and all
> these
> > > members are called only on the UI thread, why do you need these comments?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:101: base::Unretained(this),
> > On 2013/09/03 20:07:04, Wez wrote:
> > > This will use-after-free if the StartMouseMonitoring() is called, and the
> > caller
> > > then deletes the UserInputMonitor immediately on the calling thread, since
> the
> > > task will still be processed on the UI thread.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:105: StartMonitor(MOUSE_EVENT_MASK);
> > On 2013/09/03 20:07:04, Wez wrote:
> > > This path will never be hit unless the caller is also on the UI thread.
> Better
> > > to have the PostTask, above, re-post StartMouseMonitoring().
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:142:
> > DCHECK(ui_task_runner_->BelongsToCurrentThread());
> > On 2013/09/03 20:07:04, Wez wrote:
> > > nit: I prefer pre-condition DCHECKs to be followed by a blank line before
> the
> > > rest of the method logic, for ease of readability.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:151: window_.reset(new
> > base::win::MessageWindow());
> > On 2013/09/03 20:07:04, Wez wrote:
> > > Why re-create the window every time a new event is requested? Can't you
just
> > > create the window if |events_monitored_| was empty, and call
> > > RegisterRawInputDevices() to start it receiving events?
> > > 
> > > You're also never un-registering for notifications to the old window.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:165: if (window_) {
> > On 2013/09/03 20:07:04, Wez wrote:
> > > Clarify that |window_| may be NULL if we failed to monitor events.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:169: // The error is harmless, ignore
it.
> > On 2013/09/03 20:07:04, Wez wrote:
> > > Which error? Why not trap & at least LOG_SYSERR(WARNING) it?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:183: input_handle, RID_INPUT, NULL,
> &size,
> > sizeof(RAWINPUTHEADER));
> > On 2013/09/03 20:07:04, Wez wrote:
> > > nit: Verify that |result| is not greater than zero here, or some other -ve
> > value
> > > than -1?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:194: if (result == -1) {
> > On 2013/09/03 20:07:04, Wez wrote:
> > > nit: See above re verifying |result|
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_moni...
> > media/base/user_input_monitor_win.cc:230: if
> > (RegisterRawInputDevices(devices.get(),
> > On 2013/09/03 20:07:04, Wez wrote:
> > > Have you checked the semantics if e.g. two separate calls are made to
> > > RegisterRawInputDevices() that direct the same type of input to different
> > > windows, to know how this will interact w/ other users of the API in
Chrome?
> > Hmm...I verified that only one window will receive the event if multiple
ones
> > are registered. Chrome has no other callers though.
> 
> Do you only create a single UserInputMonitor no matter how many capture
streams
> are active, then?

Correct.

jiayl

On 2013/09/04 00:07:51, Wez wrote: > On 2013/09/03 23:01:51, jiayl wrote: > > On 2013/09/03 ...

7 years, 3 months ago (2013-09-04 00:12:14 UTC) #13

Wez

On 3 September 2013 17:12, <jiayl@chromium.org> wrote: > On 2013/09/04 00:07:51, Wez wrote: > >> ...

7 years, 3 months ago (2013-09-04 00:21:23 UTC) #14

On 3 September 2013 17:12, <jiayl@chromium.org> wrote:

> On 2013/09/04 00:07:51, Wez wrote:
>
>> On 2013/09/03 23:01:51, jiayl wrote:
>> > On 2013/09/03 22:59:32, DaleCurtis wrote:
>> > >
>> >
>>
>
> https://codereview.chromium.**org/23702008/diff/8001/media/**
>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc>
>
>> > > File media/base/user_input_monitor_**win.cc (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.**org/23702008/diff/8001/media/**
>
base/user_input_monitor_win.**cc#newcode101<https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc#newcode101>
>
>> > > media/base/user_input_monitor_**win.cc:101: base::Unretained(this),
>>
>> > > On 2013/09/03 21:41:28, jiayl wrote:
>> > > > On 2013/09/03 20:07:04, Wez wrote:
>> > > > > This will use-after-free if the StartMouseMonitoring() is called,
>> and
>> the
>> > > > caller
>> > > > > then deletes the UserInputMonitor immediately on the calling
>> thread,
>> since
>> > > the
>> > > > > task will still be processed on the UI thread.
>> > > >
>> > > > You are right. I was assuming the caller should make sure
>>
> UserInputMonitor
>
>> > is
>> > > > not deleted too early, but that seems not a good assumption. I'm
>> going
>>
> to
>
>> > > change
>> > > > UserInputMonitor to ref counted. WeakPtr won't work since
>>
> UserInputMonitor
>
>> > is
>> > > > destroyed and called back on different threads.
>> > >
>> > > Isn't it invalid for a client to call StartMouseMonitoring() then
>> delete
>> > without
>> > > calling StopMouseMonitoring() ? If we can avoid ref-counting anything
>>
> that'd
>
>> > be
>> > > great.
>> >
>> > That's true. But if StopMouseMonitoring is called right before
>> UserInputMonitor,
>> > the queued async events on the UI thread may still call back to the
>>
> destroyed
>
>> > UserInputMonitor.
>>
>
>  Not if those events were bound to WeakPtrs to the UserInputMonitor. :)
>>
>
> But the WeakPtrs need to be deref'd on the callback thread while
> UserInputMonitor is destroyed on the main thread, which means WeakPtrs are
> invalidated and deref'd on different threads.


Which are the "callback" and "main" threads wrt this CL? You mean the
|ui_task_runner| and calling threads?

Looking at UserInputMonitor's implementation, I'd assumed you must punt the
mouse notifications back to the calling thread for dispatch, to avoid a
race between notifications and calls to RemoveMouseListener(), which case
you;d be dereferencing the WeakPtr on the caller thread, which would work.

It looks like you're instead relying on locking around the observer-list
modifications, and observer notifications - are you sure that's a good
idea? It means that one badly-behaved observer can indirectly block the UI
thread. :(


>
>
https://codereview.chromium.**org/23702008/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

jiayl

On 2013/09/04 00:21:23, Wez wrote: > On 3 September 2013 17:12, <mailto:jiayl@chromium.org> wrote: > > ...

7 years, 3 months ago (2013-09-04 00:38:43 UTC) #15

On 2013/09/04 00:21:23, Wez wrote:
> On 3 September 2013 17:12, <mailto:jiayl@chromium.org> wrote:
> 
> > On 2013/09/04 00:07:51, Wez wrote:
> >
> >> On 2013/09/03 23:01:51, jiayl wrote:
> >> > On 2013/09/03 22:59:32, DaleCurtis wrote:
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.**org/23702008/diff/8001/media/**
> >
>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc>
> >
> >> > > File media/base/user_input_monitor_**win.cc (right):
> >> > >
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.**org/23702008/diff/8001/media/**
> >
>
base/user_input_monitor_win.**cc#newcode101<https://codereview.chromium.org/23702008/diff/8001/media/base/user_input_monitor_win.cc#newcode101>
> >
> >> > > media/base/user_input_monitor_**win.cc:101: base::Unretained(this),
> >>
> >> > > On 2013/09/03 21:41:28, jiayl wrote:
> >> > > > On 2013/09/03 20:07:04, Wez wrote:
> >> > > > > This will use-after-free if the StartMouseMonitoring() is called,
> >> and
> >> the
> >> > > > caller
> >> > > > > then deletes the UserInputMonitor immediately on the calling
> >> thread,
> >> since
> >> > > the
> >> > > > > task will still be processed on the UI thread.
> >> > > >
> >> > > > You are right. I was assuming the caller should make sure
> >>
> > UserInputMonitor
> >
> >> > is
> >> > > > not deleted too early, but that seems not a good assumption. I'm
> >> going
> >>
> > to
> >
> >> > > change
> >> > > > UserInputMonitor to ref counted. WeakPtr won't work since
> >>
> > UserInputMonitor
> >
> >> > is
> >> > > > destroyed and called back on different threads.
> >> > >
> >> > > Isn't it invalid for a client to call StartMouseMonitoring() then
> >> delete
> >> > without
> >> > > calling StopMouseMonitoring() ? If we can avoid ref-counting anything
> >>
> > that'd
> >
> >> > be
> >> > > great.
> >> >
> >> > That's true. But if StopMouseMonitoring is called right before
> >> UserInputMonitor,
> >> > the queued async events on the UI thread may still call back to the
> >>
> > destroyed
> >
> >> > UserInputMonitor.
> >>
> >
> >  Not if those events were bound to WeakPtrs to the UserInputMonitor. :)
> >>
> >
> > But the WeakPtrs need to be deref'd on the callback thread while
> > UserInputMonitor is destroyed on the main thread, which means WeakPtrs are
> > invalidated and deref'd on different threads.
> 
> 
> Which are the "callback" and "main" threads wrt this CL? You mean the
> |ui_task_runner| and calling threads?
> 
> Looking at UserInputMonitor's implementation, I'd assumed you must punt the
> mouse notifications back to the calling thread for dispatch, to avoid a
> race between notifications and calls to RemoveMouseListener(), which case
> you;d be dereferencing the WeakPtr on the caller thread, which would work.
> 
> It looks like you're instead relying on locking around the observer-list
> modifications, and observer notifications - are you sure that's a good
> idea? It means that one badly-behaved observer can indirectly block the UI
> thread. :(
> 
> 
> >
> >
>
https://codereview.chromium.**org/23702008/%3Chttps://codereview.chromium.org...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

But AddMouseListener/RemoveMouseListener can be called on any thread. Which
thread should the callback be dispatched to? Plus the calling thread of
Add/RemoveListener is not the same thread that creates/destroys
UserInputMonitor, which means there is still a problem in using WeakPtr.

Sergey Ulanov

Looks like there are some races - see my comments. I already suggested in previous ...

7 years, 3 months ago (2013-09-04 18:40:09 UTC) #17

Looks like there are some races - see my comments.

I already suggested in previous CLs to use ObserverListThreadSafe - it will help
to avoid the problems Wez pointed at and also guarantees that notifications are
delivered on the same thread on which listener is registered. You can use it
with a locked listeners counter to detect when to start/stop monitoring.

I also don't like that you are making UserInputMonitor ref-counted. You can
avoid it easily by using a "Core" object. It can be ref-counted, e.g. as in
remoting::LocalInputMonitorLinux, but if you destroy on the thread on which
events are received then it won't need to be ref-counted.

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:138: if
(!io_task_runner_->BelongsToCurrentThread()) {
There is potential race between StartKeyboardMonitoring() and
StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a thread
other than the IO thread and then StartKeyboardMonitoring() is called the IO
immediately after that then monitoring will be started and then stopped on the
IO thread, while it should be stopped first and then started. I think you can
avoid it by always posting a task to call StopMonitor().

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
File media/base/user_input_monitor_win.cc (right):

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
media/base/user_input_monitor_win.cc:114: if
(!ui_task_runner_->BelongsToCurrentThread()) {
There is a same race as in Linux version of this code. Just post a task for
StopMonitor()?

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
media/base/user_input_monitor_win.cc:207: ? ui::ET_KEY_RELEASED
nit: operators should not be wrapped. ? goes on the previous line.

https://codereview.chromium.org/23702008/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884
media/media.gyp:884: 'sources!': [
indentation

jiayl

On Wed, Sep 4, 2013 at 11:40 AM, <sergeyu@chromium.org> wrote: > Looks like there are ...

7 years, 3 months ago (2013-09-04 18:49:29 UTC) #18

On Wed, Sep 4, 2013 at 11:40 AM, <sergeyu@chromium.org> wrote:

> Looks like there are some races - see my comments.
>
> I already suggested in previous CLs to use ObserverListThreadSafe - it
> will help
> to avoid the problems Wez pointed at and also guarantees that
> notifications are
> delivered on the same thread on which listener is registered. You can use
> it
> with a locked listeners counter to detect when to start/stop monitoring.
>
> I also don't like that you are making UserInputMonitor ref-counted. You can
> avoid it easily by using a "Core" object. It can be ref-counted, e.g. as in
> remoting::**LocalInputMonitorLinux, but if you destroy on the thread on
> which
> events are received then it won't need to be ref-counted.
>


OberserListThreadSafe is also implemented through locks (
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
so will not solve the issue wez pointed out.

I don't see a way to get rid of the lock; but I can remove the ref-counting
of UserInputMonitor by posting the callback to the thread that
creates/destroys UserInputMonior, i.e. the browser UI thread, to make
WeakPtr valid to use.

How about that?

>
>
https://codereview.chromium.**org/23702008/diff/20001/media/**
>
base/user_input_monitor_linux.**cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc>
> File media/base/user_input_monitor_**linux.cc (right):
>
> https://codereview.chromium.**org/23702008/diff/20001/media/**
>
base/user_input_monitor_linux.**cc#newcode138<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc#newcode138>
> media/base/user_input_monitor_**linux.cc:138: if
> (!io_task_runner_->**BelongsToCurrentThread()) {
> There is potential race between StartKeyboardMonitoring() and
> StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a
> thread other than the IO thread and then StartKeyboardMonitoring() is
> called the IO immediately after that then monitoring will be started and
> then stopped on the IO thread, while it should be stopped first and then
> started. I think you can avoid it by always posting a task to call
> StopMonitor().
>
> https://codereview.chromium.**org/23702008/diff/20001/media/**
>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc>
> File media/base/user_input_monitor_**win.cc (right):
>
> https://codereview.chromium.**org/23702008/diff/20001/media/**
>
base/user_input_monitor_win.**cc#newcode114<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode114>
> media/base/user_input_monitor_**win.cc:114: if
> (!ui_task_runner_->**BelongsToCurrentThread()) {
> There is a same race as in Linux version of this code. Just post a task
> for StopMonitor()?
>
> https://codereview.chromium.**org/23702008/diff/20001/media/**
>
base/user_input_monitor_win.**cc#newcode207<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode207>
> media/base/user_input_monitor_**win.cc:207: ? ui::ET_KEY_RELEASED
> nit: operators should not be wrapped. ? goes on the previous line.
>
>
https://codereview.chromium.**org/23702008/diff/20001/media/**media.gyp<https...
> File media/media.gyp (right):
>
> https://codereview.chromium.**org/23702008/diff/20001/media/**
>
media.gyp#newcode884<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884>
> media/media.gyp:884: 'sources!': [
> indentation
>
>
https://codereview.chromium.**org/23702008/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Sergey Ulanov

On Wed, Sep 4, 2013 at 11:49 AM, Jiayang Liu <jiayl@chromium.org> wrote: > > On ...

7 years, 3 months ago (2013-09-04 18:53:50 UTC) #19

On Wed, Sep 4, 2013 at 11:49 AM, Jiayang Liu <jiayl@chromium.org> wrote:

>
> On Wed, Sep 4, 2013 at 11:40 AM, <sergeyu@chromium.org> wrote:
>
>> Looks like there are some races - see my comments.
>>
>> I already suggested in previous CLs to use ObserverListThreadSafe - it
>> will help
>> to avoid the problems Wez pointed at and also guarantees that
>> notifications are
>> delivered on the same thread on which listener is registered. You can use
>> it
>> with a locked listeners counter to detect when to start/stop monitoring.
>>
>> I also don't like that you are making UserInputMonitor ref-counted. You
>> can
>> avoid it easily by using a "Core" object. It can be ref-counted, e.g. as
>> in
>> remoting::**LocalInputMonitorLinux, but if you destroy on the thread on
>> which
>> events are received then it won't need to be ref-counted.
>>
>
>
> OberserListThreadSafe is also implemented through locks (
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
> so will not solve the issue wez pointed out.
>

OberserListThreadSafe holds a lock only to post a task. It always releases
the lock before calling observers.


>
> I don't see a way to get rid of the lock; but I can remove the
> ref-counting of UserInputMonitor by posting the callback to the thread that
> creates/destroys UserInputMonior, i.e. the browser UI thread, to make
> WeakPtr valid to use.
>
> How about that?
>
>>
>>
>  https://codereview.chromium.**org/23702008/diff/20001/media/**
>>
base/user_input_monitor_linux.**cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc>
>> File media/base/user_input_monitor_**linux.cc (right):
>>
>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>
base/user_input_monitor_linux.**cc#newcode138<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc#newcode138>
>> media/base/user_input_monitor_**linux.cc:138: if
>> (!io_task_runner_->**BelongsToCurrentThread()) {
>> There is potential race between StartKeyboardMonitoring() and
>> StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a
>> thread other than the IO thread and then StartKeyboardMonitoring() is
>> called the IO immediately after that then monitoring will be started and
>> then stopped on the IO thread, while it should be stopped first and then
>> started. I think you can avoid it by always posting a task to call
>> StopMonitor().
>>
>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc>
>> File media/base/user_input_monitor_**win.cc (right):
>>
>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>
base/user_input_monitor_win.**cc#newcode114<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode114>
>> media/base/user_input_monitor_**win.cc:114: if
>> (!ui_task_runner_->**BelongsToCurrentThread()) {
>> There is a same race as in Linux version of this code. Just post a task
>> for StopMonitor()?
>>
>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>
base/user_input_monitor_win.**cc#newcode207<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode207>
>> media/base/user_input_monitor_**win.cc:207: ? ui::ET_KEY_RELEASED
>> nit: operators should not be wrapped. ? goes on the previous line.
>>
>>
https://codereview.chromium.**org/23702008/diff/20001/media/**media.gyp<https...
>> File media/media.gyp (right):
>>
>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>
media.gyp#newcode884<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884>
>> media/media.gyp:884: 'sources!': [
>> indentation
>>
>>
https://codereview.chromium.**org/23702008/<https://codereview.chromium.org/2...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

jiayl

On Wed, Sep 4, 2013 at 11:53 AM, Sergey Ulanov <sergeyu@chromium.org> wrote: > > On ...

7 years, 3 months ago (2013-09-04 18:56:51 UTC) #20

On Wed, Sep 4, 2013 at 11:53 AM, Sergey Ulanov <sergeyu@chromium.org> wrote:

>
> On Wed, Sep 4, 2013 at 11:49 AM, Jiayang Liu <jiayl@chromium.org> wrote:
>
>>
>> On Wed, Sep 4, 2013 at 11:40 AM, <sergeyu@chromium.org> wrote:
>>
>>> Looks like there are some races - see my comments.
>>>
>>> I already suggested in previous CLs to use ObserverListThreadSafe - it
>>> will help
>>> to avoid the problems Wez pointed at and also guarantees that
>>> notifications are
>>> delivered on the same thread on which listener is registered. You can
>>> use it
>>> with a locked listeners counter to detect when to start/stop monitoring.
>>>
>>> I also don't like that you are making UserInputMonitor ref-counted. You
>>> can
>>> avoid it easily by using a "Core" object. It can be ref-counted, e.g. as
>>> in
>>> remoting::**LocalInputMonitorLinux, but if you destroy on the thread on
>>> which
>>> events are received then it won't need to be ref-counted.
>>>
>>
>>
>> OberserListThreadSafe is also implemented through locks (
>>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
>> so will not solve the issue wez pointed out.
>>
>
> OberserListThreadSafe holds a lock only to post a task. It always releases
> the lock before calling observers.
>
>

Ah, I see.  I'll fix that then. :) Thanks!

>
>> I don't see a way to get rid of the lock; but I can remove the
>> ref-counting of UserInputMonitor by posting the callback to the thread that
>> creates/destroys UserInputMonior, i.e. the browser UI thread, to make
>> WeakPtr valid to use.
>>
>> How about that?
>>
>>>
>>>
>>  https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>
base/user_input_monitor_linux.**cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc>
>>> File media/base/user_input_monitor_**linux.cc (right):
>>>
>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>
base/user_input_monitor_linux.**cc#newcode138<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc#newcode138>
>>> media/base/user_input_monitor_**linux.cc:138: if
>>> (!io_task_runner_->**BelongsToCurrentThread()) {
>>> There is potential race between StartKeyboardMonitoring() and
>>> StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a
>>> thread other than the IO thread and then StartKeyboardMonitoring() is
>>> called the IO immediately after that then monitoring will be started and
>>> then stopped on the IO thread, while it should be stopped first and then
>>> started. I think you can avoid it by always posting a task to call
>>> StopMonitor().
>>>
>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc>
>>> File media/base/user_input_monitor_**win.cc (right):
>>>
>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>
base/user_input_monitor_win.**cc#newcode114<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode114>
>>> media/base/user_input_monitor_**win.cc:114: if
>>> (!ui_task_runner_->**BelongsToCurrentThread()) {
>>> There is a same race as in Linux version of this code. Just post a task
>>> for StopMonitor()?
>>>
>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>
base/user_input_monitor_win.**cc#newcode207<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode207>
>>> media/base/user_input_monitor_**win.cc:207: ? ui::ET_KEY_RELEASED
>>> nit: operators should not be wrapped. ? goes on the previous line.
>>>
>>>
https://codereview.chromium.**org/23702008/diff/20001/media/**media.gyp<https...
>>> File media/media.gyp (right):
>>>
>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>
media.gyp#newcode884<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884>
>>> media/media.gyp:884: 'sources!': [
>>> indentation
>>>
>>>
https://codereview.chromium.**org/23702008/<https://codereview.chromium.org/2...
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

jiayl

I'm planning to make this change to remove the assumption that UserInputMonitor has to outlive ...

7 years, 3 months ago (2013-09-04 21:23:55 UTC) #21

I'm planning to make this change to remove the assumption that
UserInputMonitor has to outlive the UI/IO browser threads. Please let me
know what you think.

I'll have a UserInputMonitorWrapper class to handle the creation and
destruction of UserInputMonitor:
class UserInputMonitorWrapper {
 public:
  UserInputMonitorWrapper(io_task_runner, ui_task_runner);
  ~UserInputMonitorWrapper();
  UserInputMonitor* user_input_monitor() { return monitor_; }
 private:
  UserInputMonitor* monitor_;
}

The implementation of UserInputMonitorWrapper will be platform specifc, for
example on Linux:
UserInputMonitorWrapper::UserInputMonitorWrapper (...) {
  monitor_ = new UserInputMonitorLinux(io_task_runner);
}
// Dispatch to the IO thread to delete |monitor_|.
UserInputMonitorWrapper::~UserInputMonitorWrapper() {
  monitor_->io_task_runner()->PostTask(
    &UserInputMonitorLinux::Shutdown, base::Owned(monitor_));
  monitor_ = NULL;
}

Then UserInputMonitorLinux can use WeakPtr for posting tasks to the IO
thread since it will be destructed on the IO thread.

It doesn't matter if the io_task_runner is not running when the wrapper is
destructed since base::Owned will make sure |monitor_| is not leaked.

UserInputMonitorLinux::Shutdown does not need to anything except checking
monitoring has been stopped.




On Wed, Sep 4, 2013 at 11:56 AM, Jiayang Liu <jiayl@chromium.org> wrote:

>
>
>
> On Wed, Sep 4, 2013 at 11:53 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>
>>
>> On Wed, Sep 4, 2013 at 11:49 AM, Jiayang Liu <jiayl@chromium.org> wrote:
>>
>>>
>>> On Wed, Sep 4, 2013 at 11:40 AM, <sergeyu@chromium.org> wrote:
>>>
>>>> Looks like there are some races - see my comments.
>>>>
>>>> I already suggested in previous CLs to use ObserverListThreadSafe - it
>>>> will help
>>>> to avoid the problems Wez pointed at and also guarantees that
>>>> notifications are
>>>> delivered on the same thread on which listener is registered. You can
>>>> use it
>>>> with a locked listeners counter to detect when to start/stop monitoring.
>>>>
>>>> I also don't like that you are making UserInputMonitor ref-counted. You
>>>> can
>>>> avoid it easily by using a "Core" object. It can be ref-counted, e.g.
>>>> as in
>>>> remoting::**LocalInputMonitorLinux, but if you destroy on the thread
>>>> on which
>>>> events are received then it won't need to be ref-counted.
>>>>
>>>
>>>
>>> OberserListThreadSafe is also implemented through locks (
>>>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
>>> so will not solve the issue wez pointed out.
>>>
>>
>> OberserListThreadSafe holds a lock only to post a task. It always
>> releases the lock before calling observers.
>>
>>
>
> Ah, I see.  I'll fix that then. :) Thanks!
>
>>
>>> I don't see a way to get rid of the lock; but I can remove the
>>> ref-counting of UserInputMonitor by posting the callback to the thread that
>>> creates/destroys UserInputMonior, i.e. the browser UI thread, to make
>>> WeakPtr valid to use.
>>>
>>> How about that?
>>>
>>>>
>>>>
>>>  https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>
base/user_input_monitor_linux.**cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc>
>>>> File media/base/user_input_monitor_**linux.cc (right):
>>>>
>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>
base/user_input_monitor_linux.**cc#newcode138<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc#newcode138>
>>>> media/base/user_input_monitor_**linux.cc:138: if
>>>> (!io_task_runner_->**BelongsToCurrentThread()) {
>>>> There is potential race between StartKeyboardMonitoring() and
>>>> StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a
>>>> thread other than the IO thread and then StartKeyboardMonitoring() is
>>>> called the IO immediately after that then monitoring will be started and
>>>> then stopped on the IO thread, while it should be stopped first and then
>>>> started. I think you can avoid it by always posting a task to call
>>>> StopMonitor().
>>>>
>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc>
>>>> File media/base/user_input_monitor_**win.cc (right):
>>>>
>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>
base/user_input_monitor_win.**cc#newcode114<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode114>
>>>> media/base/user_input_monitor_**win.cc:114: if
>>>> (!ui_task_runner_->**BelongsToCurrentThread()) {
>>>> There is a same race as in Linux version of this code. Just post a task
>>>> for StopMonitor()?
>>>>
>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>
base/user_input_monitor_win.**cc#newcode207<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode207>
>>>> media/base/user_input_monitor_**win.cc:207: ? ui::ET_KEY_RELEASED
>>>> nit: operators should not be wrapped. ? goes on the previous line.
>>>>
>>>>
https://codereview.chromium.**org/23702008/diff/20001/media/**media.gyp<https...
>>>> File media/media.gyp (right):
>>>>
>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>
media.gyp#newcode884<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884>
>>>> media/media.gyp:884: 'sources!': [
>>>> indentation
>>>>
>>>>
https://codereview.chromium.**org/23702008/<https://codereview.chromium.org/2...
>>>>
>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Sergey Ulanov

That's essentially what I and Wez were suggesting, but please consider adding Core inside of ...

7 years, 3 months ago (2013-09-04 22:42:37 UTC) #22

That's essentially what I and Wez were suggesting, but please consider
adding Core inside of UserInputMonitor instead of a wrapper outside of
it. I.e. in you code below rename the core
UserInputMonitor=>UserInputMonitor::Core and the
wrapper UserInputMonitorWrapper=>UserInputMonitor. Then instead of exposing
core from the outer class add separate methods that call the core.


On Wed, Sep 4, 2013 at 2:23 PM, Jiayang Liu <jiayl@chromium.org> wrote:

> I'm planning to make this change to remove the assumption that
> UserInputMonitor has to outlive the UI/IO browser threads. Please let me
> know what you think.
>
> I'll have a UserInputMonitorWrapper class to handle the creation and
> destruction of UserInputMonitor:
> class UserInputMonitorWrapper {
>  public:
>   UserInputMonitorWrapper(io_task_runner, ui_task_runner);
>   ~UserInputMonitorWrapper();
>   UserInputMonitor* user_input_monitor() { return monitor_; }
>  private:
>   UserInputMonitor* monitor_;
> }
>
> The implementation of UserInputMonitorWrapper will be platform specifc,
> for example on Linux:
> UserInputMonitorWrapper::UserInputMonitorWrapper (...) {
>   monitor_ = new UserInputMonitorLinux(io_task_runner);
> }
> // Dispatch to the IO thread to delete |monitor_|.
> UserInputMonitorWrapper::~UserInputMonitorWrapper() {
>   monitor_->io_task_runner()->PostTask(
>     &UserInputMonitorLinux::Shutdown, base::Owned(monitor_));
>   monitor_ = NULL;
> }
>
> Then UserInputMonitorLinux can use WeakPtr for posting tasks to the IO
> thread since it will be destructed on the IO thread.
>
> It doesn't matter if the io_task_runner is not running when the wrapper is
> destructed since base::Owned will make sure |monitor_| is not leaked.
>

I think you don't really need base::Owned() - just call
TaskRunner::DeleteSoon() when you are ready to delete it.


>
> UserInputMonitorLinux::Shutdown does not need to anything except checking
> monitoring has been stopped.
>
>
>
>
> On Wed, Sep 4, 2013 at 11:56 AM, Jiayang Liu <jiayl@chromium.org> wrote:
>
>>
>>
>>
>> On Wed, Sep 4, 2013 at 11:53 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>>
>>>
>>> On Wed, Sep 4, 2013 at 11:49 AM, Jiayang Liu <jiayl@chromium.org> wrote:
>>>
>>>>
>>>> On Wed, Sep 4, 2013 at 11:40 AM, <sergeyu@chromium.org> wrote:
>>>>
>>>>> Looks like there are some races - see my comments.
>>>>>
>>>>> I already suggested in previous CLs to use ObserverListThreadSafe - it
>>>>> will help
>>>>> to avoid the problems Wez pointed at and also guarantees that
>>>>> notifications are
>>>>> delivered on the same thread on which listener is registered. You can
>>>>> use it
>>>>> with a locked listeners counter to detect when to start/stop
>>>>> monitoring.
>>>>>
>>>>> I also don't like that you are making UserInputMonitor ref-counted.
>>>>> You can
>>>>> avoid it easily by using a "Core" object. It can be ref-counted, e.g.
>>>>> as in
>>>>> remoting::**LocalInputMonitorLinux, but if you destroy on the thread
>>>>> on which
>>>>> events are received then it won't need to be ref-counted.
>>>>>
>>>>
>>>>
>>>> OberserListThreadSafe is also implemented through locks (
>>>>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
>>>> so will not solve the issue wez pointed out.
>>>>
>>>
>>> OberserListThreadSafe holds a lock only to post a task. It always
>>> releases the lock before calling observers.
>>>
>>>
>>
>> Ah, I see.  I'll fix that then. :) Thanks!
>>
>>>
>>>> I don't see a way to get rid of the lock; but I can remove the
>>>> ref-counting of UserInputMonitor by posting the callback to the thread
that
>>>> creates/destroys UserInputMonior, i.e. the browser UI thread, to make
>>>> WeakPtr valid to use.
>>>>
>>>> How about that?
>>>>
>>>>>
>>>>>
>>>>  https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>
base/user_input_monitor_linux.**cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc>
>>>>> File media/base/user_input_monitor_**linux.cc (right):
>>>>>
>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>
base/user_input_monitor_linux.**cc#newcode138<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc#newcode138>
>>>>> media/base/user_input_monitor_**linux.cc:138: if
>>>>> (!io_task_runner_->**BelongsToCurrentThread()) {
>>>>> There is potential race between StartKeyboardMonitoring() and
>>>>> StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a
>>>>> thread other than the IO thread and then StartKeyboardMonitoring() is
>>>>> called the IO immediately after that then monitoring will be started
>>>>> and
>>>>> then stopped on the IO thread, while it should be stopped first and
>>>>> then
>>>>> started. I think you can avoid it by always posting a task to call
>>>>> StopMonitor().
>>>>>
>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc>
>>>>> File media/base/user_input_monitor_**win.cc (right):
>>>>>
>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>
base/user_input_monitor_win.**cc#newcode114<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode114>
>>>>> media/base/user_input_monitor_**win.cc:114: if
>>>>> (!ui_task_runner_->**BelongsToCurrentThread()) {
>>>>> There is a same race as in Linux version of this code. Just post a task
>>>>> for StopMonitor()?
>>>>>
>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>
base/user_input_monitor_win.**cc#newcode207<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode207>
>>>>> media/base/user_input_monitor_**win.cc:207: ? ui::ET_KEY_RELEASED
>>>>> nit: operators should not be wrapped. ? goes on the previous line.
>>>>>
>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>
media.gyp<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp>
>>>>> File media/media.gyp (right):
>>>>>
>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>
media.gyp#newcode884<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884>
>>>>> media/media.gyp:884: 'sources!': [
>>>>> indentation
>>>>>
>>>>>
https://codereview.chromium.**org/23702008/<https://codereview.chromium.org/2...
>>>>>
>>>>
>>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

jiayl

On Wed, Sep 4, 2013 at 3:42 PM, Sergey Ulanov <sergeyu@chromium.org> wrote: > That's essentially ...

7 years, 3 months ago (2013-09-04 22:49:39 UTC) #23

On Wed, Sep 4, 2013 at 3:42 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

> That's essentially what I and Wez were suggesting, but please consider
> adding Core inside of UserInputMonitor instead of a wrapper outside of
> it. I.e. in you code below rename the core
> UserInputMonitor=>UserInputMonitor::Core and the
> wrapper UserInputMonitorWrapper=>UserInputMonitor. Then instead of exposing
> core from the outer class add separate methods that call the core.
>
>
> That means the mouse listeners need to be moved into Core so that Core
does not need to call back to the outer class. Does that sound good to you?


> On Wed, Sep 4, 2013 at 2:23 PM, Jiayang Liu <jiayl@chromium.org> wrote:
>
>> I'm planning to make this change to remove the assumption that
>> UserInputMonitor has to outlive the UI/IO browser threads. Please let me
>> know what you think.
>>
>> I'll have a UserInputMonitorWrapper class to handle the creation and
>> destruction of UserInputMonitor:
>> class UserInputMonitorWrapper {
>>  public:
>>   UserInputMonitorWrapper(io_task_runner, ui_task_runner);
>>   ~UserInputMonitorWrapper();
>>   UserInputMonitor* user_input_monitor() { return monitor_; }
>>  private:
>>   UserInputMonitor* monitor_;
>> }
>>
>> The implementation of UserInputMonitorWrapper will be platform specifc,
>> for example on Linux:
>> UserInputMonitorWrapper::UserInputMonitorWrapper (...) {
>>   monitor_ = new UserInputMonitorLinux(io_task_runner);
>> }
>> // Dispatch to the IO thread to delete |monitor_|.
>> UserInputMonitorWrapper::~UserInputMonitorWrapper() {
>>   monitor_->io_task_runner()->PostTask(
>>     &UserInputMonitorLinux::Shutdown, base::Owned(monitor_));
>>   monitor_ = NULL;
>> }
>>
>> Then UserInputMonitorLinux can use WeakPtr for posting tasks to the IO
>> thread since it will be destructed on the IO thread.
>>
>> It doesn't matter if the io_task_runner is not running when the wrapper
>> is destructed since base::Owned will make sure |monitor_| is not leaked.
>>
>
> I think you don't really need base::Owned() - just call
> TaskRunner::DeleteSoon() when you are ready to delete it.
>
>
>>
>> UserInputMonitorLinux::Shutdown does not need to anything except checking
>> monitoring has been stopped.
>>
>>
>>
>>
>> On Wed, Sep 4, 2013 at 11:56 AM, Jiayang Liu <jiayl@chromium.org> wrote:
>>
>>>
>>>
>>>
>>> On Wed, Sep 4, 2013 at 11:53 AM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>>>
>>>>
>>>> On Wed, Sep 4, 2013 at 11:49 AM, Jiayang Liu <jiayl@chromium.org>wrote:
>>>>
>>>>>
>>>>> On Wed, Sep 4, 2013 at 11:40 AM, <sergeyu@chromium.org> wrote:
>>>>>
>>>>>> Looks like there are some races - see my comments.
>>>>>>
>>>>>> I already suggested in previous CLs to use ObserverListThreadSafe -
>>>>>> it will help
>>>>>> to avoid the problems Wez pointed at and also guarantees that
>>>>>> notifications are
>>>>>> delivered on the same thread on which listener is registered. You can
>>>>>> use it
>>>>>> with a locked listeners counter to detect when to start/stop
>>>>>> monitoring.
>>>>>>
>>>>>> I also don't like that you are making UserInputMonitor ref-counted.
>>>>>> You can
>>>>>> avoid it easily by using a "Core" object. It can be ref-counted, e.g.
>>>>>> as in
>>>>>> remoting::**LocalInputMonitorLinux, but if you destroy on the thread
>>>>>> on which
>>>>>> events are received then it won't need to be ref-counted.
>>>>>>
>>>>>
>>>>>
>>>>> OberserListThreadSafe is also implemented through locks (
>>>>>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
>>>>> so will not solve the issue wez pointed out.
>>>>>
>>>>
>>>> OberserListThreadSafe holds a lock only to post a task. It always
>>>> releases the lock before calling observers.
>>>>
>>>>
>>>
>>> Ah, I see.  I'll fix that then. :) Thanks!
>>>
>>>>
>>>>> I don't see a way to get rid of the lock; but I can remove the
>>>>> ref-counting of UserInputMonitor by posting the callback to the thread
that
>>>>> creates/destroys UserInputMonior, i.e. the browser UI thread, to make
>>>>> WeakPtr valid to use.
>>>>>
>>>>> How about that?
>>>>>
>>>>>>
>>>>>>
>>>>>  https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>
base/user_input_monitor_linux.**cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc>
>>>>>> File media/base/user_input_monitor_**linux.cc (right):
>>>>>>
>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>
base/user_input_monitor_linux.**cc#newcode138<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc#newcode138>
>>>>>> media/base/user_input_monitor_**linux.cc:138: if
>>>>>> (!io_task_runner_->**BelongsToCurrentThread()) {
>>>>>> There is potential race between StartKeyboardMonitoring() and
>>>>>> StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a
>>>>>> thread other than the IO thread and then StartKeyboardMonitoring() is
>>>>>> called the IO immediately after that then monitoring will be started
>>>>>> and
>>>>>> then stopped on the IO thread, while it should be stopped first and
>>>>>> then
>>>>>> started. I think you can avoid it by always posting a task to call
>>>>>> StopMonitor().
>>>>>>
>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc>
>>>>>> File media/base/user_input_monitor_**win.cc (right):
>>>>>>
>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>
base/user_input_monitor_win.**cc#newcode114<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode114>
>>>>>> media/base/user_input_monitor_**win.cc:114: if
>>>>>> (!ui_task_runner_->**BelongsToCurrentThread()) {
>>>>>> There is a same race as in Linux version of this code. Just post a
>>>>>> task
>>>>>> for StopMonitor()?
>>>>>>
>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>
base/user_input_monitor_win.**cc#newcode207<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode207>
>>>>>> media/base/user_input_monitor_**win.cc:207: ? ui::ET_KEY_RELEASED
>>>>>> nit: operators should not be wrapped. ? goes on the previous line.
>>>>>>
>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>
media.gyp<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp>
>>>>>> File media/media.gyp (right):
>>>>>>
>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>
media.gyp#newcode884<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884>
>>>>>> media/media.gyp:884: 'sources!': [
>>>>>> indentation
>>>>>>
>>>>>>
https://codereview.chromium.**org/23702008/<https://codereview.chromium.org/2...
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Sergey Ulanov

On Wed, Sep 4, 2013 at 3:49 PM, Jiayang Liu <jiayl@chromium.org> wrote: > > On ...

7 years, 3 months ago (2013-09-04 22:52:56 UTC) #24

On Wed, Sep 4, 2013 at 3:49 PM, Jiayang Liu <jiayl@chromium.org> wrote:

>
> On Wed, Sep 4, 2013 at 3:42 PM, Sergey Ulanov <sergeyu@chromium.org>wrote:
>
>> That's essentially what I and Wez were suggesting, but please consider
>> adding Core inside of UserInputMonitor instead of a wrapper outside of
>> it. I.e. in you code below rename the core
>> UserInputMonitor=>UserInputMonitor::Core and the
>> wrapper UserInputMonitorWrapper=>UserInputMonitor. Then instead of exposing
>> core from the outer class add separate methods that call the core.
>>
>>
>> That means the mouse listeners need to be moved into Core so that Core
> does not need to call back to the outer class. Does that sound good to you?
>

Yes. The wrapper will just delegate AddListener() and RemoveListener()
calls to the core and the core will take care of storing the listeners and
invoking them when necessary.




>
>
>> On Wed, Sep 4, 2013 at 2:23 PM, Jiayang Liu <jiayl@chromium.org> wrote:
>>
>>> I'm planning to make this change to remove the assumption that
>>> UserInputMonitor has to outlive the UI/IO browser threads. Please let me
>>> know what you think.
>>>
>>> I'll have a UserInputMonitorWrapper class to handle the creation and
>>> destruction of UserInputMonitor:
>>> class UserInputMonitorWrapper {
>>>  public:
>>>   UserInputMonitorWrapper(io_task_runner, ui_task_runner);
>>>   ~UserInputMonitorWrapper();
>>>   UserInputMonitor* user_input_monitor() { return monitor_; }
>>>  private:
>>>   UserInputMonitor* monitor_;
>>> }
>>>
>>> The implementation of UserInputMonitorWrapper will be platform specifc,
>>> for example on Linux:
>>> UserInputMonitorWrapper::UserInputMonitorWrapper (...) {
>>>   monitor_ = new UserInputMonitorLinux(io_task_runner);
>>> }
>>> // Dispatch to the IO thread to delete |monitor_|.
>>> UserInputMonitorWrapper::~UserInputMonitorWrapper() {
>>>   monitor_->io_task_runner()->PostTask(
>>>     &UserInputMonitorLinux::Shutdown, base::Owned(monitor_));
>>>   monitor_ = NULL;
>>> }
>>>
>>> Then UserInputMonitorLinux can use WeakPtr for posting tasks to the IO
>>> thread since it will be destructed on the IO thread.
>>>
>>> It doesn't matter if the io_task_runner is not running when the wrapper
>>> is destructed since base::Owned will make sure |monitor_| is not leaked.
>>>
>>
>> I think you don't really need base::Owned() - just call
>> TaskRunner::DeleteSoon() when you are ready to delete it.
>>
>>
>>>
>>> UserInputMonitorLinux::Shutdown does not need to anything except
>>> checking monitoring has been stopped.
>>>
>>>
>>>
>>>
>>> On Wed, Sep 4, 2013 at 11:56 AM, Jiayang Liu <jiayl@chromium.org> wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Wed, Sep 4, 2013 at 11:53 AM, Sergey Ulanov
<sergeyu@chromium.org>wrote:
>>>>
>>>>>
>>>>> On Wed, Sep 4, 2013 at 11:49 AM, Jiayang Liu <jiayl@chromium.org>wrote:
>>>>>
>>>>>>
>>>>>> On Wed, Sep 4, 2013 at 11:40 AM, <sergeyu@chromium.org> wrote:
>>>>>>
>>>>>>> Looks like there are some races - see my comments.
>>>>>>>
>>>>>>> I already suggested in previous CLs to use ObserverListThreadSafe -
>>>>>>> it will help
>>>>>>> to avoid the problems Wez pointed at and also guarantees that
>>>>>>> notifications are
>>>>>>> delivered on the same thread on which listener is registered. You
>>>>>>> can use it
>>>>>>> with a locked listeners counter to detect when to start/stop
>>>>>>> monitoring.
>>>>>>>
>>>>>>> I also don't like that you are making UserInputMonitor ref-counted.
>>>>>>> You can
>>>>>>> avoid it easily by using a "Core" object. It can be ref-counted,
>>>>>>> e.g. as in
>>>>>>> remoting::**LocalInputMonitorLinux, but if you destroy on the
>>>>>>> thread on which
>>>>>>> events are received then it won't need to be ref-counted.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> OberserListThreadSafe is also implemented through locks (
>>>>>>
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...),
>>>>>> so will not solve the issue wez pointed out.
>>>>>>
>>>>>
>>>>> OberserListThreadSafe holds a lock only to post a task. It always
>>>>> releases the lock before calling observers.
>>>>>
>>>>>
>>>>
>>>> Ah, I see.  I'll fix that then. :) Thanks!
>>>>
>>>>>
>>>>>> I don't see a way to get rid of the lock; but I can remove the
>>>>>> ref-counting of UserInputMonitor by posting the callback to the thread
that
>>>>>> creates/destroys UserInputMonior, i.e. the browser UI thread, to make
>>>>>> WeakPtr valid to use.
>>>>>>
>>>>>> How about that?
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>  https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>>
base/user_input_monitor_linux.**cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc>
>>>>>>> File media/base/user_input_monitor_**linux.cc (right):
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>>
base/user_input_monitor_linux.**cc#newcode138<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc#newcode138>
>>>>>>> media/base/user_input_monitor_**linux.cc:138: if
>>>>>>> (!io_task_runner_->**BelongsToCurrentThread()) {
>>>>>>> There is potential race between StartKeyboardMonitoring() and
>>>>>>> StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a
>>>>>>> thread other than the IO thread and then StartKeyboardMonitoring() is
>>>>>>> called the IO immediately after that then monitoring will be started
>>>>>>> and
>>>>>>> then stopped on the IO thread, while it should be stopped first and
>>>>>>> then
>>>>>>> started. I think you can avoid it by always posting a task to call
>>>>>>> StopMonitor().
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>>
base/user_input_monitor_win.cc<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc>
>>>>>>> File media/base/user_input_monitor_**win.cc (right):
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>>
base/user_input_monitor_win.**cc#newcode114<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode114>
>>>>>>> media/base/user_input_monitor_**win.cc:114: if
>>>>>>> (!ui_task_runner_->**BelongsToCurrentThread()) {
>>>>>>> There is a same race as in Linux version of this code. Just post a
>>>>>>> task
>>>>>>> for StopMonitor()?
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>>
base/user_input_monitor_win.**cc#newcode207<https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_win.cc#newcode207>
>>>>>>> media/base/user_input_monitor_**win.cc:207: ? ui::ET_KEY_RELEASED
>>>>>>> nit: operators should not be wrapped. ? goes on the previous line.
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>>
media.gyp<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp>
>>>>>>> File media/media.gyp (right):
>>>>>>>
>>>>>>> https://codereview.chromium.**org/23702008/diff/20001/media/**
>>>>>>>
media.gyp#newcode884<https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884>
>>>>>>> media/media.gyp:884: 'sources!': [
>>>>>>> indentation
>>>>>>>
>>>>>>>
https://codereview.chromium.**org/23702008/<https://codereview.chromium.org/2...
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

jiayl

The mouse listener list is moved to the platform specific implementation, which is now separated ...

7 years, 3 months ago (2013-09-05 00:29:37 UTC) #25

The mouse listener list is moved to the platform specific implementation, which
is now separated from UserInputMonitor and always deleted on the monitoring
thread (IO on Linux and UI on others). 
PTAL. Thanks!

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:138: if
(!io_task_runner_->BelongsToCurrentThread()) {
On 2013/09/04 18:40:09, Sergey Ulanov wrote:
> There is potential race between StartKeyboardMonitoring() and
> StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a thread
> other than the IO thread and then StartKeyboardMonitoring() is called the IO
> immediately after that then monitoring will be started and then stopped on the
> IO thread, while it should be stopped first and then started. I think you can
> avoid it by always posting a task to call StopMonitor().

If the same caller calls Start and Stop on different threads, there is no way to
know what's the intended ordering and I think it's expected to get undefined
behavior.

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
File media/base/user_input_monitor_win.cc (right):

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
media/base/user_input_monitor_win.cc:207: ? ui::ET_KEY_RELEASED
On 2013/09/04 18:40:09, Sergey Ulanov wrote:
> nit: operators should not be wrapped. ? goes on the previous line.

I don't find the rule on the style guide. But this is auto formatted by git-cl
format.

https://codereview.chromium.org/23702008/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/23702008/diff/20001/media/media.gyp#newcode884
media/media.gyp:884: 'sources!': [
On 2013/09/04 18:40:09, Sergey Ulanov wrote:
> indentation

Done.

jiayl

Oops. Uploaded now. Thanks! On Wed, Sep 4, 2013 at 5:36 PM, <dalecurtis@chromium.org> wrote: > ...

7 years, 3 months ago (2013-09-05 00:38:16 UTC) #27

DaleCurtis

Just nits really. I defer to the remoting/ team for the WIndows specific bits. https://codereview.chromium.org/23702008/diff/34001/media/base/user_input_monitor_linux.cc ...

7 years, 3 months ago (2013-09-06 21:51:08 UTC) #28

jiayl

https://codereview.chromium.org/23702008/diff/34001/media/base/user_input_monitor_linux.cc File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/23702008/diff/34001/media/base/user_input_monitor_linux.cc#newcode359 media/base/user_input_monitor_linux.cc:359: UserInputMonitorLinux::UserInputMonitorLinux( On 2013/09/06 21:51:08, DaleCurtis wrote: > Blank line ...

7 years, 3 months ago (2013-09-06 22:43:29 UTC) #30

Sergey Ulanov

Thanks for removing ref-counting! There is still a problem with races between StartMonitoring() and StopMonitoring() ...

7 years, 3 months ago (2013-09-07 20:19:40 UTC) #31

Thanks for removing ref-counting! There is still a problem with races between
StartMonitoring() and StopMonitoring() - it should be easy to fix.

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:138: if
(!io_task_runner_->BelongsToCurrentThread()) {
On 2013/09/05 00:29:38, jiayl wrote:
> On 2013/09/04 18:40:09, Sergey Ulanov wrote:
> > There is potential race between StartKeyboardMonitoring() and
> > StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a thread
> > other than the IO thread and then StartKeyboardMonitoring() is called the IO
> > immediately after that then monitoring will be started and then stopped on
the
> > IO thread, while it should be stopped first and then started. I think you
can
> > avoid it by always posting a task to call StopMonitor().
> 
> If the same caller calls Start and Stop on different threads, there is no way
to
> know what's the intended ordering and I think it's expected to get undefined
> behavior.

It might not be the same caller.  One object may be calling
RemoveMouseListener() which would call StopMonitoring(), while completely
different object is calling AddMouseListener() which calls StartMonitoring().
What matters is that the listener gets notification after registering with
AddMouseListener(), but due to this race it could happen that monitoring will be
stopped when it shouldn't be.

https://codereview.chromium.org/23702008/diff/45001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/45001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:174: if
(!io_task_runner_->BelongsToCurrentThread()) {
There is still the problem that I pointed before: If some thread calls
StartMonitor() and then IO thread calls StopMonitor() immediately after that
then StopMonitor() will be executed before StartMonitor() is called on the IO
thread. This should be easy to fix. Just add DoStartMonitor() and always post a
task from StartMonitor().

StartMonitoring() and StopMonitoring() are always called with the lock held, so
there is definite order in which these methods are called. The code that
starts/stops monitoring on the IO thread should be always executed in the same
order, but that's not guaranteed if you don't always post a task here.

jiayl

Fixed the race. PTAL. Thanks! https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_monitor_linux.cc#newcode138 media/base/user_input_monitor_linux.cc:138: if (!io_task_runner_->BelongsToCurrentThread()) { On ...

7 years, 3 months ago (2013-09-10 16:30:59 UTC) #32

Fixed the race. PTAL. Thanks!

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/20001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:138: if
(!io_task_runner_->BelongsToCurrentThread()) {
On 2013/09/07 20:19:41, Sergey Ulanov wrote:
> On 2013/09/05 00:29:38, jiayl wrote:
> > On 2013/09/04 18:40:09, Sergey Ulanov wrote:
> > > There is potential race between StartKeyboardMonitoring() and
> > > StopKeyboardMonitoring(). If StopKeyboardMonitoring() is called on a
thread
> > > other than the IO thread and then StartKeyboardMonitoring() is called the
IO
> > > immediately after that then monitoring will be started and then stopped on
> the
> > > IO thread, while it should be stopped first and then started. I think you
> can
> > > avoid it by always posting a task to call StopMonitor().
> > 
> > If the same caller calls Start and Stop on different threads, there is no
way
> to
> > know what's the intended ordering and I think it's expected to get undefined
> > behavior.
> 
> It might not be the same caller.  One object may be calling
> RemoveMouseListener() which would call StopMonitoring(), while completely
> different object is calling AddMouseListener() which calls StartMonitoring().
> What matters is that the listener gets notification after registering with
> AddMouseListener(), but due to this race it could happen that monitoring will
be
> stopped when it shouldn't be.

Done.

https://codereview.chromium.org/23702008/diff/45001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/45001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:174: if
(!io_task_runner_->BelongsToCurrentThread()) {
On 2013/09/07 20:19:41, Sergey Ulanov wrote:
> There is still the problem that I pointed before: If some thread calls
> StartMonitor() and then IO thread calls StopMonitor() immediately after that
> then StopMonitor() will be executed before StartMonitor() is called on the IO
> thread. This should be easy to fix. Just add DoStartMonitor() and always post
a
> task from StartMonitor().
> 
> StartMonitoring() and StopMonitoring() are always called with the lock held,
so
> there is definite order in which these methods are called. The code that
> starts/stops monitoring on the IO thread should be always executed in the same
> order, but that's not guaranteed if you don't always post a task here.

Done.

Sergey Ulanov

LGTM. Just some minor nits. https://codereview.chromium.org/23702008/diff/50001/media/base/user_input_monitor_win.cc File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/23702008/diff/50001/media/base/user_input_monitor_win.cc#newcode40 media/base/user_input_monitor_win.cc:40: // Private interface of ...

7 years, 3 months ago (2013-09-10 18:28:38 UTC) #33

jiayl

https://codereview.chromium.org/23702008/diff/50001/media/base/user_input_monitor_win.cc File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/23702008/diff/50001/media/base/user_input_monitor_win.cc#newcode40 media/base/user_input_monitor_win.cc:40: // Private interface of UserInputMonitor. On 2013/09/10 18:28:39, Sergey ...

7 years, 3 months ago (2013-09-10 18:34:15 UTC) #34

Wez

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc#newcode62 media/base/user_input_monitor_linux.cc:62: class UserInputMonitorLinux::Core : public base::MessagePumpLibevent::Watcher { nit: If you ...

7 years, 3 months ago (2013-09-10 21:09:00 UTC) #35

jiayl

PTAL. Thanks! https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc#newcode62 media/base/user_input_monitor_linux.cc:62: class UserInputMonitorLinux::Core : public base::MessagePumpLibevent::Watcher { On ...

7 years, 3 months ago (2013-09-10 23:14:18 UTC) #36

PTAL. Thanks!

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:62: class UserInputMonitorLinux::Core :
public base::MessagePumpLibevent::Watcher {
On 2013/09/10 21:09:00, Wez wrote:
> nit: If you rename this UserInputMonitorLinuxCore and define it before
> UserInputMonitorLinux, above, then you can avoid needing to forward-define it
in
> UIML's private section.

Done.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:103: Display* display_;
On 2013/09/10 21:09:00, Wez wrote:
> nit: Suggest |display_control_| and |display_record_| for these, to make it
> clearer why you need two.

Done.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:127: DCHECK_EQ(0u,
mouse_listeners_count_);
On 2013/09/10 21:09:00, Wez wrote:
> nit: You could also mouse_listeners_.AssertEmpty() here if you want.

Done.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:138: {
On 2013/09/10 21:09:00, Wez wrote:
> If you move |lock_|, |mouse_listeners_count_|, |weak_factory_| into the main
> class, and keep a copy of |io_task_runner_| there as well then you can move
the
> lock/increment/post logic back to the outer class.
lock is removed now; "post" cannot be done from the outer class because the
listener list must be updated on the calling thread.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:140: mouse_listeners_count_++;
On 2013/09/10 21:09:00, Wez wrote:
> Why not keep this count on the IO thread, and increment/decrement when
> processing Start/StopMonitor()? Then you wouldn't need the lock.

Done.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:171: void
UserInputMonitorLinux::Core::StartKeyboardMonitoring() {
On 2013/09/10 21:09:00, Wez wrote:
> If you move |weak_factory_| into the outer class, and keep a copy of
> |io_task_runner_| there then you don't need these boilerplate methods - you
can
> Bind() to Start/StopMonitor directly from StartKeyboardMonitoring instead.

Done.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:189: counter_.Reset();
On 2013/09/10 21:09:00, Wez wrote:
> You're resetting the counter every time StartKeyboardMonitoring() is called -
> doesn't that mean that if you get multiple calls then earlier callers might be
> confused by the event count being reset when later callers start monitoring?
The base class makes sure that StartKeyboardMonitoring is only called for the
first external caller, so the count is consistent for each caller.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
File media/base/user_input_monitor_win.cc (right):

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:52: class UserInputMonitorWin::Core {
On 2013/09/10 21:09:00, Wez wrote:
> See comment on Linux impl

Done.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_win.cc:88: size_t mouse_listeners_count_;
On 2013/09/10 21:09:00, Wez wrote:
> See comments on Linux impl

Done.

Wez

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc#newcode138 media/base/user_input_monitor_linux.cc:138: { On 2013/09/10 23:14:19, jiayl wrote: > On 2013/09/10 ...

7 years, 3 months ago (2013-09-11 08:03:42 UTC) #37

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:138: {
On 2013/09/10 23:14:19, jiayl wrote:
> On 2013/09/10 21:09:00, Wez wrote:
> > If you move |lock_|, |mouse_listeners_count_|, |weak_factory_| into the main
> > class, and keep a copy of |io_task_runner_| there as well then you can move
> the
> > lock/increment/post logic back to the outer class.
> lock is removed now; "post" cannot be done from the outer class because the
> listener list must be updated on the calling thread.

Sorry, I don't understand that logic.  Calling PostTask() in
UserInputMonitor::AddMouseListener() immediately after core_->AddMouseListener()
is equivalent to calling it here, surely?

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:189: counter_.Reset();
On 2013/09/10 23:14:19, jiayl wrote:
> On 2013/09/10 21:09:00, Wez wrote:
> > You're resetting the counter every time StartKeyboardMonitoring() is called
-
> > doesn't that mean that if you get multiple calls then earlier callers might
be
> > confused by the event count being reset when later callers start monitoring?
> The base class makes sure that StartKeyboardMonitoring is only called for the
> first external caller, so the count is consistent for each caller. 

Why not do the same for the mouse? i.e:

AddMouseListener(listener) // adds the Observer
Lock
mouse_listener_count_++
if (mouse_listener_count == 1)
  StartMonitor(MOUSE_TYPE);
Unlock

So long as the counter increment/decrement & Start/Stop occur with the locked
held this won't be racey.

https://codereview.chromium.org/23702008/diff/65001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/65001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:58: return weak_factory_.GetWeakPtr();
You can avoid this ugly WeakPtr getter by keeping the |weak_factory_| in
UserInputMonitor and posting the OnMouseListenerAdded/Removed() tasks from its
Add/RemoveMouseListener() methods.

You just have to remember to |weak_factory_.Invalidate()| in ~UserInputMonitor()
before you call DeleteSoon() on the core.

jiayl

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc#newcode138 media/base/user_input_monitor_linux.cc:138: { On 2013/09/11 08:03:42, Wez wrote: > On 2013/09/10 ...

7 years, 3 months ago (2013-09-11 16:57:10 UTC) #38

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:138: {
On 2013/09/11 08:03:42, Wez wrote:
> On 2013/09/10 23:14:19, jiayl wrote:
> > On 2013/09/10 21:09:00, Wez wrote:
> > > If you move |lock_|, |mouse_listeners_count_|, |weak_factory_| into the
main
> > > class, and keep a copy of |io_task_runner_| there as well then you can
move
> > the
> > > lock/increment/post logic back to the outer class.
> > lock is removed now; "post" cannot be done from the outer class because the
> > listener list must be updated on the calling thread.
> 
> Sorry, I don't understand that logic.  Calling PostTask() in
> UserInputMonitor::AddMouseListener() immediately after
core_->AddMouseListener()
> is equivalent to calling it here, surely?
Core::AddMouseListener updates the listener list in the calling thread, not in
the posted task. If we don't update the listener list in the calling thread, we
may call into destroyed listener since the caller may destroy the listener
immediately after calling RemoveListener.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:189: counter_.Reset();
On 2013/09/11 08:03:42, Wez wrote:
> On 2013/09/10 23:14:19, jiayl wrote:
> > On 2013/09/10 21:09:00, Wez wrote:
> > > You're resetting the counter every time StartKeyboardMonitoring() is
called
> -
> > > doesn't that mean that if you get multiple calls then earlier callers
might
> be
> > > confused by the event count being reset when later callers start
monitoring?
> > The base class makes sure that StartKeyboardMonitoring is only called for
the
> > first external caller, so the count is consistent for each caller. 
> 
> Why not do the same for the mouse? i.e:
> 
> AddMouseListener(listener) // adds the Observer
> Lock
> mouse_listener_count_++
> if (mouse_listener_count == 1)
>   StartMonitor(MOUSE_TYPE);
> Unlock
> 
> So long as the counter increment/decrement & Start/Stop occur with the locked
> held this won't be racey.

What's the benefit? I see two shortcomings instead: 1. the listener count and
the listener list will be scattered in two classes thus harder to maintain; 2.
it reintroduces the lock that was removed in this patch.

https://codereview.chromium.org/23702008/diff/65001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/65001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:58: return weak_factory_.GetWeakPtr();
On 2013/09/11 08:03:42, Wez wrote:
> You can avoid this ugly WeakPtr getter by keeping the |weak_factory_| in
> UserInputMonitor and posting the OnMouseListenerAdded/Removed() tasks from its
> Add/RemoveMouseListener() methods.
> 
> You just have to remember to |weak_factory_.Invalidate()| in
~UserInputMonitor()
> before you call DeleteSoon() on the core.
To reiterate why PostTask to IO thread from UserInputMonitor will not work:
1. the listener list has to live in Core so that Core does not need to callback
to UserInputMonitor. Calling back to UserInputMonitor is problematic because
WeakPtr does not work across threads (i.e. UserInputMonitor will be deref'd on
IO thread but invalidated on UI thread) and we don't want to add ref counting
for UserInputMonitor.
2. Because of #1, Core is responsible to update the listener list. It has to
happen on the calling thread, since the caller may destroy the listener
immediately after calling RemoveListener. So we cannot PostTask to IO thread
from UserInputMonitor.

Sergey Ulanov

Wez, I think it's better if we don't call X Window or Windows APIs with ...

7 years, 3 months ago (2013-09-11 19:25:03 UTC) #39

Wez

What I suggested was e.g. for AddMouseListener(MouseListener): 1. Add MouseListener to thread-safe ObserverList. 2. Take ...

7 years, 3 months ago (2013-09-12 11:36:18 UTC) #40

jiayl

Moved the common logic of adding/removing mouse listeners into the base class to avoid duplication. ...

7 years, 3 months ago (2013-09-12 19:19:23 UTC) #41

Wez

LGTM but please see SupportsWeakPtr comments. https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_monitor_linux.cc#newcode138 media/base/user_input_monitor_linux.cc:138: { On 2013/09/10 ...

7 years, 3 months ago (2013-09-13 10:01:26 UTC) #42

LGTM but please see SupportsWeakPtr comments.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:138: {
On 2013/09/10 23:14:19, jiayl wrote:
> On 2013/09/10 21:09:00, Wez wrote:
> > If you move |lock_|, |mouse_listeners_count_|, |weak_factory_| into the main
> > class, and keep a copy of |io_task_runner_| there as well then you can move
> the
> > lock/increment/post logic back to the outer class.
> lock is removed now; "post" cannot be done from the outer class because the
> listener list must be updated on the calling thread.

True, you would need to add the observer from the outer class as well , before
posting StartMonitor() to the Core.

https://codereview.chromium.org/23702008/diff/3001/media/base/user_input_moni...
media/base/user_input_monitor_linux.cc:138: {
On 2013/09/11 16:57:10, jiayl wrote:
> On 2013/09/11 08:03:42, Wez wrote:
> > On 2013/09/10 23:14:19, jiayl wrote:
> > > On 2013/09/10 21:09:00, Wez wrote:
> > > > If you move |lock_|, |mouse_listeners_count_|, |weak_factory_| into the
> main
> > > > class, and keep a copy of |io_task_runner_| there as well then you can
> move
> > > the
> > > > lock/increment/post logic back to the outer class.
> > > lock is removed now; "post" cannot be done from the outer class because
the
> > > listener list must be updated on the calling thread.
> > 
> > Sorry, I don't understand that logic.  Calling PostTask() in
> > UserInputMonitor::AddMouseListener() immediately after
> core_->AddMouseListener()
> > is equivalent to calling it here, surely?
> Core::AddMouseListener updates the listener list in the calling thread, not in
> the posted task. If we don't update the listener list in the calling thread,
we
> may call into destroyed listener since the caller may destroy the listener
> immediately after calling RemoveListener.

Yes, see above re updating the listener list from the outer class.

https://codereview.chromium.org/23702008/diff/65001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/65001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:58: return weak_factory_.GetWeakPtr();
On 2013/09/11 16:57:10, jiayl wrote:
> On 2013/09/11 08:03:42, Wez wrote:
> > You can avoid this ugly WeakPtr getter by keeping the |weak_factory_| in
> > UserInputMonitor and posting the OnMouseListenerAdded/Removed() tasks from
its
> > Add/RemoveMouseListener() methods.
> > 
> > You just have to remember to |weak_factory_.Invalidate()| in
> ~UserInputMonitor()
> > before you call DeleteSoon() on the core.
> To reiterate why PostTask to IO thread from UserInputMonitor will not work:
> 1. the listener list has to live in Core so that Core does not need to
callback
> to UserInputMonitor. Calling back to UserInputMonitor is problematic because
> WeakPtr does not work across threads (i.e. UserInputMonitor will be deref'd on
> IO thread but invalidated on UI thread) and we don't want to add ref counting
> for UserInputMonitor.
> 2. Because of #1, Core is responsible to update the listener list. It has to
> happen on the calling thread, since the caller may destroy the listener
> immediately after calling RemoveListener. So we cannot PostTask to IO thread
> from UserInputMonitor.

You could add the listener from UserInputMonitor, though, before posting the
task.

It'd mean providing a scoped_refptr<> mouse_listeners() getter for
UserInputMonitor to use, but since Core is an internal implementation detail,
that seems reasonable, and it saves the boilerplate Add/RemoveMouseListener()
and GetWeakPtr() calls on Core.

You'd need to re-instate the Lock around the mouse counter updates for that to
work, but since that's how keyboard is handled, that doesn't seem unreasonable.

https://codereview.chromium.org/23702008/diff/20002/media/base/user_input_mon...
File media/base/user_input_monitor.h (right):

https://codereview.chromium.org/23702008/diff/20002/media/base/user_input_mon...
media/base/user_input_monitor.h:81: size_t mouse_listeners_count_;
nit: Name these two counters consistently, e.g. mouse_monitor_count_,
keyboard_monitor_count_

https://codereview.chromium.org/23702008/diff/20002/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/23702008/diff/20002/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:41: public
base::SupportsWeakPtr<UserInputMonitorLinuxCore> {
You have to be careful using SupportsWeakPtr - WeakPtrs it dispenses are only
invalidated when the base-class dtor runs, which only happens after the derived
class and its members have been destroyed.

I think it's cleaner here to have UserInputMonitorLinux create a WeakPtrFactory
wrapping the Core, and to explicitly Invalidate() that before posting the
deletion task for the Core.

https://codereview.chromium.org/23702008/diff/20002/media/base/user_input_mon...
File media/base/user_input_monitor_win.cc (right):

https://codereview.chromium.org/23702008/diff/20002/media/base/user_input_mon...
media/base/user_input_monitor_win.cc:30: : public
base::SupportsWeakPtr<UserInputMonitorWinCore> {
See comment on Linux impl re SupportsWeakPtr

jiayl

https://codereview.chromium.org/23702008/diff/20002/media/base/user_input_monitor_linux.cc File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/23702008/diff/20002/media/base/user_input_monitor_linux.cc#newcode41 media/base/user_input_monitor_linux.cc:41: public base::SupportsWeakPtr<UserInputMonitorLinuxCore> { On 2013/09/13 10:01:27, Wez wrote: > ...

7 years, 3 months ago (2013-09-13 16:23:32 UTC) #43