Closed Bug 1340904 Opened 3 years ago Closed 3 years ago

Rewrite the telemetry scroll probes in C++

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: rhunt, Assigned: rhunt)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch scroll-probe.patch (obsolete) — Splinter Review
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!
Attachment #8842749 - Flags: review?(bugs)
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 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.
Attachment #8842749 - Flags: review?(bugs)
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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43f59f36b971c5e12a335d38891ec440e5d6594c

It looks like the leak has been cleaned up.
Attachment #8842749 - Attachment is obsolete: true
Attachment #8843072 - Flags: review?(bugs)
(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.
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. )
(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 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
Attachment #8843072 - Flags: review?(bugs) → review+
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?
Flags: needinfo?(bugs)
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
Flags: needinfo?(bugs)
(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.
> 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.
Harald, should we get this landed or is there anything you want to wait for or discuss?
Flags: needinfo?(hkirschner)
Looks good to me. Need to have this landed to see how it looks in the wild.
Flags: needinfo?(hkirschner)
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11b27d2c87e7
Implement telemetry scroll tracking in C++ r=smaug
https://hg.mozilla.org/mozilla-central/rev/11b27d2c87e7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Did these new probes receive data-review from a Data Peer per https://wiki.mozilla.org/Firefox/Data_Collection ?
(For the record I see that the backed-out bugs _did_ receive data-review, but these new ones appear unreviewed)
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?
Flags: needinfo?(benjamin)
If the histogram is the same and has the same meaning then you don't need a data review.
Flags: needinfo?(benjamin)
You need to log in before you can comment on or make changes to this bug.