1772905 - Hide InstallTrigger global in Nightly and Early Beta

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, task, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

As a separate follow up for Bug 1754441 and Bug 1772901, this bugzilla issue is tracking fully hiding the InstallTrigger global by setting "extensions.InstallTrigger.enabled" to false.

Once "extensions.InstallTrigger.enabled" is set to false, the InstallTrigger global will not be enumerable anymore and the common user agent detection check typeof InstallTrigger !== "undefined" will not return true anymore.

This change in behavior is likely to introduce some webcompat issues to investigate as a side effect and so:

• it is tracked by a separate bugzilla issue from the one that is going to disable the InstallTrigger methods (Bug 1772901)
• Bug 1734432 has been added as a dependency (a WebCompat issue to assess potential interventions)
• it seems reasonable to consider setting it to false only on the nightly channel in the first place and file a separate issue for getting it to the other release channels.
• a draft redash query of the telemetry for the InstallTrigger implementation and InstallTrigger global deprecation use-counters has been created and linked to aid further planning of the next steps

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1772905 - Hide InstallTrigger global on Nightly and Early Beta builds.

Depends on D148542

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Dennis,
I described in comment 0 some proposed next steps, but all of that is also to be considered subjected to changes based on webcompat team perspective and what we will agree on.

The attached patch is currently disabling the InstallTrigger global only in Nightly builds but keeping it enabled on all other channels (and I'm going to add the webcompat review group as a blocking reviewer on that patch, we don't plan to land that patch until webcompat team confirms that would be a reasonable first step).

I'm starting by needinfo-ing you because this is also related to Bug 1734432 that you filed, but this is meant to be a needinfo for the webcompat team and so feel free to redirect to someone else in the webcompat team if appropriate.

In comment 0 I also attached a draft redash query for the two InstallTrigger deprecation use-counters, let me know how that query looks like from the webcompat team perspective, I'm pretty sure that the webcompat team may have more experience than me in querying this data (this was the first time I was looking into querying the "use counters" telemetry) and so any insight / fix / proposed tweak from the webcompat team perspective that would help to make that data as useful as possible to aid planning the next steps would be greatly appreciated.

I'm also happy to schedule a meeting to discuss about this further if that can be helpful.

(I'm also linking as a see also Bug 1772901, which is the one tracking disabling the InstallTrigger methods).

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Added needinfo flag related to comment 2.

Comment 4

[Dennis Schubert [:denschub]]

Hi Luca, thanks a lot for the work and the summary here.

Your proposal is very reasonable. While I can't help with the telemetry dashboard specifically as my knowledge about the use counters is a bit more shaky than I want it to be, we know that InstallTrigger is used a lot, we just don't really know why. So having more accurate numbers won't help here.

We've done a bit of research into this previously, and I spent a bit of time looking into public code to see how devs are using InstallTrigger. While I did see some things that might break, nothing immediately jumped out to me as critical and I didn't see anything in a big OSS library.

I agree that having this in a pre-Release channel for a while before relasing is a good idea, as that would also allow us to intervene if needed. However, I would prefer this to be in EARLY_BETA_OR_EARLIER instead of just Nightly, as our Beta population is significantly larger. I'd rather have this spend some stabilization cycles in Beta where this gets more exposure. I'll leave it up to you if you want to have this Nightly-only first for a while before exposing to Beta, I'm fine either way.

If we do discover breakage, we can easily ship a WebCompat intervention that sets window.InstallTrigger to something not-undefined. I just tested a patch to make sure this works, and it does as expected. So even if regressions pop up, we should be able to handle them in an intervention, without having to toggle the pref again.

I'll double-check with the team if I missed anything critical before r+'ing your patch, but this all looks like it's hopefully not a big deal.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Dennis Schubert [:denschub] from comment #4)

...
I agree that having this in a pre-Release channel for a while before relasing is a good idea, as that would also allow us to intervene if needed. However, I would prefer this to be in EARLY_BETA_OR_EARLIER instead of just Nightly, as our Beta population is significantly larger. I'd rather have this spend some stabilization cycles in Beta where this gets more exposure. I'll leave it up to you if you want to have this Nightly-only first for a while before exposing to Beta, I'm fine either way.

Yours is a very good point, and so I'm updating the attached patch to disable the InstallTrigger global on nightly and early beta (and I'll also updating this bugzilla issue subject message to reflect that), thanks a lot for bringing it up!

If we do discover breakage, we can easily ship a WebCompat intervention that sets window.InstallTrigger to something not-undefined. I just tested a patch to make sure this works, and it does as expected. So even if regressions pop up, we should be able to handle them in an intervention, without having to toggle the pref again.

I'll double-check with the team if I missed anything critical before r+'ing your patch, but this all looks like it's hopefully not a big deal.

That sounds great to me too, thanks a lot also for looking so quickly into this!

As an additional side note, once the last try push confirms that we didn't introduce any new tests that would fail with InstallTrigger disabled (we adapted quite an amount of tests across the tree as part of Bug 1754441 in preparation for these additional tests) both Bug 1772901 path and this one will be pushed to autoland and will both get in Nightly 103, and so:

• During Nightly 103 and Early Beta 103 => InstallTrigger global is going to be completely hidden (per pref flipped by the patch attached to this issue).
• During Late Beta 103, DevEdition 103 and Release 103 => InstallTrigger methods will be all hidden, the InstallTrigger global to be still enumerable and set to null (per pref flipped by the patch attached to Bug 1772901).

As a potential "worst case scenario":

• If a subset of the websites using InstallTrigger global for UserAgent detection are doing a different kind of check that isn't still true with InstallTrigger set to null (most of them should be using the common typeof InstallTrigger !== "undefined" and should still work just fine once Bug 1772901 gets to release), we may need to a Webcompat intervention, but it would basically the same one mentioned in comment 4 and so it seems that we will be prepared also to the less likely "worst case scenario"

(In addition to that we could also flip these two prefs back to true through AddonManager's RemoteSettings, that was also part of the preparation work from Bug 1754441 as an additional "very last resort", because unlike the WebCompat intervention through AMRemoteSettings we can only disable/enable InstallTrigger deprecation prefs globally and not on a per-origin basis).

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/202d54fa588b
Hide InstallTrigger global on Nightly and Early Beta builds. r=webcompat-reviewers,denschub,willdurand

Comment 7

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for for causing failures on browser_dom_nodes_highlight.js

Backout link : https://hg.mozilla.org/integration/autoland/rev/f4824bdc3f8425ae14532c0b527bc54fe020c736

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=MpsFrtQbTHubtiEymGjYHw.0&resultStatus=usercancel%2Cretry%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=202d54fa588bc71ff4bf8117e6682d2ca758b729

Link to failure log: https://treeherder.mozilla.org/logviewer?job_id=380934352&repo=autoland&lineNumber=2530

Failure line:
TEST-UNEXPECTED-FAIL | devtools/client/dom/test/browser_dom_nodes_highlight.js | The correct node was highlighted - Got "h2", expected "h1"

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1772905 - Update DevTools DOM panel tests that fails if InstallTrigger is not listed anymore as a window global. r=nchevobbe!

These two DevTools DOM panel tests are expecting the elements being inspected to be at a certain row index
which depends (likely due to alphabetic order) that InstallTrigger is listed as a global in the window
being inspected (it doesn't matter what the value is, the failure is triggered because when InstallTrigger
is completely hidden the elements expected by the tests are shift by one row position).

And so these two tests would be currently permafailing when "extensions.InstallTrigger.enabled" is set to false
(but they are completely uneffected when "extensions.InstallTriggerImpl.enabled" is set to false, because the
InstallTrigger global is still defined, just set to null instead of providing the API defined by the
InstallTriggerImpl interface).

This patch replaces getRowByIndex calls that are affected by hiding InstallTrigger global with corresponding
calls to getRowByLabel, which is uneffected by the number of row changing based on the value set on
"extensions.InstallTrigger.enabled" (which as part of Bug 1772905 will be different between nightly and early beta
build and the other channels).

Depends on D148542

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Clearing needinfo:

• I've attached a patch (comment 8) to address the unrelated failures triggered by hiding InstallTrigger global.
• I'll push the patch again on autoland once the newly attached patch is fully reviewed and ready to land.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Push to try:

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=0702714317a759eb284dcb58fb8683b778ef914d

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/786400ce2e54
Update DevTools DOM panel tests that fails if InstallTrigger is not listed anymore as a window global. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/4f8d91c60287
Hide InstallTrigger global on Nightly and Early Beta builds. r=webcompat-reviewers,denschub,willdurand

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/786400ce2e54
https://hg.mozilla.org/mozilla-central/rev/4f8d91c60287