Closed Bug 1293222 Opened 3 years ago Closed 3 years ago

browser.engagement.total_uri_count includes URIs from restored tabs

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
3

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox50 --- verified
firefox51 --- verified

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 1 obsolete file)

browser.engagement.total_uri_count should only count http(s) URIs after the session has been restored.

When closing Nightly with 10+ tabs, some of the URIs are counted in the scalar probe, even if they shouldn't.

STR:

- Open Nightly
- Change the Preferences to open the tabs from the previous session when starting
- Open 10+ tabs with different URIs
- Close Nightly
- Open Nightly
- Go to about:telemetry
- The value of the scalar probe should be non zero.
Blocks: 1252625
Priority: -- → P1
Whiteboard: [measurement:client]
Blocks: 1276200
No longer blocks: 1252625
John, I just stumbled upon this during vocation. It could be worth adding it to the test cases!
Flags: needinfo?(jdorlus)
Assignee: nobody → alessio.placitelli
Points: --- → 3
Does this not seem related to 1292337?
Flags: needinfo?(jdorlus) → needinfo?(alessio.placitelli)
(In reply to John Dorlus [:Silne30] from comment #2)
> Does this not seem related to 1292337?

Yes, this is related, but different: this is happening also with automatic session restore, while 1292337 isn't.
Flags: needinfo?(alessio.placitelli)
Mike, is there any way to determine if the page loaded in a tab was loaded due to a session restoration or a direct user interaction?

I'm trying to count the number of URIs [1] directly opened by the user and I need to exclude the ones from the session restoration.

Thank you for your help!

[1] - https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/browser/modules/BrowserUsageTelemetry.jsm#69
Flags: needinfo?(mdeboer)
(In reply to Alessio Placitelli [:Dexter] from comment #4)
> Mike, is there any way to determine if the page loaded in a tab was loaded
> due to a session restoration or a direct user interaction?

Not off-hand with a pre-existing API call, no. It is possible to get it in the following way:

1) Monitor restored tabs and check which URI it was restore with:
```js
window.gBrowser.tabContainer.addEventListener("SSTabRestored", tab => {
  let browser = tab.linkedBrowser;
  // Store its URL in a WeakMap.
  this._restoredURLsMap.set(browser, browser.currentURI.spec);
});
```
2) Have the progress listener code in BrowserUsageTelemetry.jsm:69 check if the browser/ URL combo it gets is present in the map and ignore it if so.
3) IMPORTANT: The 'SSTabRestored' event may be dispatched later than `onLocationChange`, so please check that. If it's dispatched _before_ `onLocationChange`, we're all good. An alternative is to listen for 'SSTabRestoring' instead, but it's less reliable because a tab restore may get canceled.

I hope this helps!
Flags: needinfo?(mdeboer)
Attached patch bug1293222.patch (obsolete) — Splinter Review
@Mike, thanks for the tips from your previous comment. Would you mind giving a look at the way I'm filtering out the URIs from the restored sessions? Thanks!
Attachment #8780048 - Flags: feedback?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #5)
> 3) IMPORTANT: The 'SSTabRestored' event may be dispatched later than
> `onLocationChange`, so please check that. If it's dispatched _before_
> `onLocationChange`, we're all good. An alternative is to listen for
> 'SSTabRestoring' instead, but it's less reliable because a tab restore may
> get canceled.

What I'm seeing is a nice onLocationChange -> SSTabRestoring -> onLocationChange dance!

onLocationChange gets called right before SSTabRestoring (I guess when the URI is set), with a null nsIRequest.
Then SSTabRestoring happens. After that, a new onLocationChange is hit when the content is loaded (and there's a valid nsIRequest there).

As mentioned over IRC it seems like SSTabRestored is always happening after onLocationChange. As you pointed out, SSTabRestoring happens at a more convenient time, so I'm using it instead. Is user the only one that can cancel tab restoration?

> I hope this helps!

Thank was REALLY helpful, thanks!
Comment on attachment 8780048 [details] [diff] [review]
bug1293222.patch

f? -> r?
Attachment #8780048 - Flags: feedback?(mdeboer) → review?(mdeboer)
Comment on attachment 8780048 [details] [diff] [review]
bug1293222.patch

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

r=me with comments addressed and a green try run. Thanks!

::: browser/modules/BrowserUsageTelemetry.jsm
@@ +51,5 @@
>    _domainSet: new Set(),
> +  // A map to keep track of the URIs loaded from the restored tabs.
> +  _restoredURIsMap: new WeakMap(),
> +
> +  isURIAllowed(uri) {

I'd vote for `isValidURI`

@@ +236,5 @@
>      // Don't include URI and domain counts when in private mode.
>      if (PrivateBrowsingUtils.isWindowPrivate(win.defaultView)) {
>        return;
>      }
> +    win.defaultView.gBrowser.tabContainer.addEventListener(TAB_RESTORING_TOPIC, this);

Let's make this `removeEventListener`!
Attachment #8780048 - Flags: review?(mdeboer) → review+
Attached patch bug1293222.patchSplinter Review
Thanks!
Attachment #8780048 - Attachment is obsolete: true
Attachment #8780422 - Flags: review+
John, can we do some QA on this once it hits Nightly?
Flags: needinfo?(jdorlus)
https://hg.mozilla.org/integration/mozilla-inbound/rev/61adb42886fe0a010c1991fbb4078dd86f3dddf0
Bug 1293222 - browser.engagement.total_uri_count includes URIs from restored tabs. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/61adb42886fe
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
This has been verified to work!
Flags: needinfo?(jdorlus)
Status: RESOLVED → VERIFIED
Comment on attachment 8780422 [details] [diff] [review]
bug1293222.patch

Approval Request Comment
[Feature/regressing bug #]: 1271313
[User impact if declined]: None
[Describe test coverage new/current, TreeHerder]: This is covered bu browser_UsageTelemetry.js, which is already on Aurora. This patch also adds a new test case.
[Risks and why]: Minor risk, this is fixing a telemetry misbehaving Telemetry probe. We should really uplift this patch if possible: otherwise the values from the "browser.engagement.total_uri_count" probe used to measure user engagement will not be useful at all.
[String/UUID change made/needed]: None
Attachment #8780422 - Flags: approval-mozilla-aurora?
Flags: qe-verify+
Comment on attachment 8780422 [details] [diff] [review]
bug1293222.patch

Fixes a data quality issue in Telemetry, verified on Nightly, Aurora50+
Attachment #8780422 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I verified this issue on the latest Aurora(build ID: 20160915004005), the browser.engagement.total_uri_count  is counting accordingly.
You need to log in before you can comment on or make changes to this bug.