1357880 - Telemetry for keyboard APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P2)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

We want to add telemetry probes to measure how often pages have non-passive keyboard event listeners on the root content document/root scrollframe. We might also want to know how often pages have mousemove events on the root, as it will affect whether we want to do the bonus step above.

See bug 1351783 comment 1 for background.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Ryan, can you help with this?  Kats can provide details.  We'd want it sooner rather than later.

Comment 2

[Ryan Hunt [:rhunt]]

Yes, I'll take a look at this.

Comment 3

[Ryan Hunt [:rhunt]]

Discussing this on #apz, I think we have a potential implementation plan for checking keyboard event listeners.

1. Add a 'everHadNonPassiveKeyListener' flag to EventListenerManager and populate it
2. Add a method like HasDocumentLevelListenersForApzAwareEvents()[1] that checks for this flag
3. Use this method in ~nsDocument (or wherever fits for the end of a document and it's content) to check if the page has ever had a non passive key listener that would impact keyboard-apz

Olli, is this reasonable, or is there a better way to do this?

[1] https://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#8621

Comment 4

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

HasDocumentLevelListenersForApzAwareEvents sounds wrong, since it is more like window, document, document.documentElement or document.body.

~nsDocument might not be the best place since then one would need to consider also data documents etc.
Perhaps inner window would be better place to report.
We have already Telemetry::INNERWINDOWS_WITH_MUTATION_LISTENERS there.

Comment 5

[Ryan Hunt [:rhunt]]

Attachment: Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

This commit adds a telemetry probe to track the percentage of pages that ever
have a non-passive 'keydown' or 'keypress' event that could preventDefault()
keyboard scrolling of the root of a page.

A flag is added to each EventListenerManager to track whether it ever had
a qualifying event listener, and then in nsGlobalWindow::PageHidden() the
event targets that could preventDefault() a scroll are checked for this flag.
This check is done at nsGlobalWindow::PageHidden() so that the DOM is still
alive.

Review commit: https://reviewboard.mozilla.org/r/134248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134248/

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: Bug 1357880 - Add a telemetry probe for mousemove event listeners

This commit adds a telemetry probe to determine the percentage of pages
that ever have a 'mousemove' event listener added to the DOM.

A flag is added to nsPIDOMWindow to track whether a qualifying event
listener was ever added to the DOM for this window, and is updated by
EventListenerManager. There are several other similar flags to this.

The probe is reported in nsGlobalWindow::PageHidden() so that it can
be compared exactly with the non-passive keyboard listener APZ probe.

Review commit: https://reviewboard.mozilla.org/r/134250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134250/

Comment 7

[Ryan Hunt [:rhunt]]

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

Mostly looks fine.

There is one failure I'm not sure about [1], a content process hang that is spewing out:

'###!!! [Child][MessageChannel] Error: (msgtype=0x4800FA,name=PContent::Msg_AccumulateChildKeyedHistograms) Closed channel: cannot send/recv'

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=95d95660074fee17fc95608c095ddc928aeb47ec&selectedJob=94745074

Comment 8

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

Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

https://reviewboard.mozilla.org/r/134248/#review137252

::: dom/base/nsGlobalWindow.cpp:10788
(Diff revision 1)
>  void
>  nsGlobalWindow::PageHidden()
>  {
>    FORWARD_TO_INNER_VOID(PageHidden, ());
>  
> +  nsIDocument* doc = GetDocument();

This is wrong place to report this. We end up reporting also pages entering bfcache.
And if the page comes out from bfcache and re-enters there, we end up reporting data about the same page again.

Could you possible move this to FreeInnerObjects();?

Comment 9

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

Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

https://reviewboard.mozilla.org/r/134250/#review137256

::: commit-message-b5e0b:1
(Diff revision 1)
> +Bug 1357880 - Add a telemetry probe for mousemove event listeners r?smaug

Why we need this whole thing?

::: toolkit/components/telemetry/Histograms.json:10540
(Diff revision 1)
>      "bug_numbers": [1352654],
>      "expires_in_version": "58",
>      "kind": "boolean",
>      "description": "The percentage of pages with a non passive key event on the path to the root scrolling element."
>    },
> +  "APZ_AWARE_KEYBOARD_MOUSE_MOVE_LISTENERS": {

Weird name for the probe. Why APZ_AWARE in the name, or KEYBOARD

Comment 10

[Ryan Hunt [:rhunt]]

Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134248/diff/1-2/

Comment 11

[Ryan Hunt [:rhunt]]

Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134250/diff/1-2/

Comment 12

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8862314 [details]
> Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners
> 
> https://reviewboard.mozilla.org/r/134248/#review137252
> 
> ::: dom/base/nsGlobalWindow.cpp:10788
> (Diff revision 1)
> >  void
> >  nsGlobalWindow::PageHidden()
> >  {
> >    FORWARD_TO_INNER_VOID(PageHidden, ());
> >  
> > +  nsIDocument* doc = GetDocument();
> 
> This is wrong place to report this. We end up reporting also pages entering
> bfcache.
> And if the page comes out from bfcache and re-enters there, we end up
> reporting data about the same page again.
> 
> Could you possible move this to FreeInnerObjects();?

I've moved it to FreeInnerObjects() and that works. I was aware we'd report multiple times, but it would be only when a user goes to a page and comes back, which is a measure of use. But this will be a better measure of distinct pages.

Comment 13

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8862315 [details]
> Bug 1357880 - Add a telemetry probe for mousemove event listeners
> 
> https://reviewboard.mozilla.org/r/134250/#review137256
> 
> ::: commit-message-b5e0b:1
> (Diff revision 1)
> > +Bug 1357880 - Add a telemetry probe for mousemove event listeners r?smaug
> 
> Why we need this whole thing?
> 
> ::: toolkit/components/telemetry/Histograms.json:10540
> (Diff revision 1)
> >      "bug_numbers": [1352654],
> >      "expires_in_version": "58",
> >      "kind": "boolean",
> >      "description": "The percentage of pages with a non passive key event on the path to the root scrolling element."
> >    },
> > +  "APZ_AWARE_KEYBOARD_MOUSE_MOVE_LISTENERS": {
> 
> Weird name for the probe. Why APZ_AWARE in the name, or KEYBOARD

This probe is for determining how often APZ key scrolling could handle interleaved mousemove events vs. not because there is a listener on the page. From my understanding of the bonus step in bug 1351783 comment 1.

The name of the probe was gross though so I've simplified it a bit. I'm still open to suggestions. The prefix APZ_AWARE is just because this probe is going to be used for determining how many event listeners are 'apz aware'.

The mousemove one doesn't necessarily need the prefix because nothing is APZ specific about the probe, only the use of it.

Comment 14

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

It is still a bit mystery to me why mousemove is so special. Focus can change at any point.
But ok, perhaps we can add the telemetry.

Comment 15

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

Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

https://reviewboard.mozilla.org/r/134248/#review137608

Comment 16

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

Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

https://reviewboard.mozilla.org/r/134250/#review137610

r+, but before landing this, ask mstange or kats whether we actually need this.
(these probes do after all slow down other execution a bit and make code a tad more complicated)

Comment 17

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

Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

https://reviewboard.mozilla.org/r/134250/#review137614

::: dom/base/nsPIDOMWindow.h:822
(Diff revision 2)
>  
>    /**
>     * Call this to check whether some node (this window, its document,
> +   * or content in that document) has or had a mousemove event listener.
> +   */
> +  bool HasMouseMoveEventListeners()

Call this something a tad more unreliable, since we don't catch cases when nodes with event listeners are moved from one document to another.
Perhaps negate it and call it

ProbablyDoesNotHaveMouseMoveListeners().


Or do what we do with mMayHaveMouseEnterLeaveEventListener and deal with this stuff properly, by updating the flag when adopting nodes.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(In reply to Olli Pettay [:smaug] from comment #16)
> r+, but before landing this, ask mstange or kats whether we actually need
> this.

Yeah I think we do. IIRC the plan was to allow APZ to do keyboard scrolling as long as we knew where the focus was, but if any non-keyboard event came through we would stop doing APZ keyboard scrolling until we can reconfirm the focus. This is because any of those events could change the focus via listeners. We further assumed that the most frequent events would be mousemove events, and so if we could maintain APZ keyboard scrolling even with interleaved mousemove events that would be an improvement (the "bonus" step in bug 1351783), but we can only do that if we know there are no mousemove listeners that could change the focus. Therefore this telemetry probe will tell us how many pages we can do this on.

Comment 19

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

focus can change at any point, not only by event listeners. And mousemoves rarely change focus, so I'm still having hard time to understand the reasoning (as I did in the meeting :) ).

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Yes, but if it changes by non-event listeners (i.e. via setTimeout) then there isn't an expectation of deterministic behaviour by the user. If they generate a sequence of input events then there is an expectation that the behaviour will be the same every time. If the focus is influenced by some non-user-input factor, then they can't really rely on it always happening at a particular point in their sequence of keyboard input. So even if we scroll the wrong thing for the next few events (until the repaint happens and the focus state is pushed over to APZ) it should be acceptable. I agree it's not ideal but basically we want to try and see if we can get away with it.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Also with respect to the performance implications of the telemetry probes - we don't need to let these probes go to release. Data from the beta channel should be sufficient. Anyway we would like to implement the keyboard scrolling pretty soon so we can't really afford to wait to get release-channel data.

Comment 22

[Ryan Hunt [:rhunt]]

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8862315 [details]
> Bug 1357880 - Add a telemetry probe for mousemove event listeners
> 
> https://reviewboard.mozilla.org/r/134250/#review137614
> 
> ::: dom/base/nsPIDOMWindow.h:822
> (Diff revision 2)
> >  
> >    /**
> >     * Call this to check whether some node (this window, its document,
> > +   * or content in that document) has or had a mousemove event listener.
> > +   */
> > +  bool HasMouseMoveEventListeners()
> 
> Call this something a tad more unreliable, since we don't catch cases when
> nodes with event listeners are moved from one document to another.
> Perhaps negate it and call it
> 
> ProbablyDoesNotHaveMouseMoveListeners().
> 
> 
> Or do what we do with mMayHaveMouseEnterLeaveEventListener and deal with
> this stuff properly, by updating the flag when adopting nodes.

I'll update the patch to deal with this properly. I missed the code in nsNodeUtils.

Comment 23

[Ryan Hunt [:rhunt]]

Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134248/diff/2-3/

Comment 24

[Ryan Hunt [:rhunt]]

Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134250/diff/2-3/

Comment 25

[Ryan Hunt [:rhunt]]

I've update the patches with two changes,

1. I renamed ELM::EverHadAPZKeyAwareListeners to ELM::MayHaveAPZAwareKeyEventListener, so that it matches with all the other properties of the same type. This is patch 1, no functional changes.

2. I added the code to handle moving event listeners from one document to another in nsNodeUtils. This is patch 2.

Comment 26

[Ryan Hunt [:rhunt]]

Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134250/diff/3-4/

Comment 27

[Ryan Hunt [:rhunt]]

One more update to update a histogram description. No functional changes.

Comment 28

[Ryan Hunt [:rhunt]]

Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

Flag for data-review.

We don't plan on allowing this probe to go to release. We should get enough data before then.

Comment 29

[Ryan Hunt [:rhunt]]

Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

Flag for data-review

Comment 30

[Benjamin Smedberg]

Comment on attachment 8862314 [details]
Bug 1357880 - Add a telemetry probe for non-passive keyboard event listeners

https://reviewboard.mozilla.org/r/134248/#review138074

data-r=me with that clarification in the histogram description.

::: toolkit/components/telemetry/Histograms.json:10436
(Diff revision 3)
> +  "APZ_AWARE_KEY_LISTENERS": {
> +    "alert_emails": ["[e-mail]"],
> +    "bug_numbers": [1352654],
> +    "expires_in_version": "58",
> +    "kind": "boolean",
> +    "description": "The percentage of pages with a non-passive key event on the path to the root scrolling element that would disable APZ key scrolling."

Is this limited to certain kinds of pages? e.g.

* chrome windows?
* only toplevel windows or all subframes?

Please at least document these questions in the histogram description for clarity and testing purposes.

Comment 31

[Benjamin Smedberg]

Comment on attachment 8862315 [details]
Bug 1357880 - Add a telemetry probe for mousemove event listeners

https://reviewboard.mozilla.org/r/134250/#review138078

data-r=me with clarifications

::: toolkit/components/telemetry/Histograms.json:10443
(Diff revision 4)
> +  "APZ_AWARE_MOUSEMOVE_LISTENERS": {
> +    "alert_emails": ["[e-mail]"],
> +    "bug_numbers": [1352654],
> +    "expires_in_version": "58",
> +    "kind": "boolean",
> +    "description": "The percentage of pages with a mousemove listener anywhere in the document that would disable APZ key scrolling."

ditto comments here about chrome windows/toplevel/subframes.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7129e53f3a9c
Add a telemetry probe for non-passive keyboard event listeners r=smaug, data-review=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/915489b79bbf
Add a telemetry probe for mousemove event listeners r=smaug, data-review=bsmedberg

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7129e53f3a9c
https://hg.mozilla.org/mozilla-central/rev/915489b79bbf

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Data is starting to come in. On nightly the APZ_AWARE_KEY_LISTENERS flag is true ~13.89% of the time. APZ_AWARE_MOUSEMOVE_LISTENERS is true ~12.03% of the time.

Comment 35

[Milan Sreckovic [:milan] (needinfo for best results)]

Nice.  Can we tell how many have both, and consequently, how many just have one?

Comment 36

[Kartikaya Gupta (email:[e-mail])]

Not with the probes as they are. I posted some analysis over at bug 1351783 comment 4.