1343765 - Label timer in dom/events

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement, P2)

Description

[Ming-Chou Shih [:stone]]

Timer gUserInteractionTimerCallback in EventStateManager.cpp

Comment 1

[Ming-Chou Shih [:stone]]

Attachment: Label timer in dom/events

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8849880 [details] [diff] [review]
Label timer in dom/events

Review of attachment 8849880 [details] [diff] [review]:
-----------------------------------------------------------------

Per offline discussion, the gUserInteractionTimer is a singleton to serve multiple documents/windows.
Hence, some design change is expected to call the timercallback from different event target.
In addition, if we keep gUserInteractionTimer as a singleton, we should make sure that the timer was cancel before we init another run of timercallback with different event target to prevent that the previous callback was invoked with the event target we set later for next InitWithCallback.

::: dom/events/EventStateManager.cpp
@@ +341,5 @@
> +    return;
> +  if (!gUserInteractionTimer && mPresContext) {
> +    EnsureDocument(mPresContext);
> +    CallCreateInstance("@mozilla.org/timer;1", &gUserInteractionTimer);
> +    nsCOMPtr<nsPIDOMWindowOuter> win(mDocument->GetWindow());

You can get DocGroup EventTarget via mDocument->EventTargetFor() without getting TabGroup from its outter window.

@@ +1432,5 @@
> +    EnsureDocument(mPresContext);
> +    nsCOMPtr<nsPIDOMWindowOuter> win(mDocument->GetWindow());
> +    NS_ENSURE_TRUE_VOID(win);
> +    mClickHoldTimer->SetTarget(
> +      win->TabGroup()->EventTargetFor(TaskCategory::UI));

ditto

Comment 3

[Ming-Chou Shih [:stone]]

Attachment: Label timer in dom/events (V2)

Comment 4

[Ming-Chou Shih [:stone]]

Comment on attachment 8850260 [details] [diff] [review]
Label timer in dom/events (V2)

Maybe we should use SystemGroup for UserInteractionTimer since it's not bound to a specific document.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8850260 [details] [diff] [review]
Label timer in dom/events (V2)

Review of attachment 8850260 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my comment below.

Thanks!

::: dom/events/EventStateManager.cpp
@@ +343,4 @@
>      CallCreateInstance("@mozilla.org/timer;1", &gUserInteractionTimer);
> +    if (gUserInteractionTimer) {
> +      gUserInteractionTimer->SetTarget(
> +        SystemGroup::EventTargetFor(TaskCategory::Other));

We have to use SystemGroup carefully to prevent an assertion similar to 
https://bugzilla.mozilla.org/show_bug.cgi?id=1339006#c11

The notification of user-interaction-active/user-interaction-inactive will trigger a JS callback in SessionRecorder:
http://searchfox.org/mozilla-central/source/toolkit/modules/SessionRecorder.jsm#390-396

Comprehensive test is required to ensure that SessionRecorder and its user are chrome-privileged to prevent hitting this assertion.

Comment 6

[Ming-Chou Shih [:stone]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #5)
> Comment on attachment 8850260 [details] [diff] [review]
> Label timer in dom/events (V2)
> 
> Review of attachment 8850260 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comment below.
> 
> Thanks!
> 
> ::: dom/events/EventStateManager.cpp
> @@ +343,4 @@
> >      CallCreateInstance("@mozilla.org/timer;1", &gUserInteractionTimer);
> > +    if (gUserInteractionTimer) {
> > +      gUserInteractionTimer->SetTarget(
> > +        SystemGroup::EventTargetFor(TaskCategory::Other));
> 
> We have to use SystemGroup carefully to prevent an assertion similar to 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1339006#c11
> 
> The notification of user-interaction-active/user-interaction-inactive will
> trigger a JS callback in SessionRecorder:
> http://searchfox.org/mozilla-central/source/toolkit/modules/SessionRecorder.
> jsm#390-396
> 
> Comprehensive test is required to ensure that SessionRecorder and its user
> are chrome-privileged to prevent hitting this assertion.
Tested with my local build and it works fine.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8850260 [details] [diff] [review]
Label timer in dom/events (V2)

>+    EnsureDocument(mPresContext);
Technically, what guarantees mPresContext is non-null here?

>+    mClickHoldTimer->SetTarget(mDocument->EventTargetFor(TaskCategory::UI));
>     mClickHoldTimer->InitWithFuncCallback(sClickHoldCallback, this,
>                                           clickHoldDelay,
>                                           nsITimer::TYPE_ONE_SHOT);
Given that this is really about a system level thing, not so much about some particular document,
I would just use SystemGroup::EventTargetFor(TaskCategory::Other)
(we don't want this kind of timer to be delayed if the docgroup is preempted temporarily)


That changed, r+

(also, I think CreateClickHoldTimer code is effectively dead. There is even a bug to remove this method)

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Label timer in dom/events (V3)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d950605fbc83379b1d19a68afb4be3d8db9d04e9

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c01b9f972f5a
Label timer in dom/events. f=bevistseng. r=smaug.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c01b9f972f5a