Closed Bug 1754441 Opened 4 months ago Closed 2 months ago

Deprecate InstallTrigger

Categories

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

Firefox 100
task

Tracking

()

VERIFIED FIXED
100 Branch
Tracking Status
firefox100 --- verified

People

(Reporter: rpl, Assigned: rpl, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [addons-jira])

Attachments

(9 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
3.54 KB, text/plain
chutten
: data-review+
Details
48 bytes, text/x-phabricator-request
Details | Review

+++ This bug was initially created as a clone of Bug #1442035 +++

Short summary of the InstallTrigger use cases

Deprecation plan

Instead of removing it right away (which seems to be what Bug 1442035 was aiming for), we are considering to start by:

  • logging an explicit deprecation warning (through the Deprecated Web IDL extended attribute)

  • make sure we are going to be collect telemetry (if I'm not mistaken using this WebIDL attribute should also automatically collect telemetry for it as a side-effect) and that we can derive from it how much of its usage may be due to just the "UA detection" vs. the "install addons" use case

  • remove from it old cruft (e.g. methods that are not doing anymore what they were originally intended for) and some corner use cases that were never supposed to be supported

  • add an about:config pref (and/or RemoteSetting) to hide all methods but leaves the InstallTrigger objectas defined (and so fully remove the "install addons" support without overlapping with the "UA detection" issues)

  • add a separate about:config pref (and/or RemoteSetting) to hide the InstallTrigger itself (in preparation to fully remove it if our telemetry makes us confident enough that the "UA detection" use case isn't going to be a problem anymore)

Assignee: nobody → lgreco
Severity: normal → --
Status: NEW → ASSIGNED
Priority: -- → P2
Severity: -- → N/A
Keywords: leave-open
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/f72c3caea300
Explicitly check for supported web schemes. r=zombie

Data review request for the two new prefs added to list of prefs collected in the telemetryEnvironment userPrefs property (related to the phabricator revision: https://phabricator.services.mozilla.com/D138796).

Attachment #9264831 - Flags: data-review?(chutten)

Comment on attachment 9264831 [details]
request_bug1754441_telemetryEnvironment_InstallTrigger_prefs.md

PRELIMINARY NOTES:
An individual must be named as the one responsible for non-expiring collections, so I've volunteered you, :rpl : D

If this isn't correct, please supply a different individual who will be responsible for this collection.

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Luca Greco is responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.


Result: datareview+

Flags: needinfo?(lgreco)
Attachment #9264831 - Flags: data-review?(chutten) → data-review+

PRELIMINARY NOTES:
An individual must be named as the one responsible for non-expiring collections, so I've volunteered you, :rpl : D

If this isn't correct, please supply a different individual who will be responsible for this collection.

Hi Chris,
thanks for needinfo-ing me along with the data review, I shouldn't have forgot that, my apologies.

I was just chatting about this with Shane and we were wondering if there can be more than one individual (e.g. to have a fallback, "just in case").
is that allowed / possible?

In any case, I'm ok to be the individual responsible for this collection.

Flags: needinfo?(lgreco)

The more the merrier! There needs to be at least one specific individual listed in the review response. (Or, in this case, below the review response). More than one is groovy.

(In reply to Chris H-C :chutten from comment #13)

The more the merrier! There needs to be at least one specific individual listed in the review response. (Or, in this case, below the review response). More than one is groovy.

Hi Mike,
given that you may be the one monitoring this collection more directly, do you want to be listed as individual responsible for this collection along with me?

Flags: needinfo?(mconnor)

Push to try for the entire stack of patches (after having pushed to phabricator one last round of updates and rebased the entire stack on top of today's mozilla-central tip):

Hi Thomas,
as I briefly described in comment 0 InstallTrigger deprecation does also have potential webcompat side-effects, and so I wanted to make sure to provide to the webcompat team an "heads-up" and some details about it up front.

This set of patches is now all signed-off and so it should land soon enough in Firefox 100, but it is not going to hide the InstallTrigger global or its methods just yet.

On the contrary once we will land these patches, Firefox will start to emit deprecation warnings when the InstallTrigger global is accessed or its InstallTrigger.install method called (there are two separate deprecation warning messages, and so there will be also two separate deprecation counters in telemetry too).

There are some more details in planning documents, I'm about to share those with you as well, to make sure you can have a good picture of the next steps of the deprecation plans and we can discuss further about it.

Flags: needinfo?(twisniewski)
Blocks: 1734432

There is probably a need for an intent to deprecate on dev-platform.

In terms of webcompat, installTrigger is currently used as a way to detect Firefox.

https://github.com/webcompat/web-bugs/issues/98497#issuecomment-1025404718
https://github.com/webcompat/web-bugs/issues/82928#issuecomment-933145083
https://github.com/webcompat/web-bugs/issues/89116#issuecomment-936671603
https://github.com/webcompat/web-bugs/issues/75078#issuecomment-854385928
https://github.com/webcompat/web-bugs/issues/68738#issuecomment-812339501
https://github.com/webcompat/web-bugs/issues/46313#issuecomment-565581586
https://github.com/webcompat/web-bugs/issues/21650#issuecomment-460529140

The pattern is often a variation of:

var isFirefox = typeof InstallTrigger !== "undefined";

In some cases removing installTrigger will solve the webcompat issues but in some other cases it will probably create breakages by sending Firefox in codepaths that were not planned by the web developers.

So expect breakages in nightly once this is released. And we could have big surprises.

We probably need telemetry for window.installTrigger
https://github.com/search?l=JavaScript&q=installTrigger+firefox&type=Code

(In reply to Karl Dubost💡 :karlcow from comment #19)

We probably need telemetry for window.installTrigger
https://github.com/search?l=JavaScript&q=installTrigger+firefox&type=Code

I definitely agree, that should actually be already covered by D138302, the Deprecated WebIDL extended attribute does have an implicit side-effect of collecting data in an auto-generated telemetry histogram, in particular:

  • accessing the InstallTrigger global collects telemetry in the following two histograms:
    • USE_COUNTER2_DEPRECATED_InstallTriggerDeprecated_PAGE (on the parent process) / USE_COUNTER2_DEPRECATED_InstallTriggerDeprecated_DOCUMENT (on the content process)
  • accessing the InstallTrigger.install methods collects telemetry in the following two histograms:
    • USE_COUNTER2_DEPRECATED_InstallTriggerInstallDeprecated_PAGE (on the parent process) / USE_COUNTER2_DEPRECATED_InstallTriggerInstallDeprecated_DOCUMENT (on the content process)

(clearing Thomas needinfo, he already handled it by forwarding the request to the rest of the webcompat Team, thanks a lot Thomas!).

Flags: needinfo?(twisniewski)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/74d84f0f64ac
Add an explicit deprecation warning on InstallTrigger. r=mixedpuppy,emilio
https://hg.mozilla.org/integration/autoland/rev/5e55e93b5336
Add prefs to control InstallTrigger/InstallTriggerImpl visibility. r=mixedpuppy,webidl,smaug
https://hg.mozilla.org/integration/autoland/rev/095054f6f110
Explicit set prefs to enable InstallTrigger on all tests that depend on its availability. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/ddf908d08916
Add RemoteSettings to control InstallTrigger/InterTriggerImpl visibility. r=robwu,leplatrem
https://hg.mozilla.org/integration/autoland/rev/cc415c5944ed
List extensions.InstallTrigger/InstallTriggerImpl.enabled prefs in about:support. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/4b572b213e40
Add extensions.InstallTrigger/InstallTriggerImpl.enabled prefs to telemetry environment. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/dc10a99012c0
Add in-tree docs for AMRemoteSettings. r=leplatrem,robwu
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Keywords: leave-open
Resolution: --- → FIXED
Version: unspecified → Firefox 100
Target Milestone: --- → 100 Branch
Blocks: 1761775

Verified the fix on the latest Nightly (100.0a1/20220328213646) under Windows 10 x64, Ubuntu 16.04 LTS and macOS 11.3.1.

InstallTrigger and InstallTrigger.install are currently controlled by new about:config prefs - “extensions.InstallTrigger.enabled” and “extensions.InstallTriggerImpl.enabled” set by default to “true”.

Flipping “extensions.InstallTrigger.enabled” to false will hide InstallTrigger global completely and flipping “extensions.InstallTriggerImpl.enabled” to false will make InstallTrigger global to be still visible but set to null.

Corresponding deprecation warning messages are logged to console when accessing the deprecated features:

Telemetry is properly collected when accessing the deprecated features:

  • Accessing the InstallTrigger global collects telemetry in the following two histograms:
    USE_COUNTER2_DEPRECATED_InstallTriggerDeprecated_PAGE
    USE_COUNTER2_DEPRECATED_InstallTriggerDeprecated_DOCUMENT
  • Accessing the InstallTrigger.install methods collects telemetry in the following two histograms:
    USE_COUNTER2_DEPRECATED_InstallTriggerInstallDeprecated_PAGE
    USE_COUNTER2_DEPRECATED_InstallTriggerInstallDeprecated_DOCUMENT

The new prefs are properly shown in both about:support and about:telemetry once changed from their default values

The prefs can also be properly set/reverted via the RemoteSettings “main/addons-manager-settings” collection with the same expected results as when manually setting the prefs.

Status: RESOLVED → VERIFIED

(In reply to Karl Dubost💡 :karlcow from comment #25)

Webcompat Regression https://github.com/webcompat/web-bugs/issues/101685

I'm moving the link to the webcompat regression issue to Bug 1759737, the install page from www.dashlane.com is hitting the error raised on InstallTrigger.install called without user activation (user input requirement.

See Also: → 1762241

For UA detection, is there an alternative to InstallTrigger (not based on user agent)?

I know that in theory one should not use browser detection at all, but there are subtle differences in browsers and especially when using JS rendering frameworks (like React), you sometimes need to adapt component behavior specific to browsers...

Just as an example: Autofill/Autocomplete is not working at all in FF with react standard input elements unless you do some workarounds (and will probably not be fixed)
https://github.com/facebook/react/issues/18986

You need to log in before you can comment on or make changes to this bug.