1340904 - Rewrite the telemetry scroll probes in C++

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ryan Hunt [:rhunt] (on leave until early May)]

After bug 1338891, bug 1297867 and bug 1312881 were backed out of beta, aurora, and central. This bug is for rewriting the scroll probes in c++ in a way that does no significantly affect performance.

Comment 1

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: scroll-probe.patch

An evolution of the previous patch.

The max scroll and amount scroll metrics are updated by FireScrollEvent in nsGfxScrollFrame and stored in the PresContext of any root scroll frame. The metrics are submitted by TelemetryScrollProbe which listens for 'pagehide' events coming from the frame script global.

I've verified that this patch accurately records the same metric that the original JS probe did.

This approach isn't the cleanest, but it seems like it is sensible. Please let me know if there is a better/cleaner way.

Thanks for all your help!

Comment 2

[Ryan Hunt [:rhunt] (on leave until early May)]

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

It looks like this patch is leaking the world, and potentially has a crash in it.

Comment 3

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

Comment on attachment 8842749 [details] [diff] [review]
scroll-probe.patch

>+/* static */ void
>+TelemetryScrollProbe::Create(TabChildGlobal* aWebFrame) {
Nit, { goes to its own line after method definition. Same thing also elsewhere.


But I'll re-read once the leak is fixed.

Comment 4

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

Could you use nsIDocument::GetOriginalURI and not Location().
And it isn't clear to me why about:blank is filtered out.

And using nsAutoString on stack is probably faster than nsString.

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: scroll-probe.patch

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

It looks like the leak has been cleaned up.

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Olli Pettay [:smaug] from comment #4)
> And it isn't clear to me why about:blank is filtered out.

about:blank, about:newtab, etc. are excluded from this probe so that we focus on web page interaction that isn't 'chrome' or controlled by firefox. We're not sure if that's important or not, but it's not hard to test for.

Comment 7

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

about:blank in general isn't content or chrome. Web pages do use about:blank all the time.
Maybe you want principal check and store data only about non-system-principal documents?
!nsContentUtils::IsSystemPrincipal(nsIDocument_object->NodePrincipal())


(and about:newtab seems to run in the parent process. )

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Olli Pettay [:smaug] from comment #7)
> about:blank in general isn't content or chrome. Web pages do use about:blank
> all the time.
> Maybe you want principal check and store data only about
> non-system-principal documents?
> !nsContentUtils::IsSystemPrincipal(nsIDocument_object->NodePrincipal())
> 
> 
> (and about:newtab seems to run in the parent process. )

Ah yeah, you're right I didn't mean chrome in the strict sense. But I didn't know web pages actually use about:blank, interesting.

A principal check could be good. That could work better than looking at the scheme.

Comment 9

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

Comment on attachment 8843072 [details] [diff] [review]
scroll-probe.patch

>+bool
>+TelemetryScrollProbe::ShouldIgnore(nsIDOMEvent* aEvent) const
>+{
>+  nsCOMPtr<nsIDOMEventTarget> target;
>+  aEvent->GetTarget(getter_AddRefs(target));
>+  nsCOMPtr<nsIDocument> targetDocument = do_QueryInterface(target);
>+  RefPtr<nsIDocument> document = GetDocument();
>+
>+  // only respond to the root, ignore 'pagehide' from iframes
>+  if (!document || targetDocument != document) {
>+    return false;
>+  }
>+
>+  // skip about:// pages for this probe
>+  nsCOMPtr<nsIURI> location = document->GetOriginalURI();
>+  nsAutoCString scheme;
>+  return !location || NS_FAILED(location->GetScheme(scheme)) || scheme.EqualsLiteral("about");
>+}
So I think we can simplify this a bit and just have (I think this captures the meaning well enough)

if (!document || targetDocument != document || nsContentUtils::IsSystemPrincipal(document->NodePrincipal())) {
  return false;
}

return true;


>+  void SetTelemetryScrollY(nscoord aScrollY) {
>+    nscoord delta = abs(aScrollY - mTelemetryScrollLastY);
>+    mTelemetryScrollLastY = aScrollY;
>+
>+    mTelemetryScrollTotalY += delta;
>+    if (aScrollY > mTelemetryScrollMaxY) {
>+      mTelemetryScrollMaxY = aScrollY;
>+    }
>+  }
>+  nscoord TelemetryScrollMaxY() const {
>+    return mTelemetryScrollMaxY;
>+  }
>+  nscoord TelemetryScrollTotalY() const {
>+    return mTelemetryScrollTotalY;
>+  }
I know some of nsPresContext uses wrong coding style, but { after method definition should go to its own line

Comment 10

[Ryan Hunt [:rhunt] (on leave until early May)]

Testing with (In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8843072 [details] [diff] [review]
> scroll-probe.patch
>
> So I think we can simplify this a bit and just have (I think this captures
> the meaning well enough)
> 
> if (!document || targetDocument != document ||
> nsContentUtils::IsSystemPrincipal(document->NodePrincipal())) {
>   return false;
> }
> 
> return true;

Hm. So running this code (inverted the condition because it's ShouldIgnore()) I'm getting about: pages recorded.  So that's making me guess that about: pages are not running under the system principal?

Is there any other way to detect pages in about:, or is looking at the URI the best shot?

Comment 11

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

some about: pages use system principal (at least in parent process, but that isn't relevant here), and some don't.
about:blank inherits principal from load triggerer, and in content-docshell case it is by default non-system principal.

which about:* which doesn't have system principal are you worried about?

We can leave the explicit about: check there too, but it feels a bit wrong.


And oops, it was ShouldIgnore, so !nsContentUtils::IsSystemPrincipal(document->NodePrincipal())
Though, I think !document should be ignored

Comment 12

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Olli Pettay [:smaug] from comment #11)
> which about:* which doesn't have system principal are you worried about?

I think the concern is only about biasing towards 0 with this metric because every user has to go through about:home, which generally doesn't scroll.

But, this whole metric is hypothetical if it will be even useful, and any noise from about:home will be before Quantum changes and after. So I'd rather not worry much about it, and just stick with checking for the system principal.

I'm going to do some more testing on this to make sure it's okay before landing, I'd rather not cause a third problem with this probe.

Comment 13

[:Harald Kirschner :digitarald]

> So I'd rather not worry much about it, and just stick with checking for the system principal.

+1, bias from about: is not expected to cause noise and if people engage on them more it might even be interesting to know.

Comment 14

[Ryan Hunt [:rhunt] (on leave until early May)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ba15fb30b0456a00a6fbad8d52e77db39abbf64&selectedJob=81517700

It looks green to me.

Comment 15

[Ryan Hunt [:rhunt] (on leave until early May)]

Harald, should we get this landed or is there anything you want to wait for or discuss?

Comment 16

[:Harald Kirschner :digitarald]

Looks good to me. Need to have this landed to see how it looks in the wild.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11b27d2c87e7
Implement telemetry scroll tracking in C++ r=smaug

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11b27d2c87e7

Comment 19

[Chris H-C :chutten]

Did these new probes receive data-review from a Data Peer per https://wiki.mozilla.org/Firefox/Data_Collection ?

Comment 20

[Chris H-C :chutten]

(For the record I see that the backed-out bugs _did_ receive data-review, but these new ones appear unreviewed)

Comment 21

[Ryan Hunt [:rhunt] (on leave until early May)]

No the new patches did not receive data-review. The new patches have a different implementation, but measure the same metric as before. The histograms are the exact same as the ones backed out, so no descriptions or expire date change.

I didn't think about data-review, do we need it in this case?

Comment 22

[Benjamin Smedberg]

If the histogram is the same and has the same meaning then you don't need a data review.