Closed Bug 1262880 Opened 4 years ago Closed 4 years ago

Remove add-on compatibility check from application update

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: rstrong, Assigned: rstrong)

References

Details

Attachments

(4 files, 4 obsolete files)

According to telemetry at most 0.5% of background update checks have incompatible add-ons.
The add-ons manager code is always called when there is an app version change for an update.
There have been add-ons that override the add-ons manager code and break application update even though both the add-ons manager and application update have been coded to prevent this in most circumstances.
There have been add-ons that are essentially abandoned which will require the user to opt-in to getting an application update instead of updating silently. For example, this happened with google toolbar years ago.
Supporting the add-on compatibility check requires UI and code that is complex and it is the one piece that will prevent us from never showing UI to update if we choose to require application updates.
Since the add-on compatibility check requires a user to opt-in to the update when an add-on is incompatible with the new version it is another step in the update process where the user might not update and hence not get security fixes as well as increase the number of users on older versions (e.g. update orphaning).
Stephen, would you have a problem with me landing this on oak?
Flags: needinfo?(spohl.mozilla.bugs)
Attached patch patch test changes rev1 (obsolete) — Splinter Review
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Comment on attachment 8745808 [details] [diff] [review]
patch toolkit changes rev1

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

not a review, just noticed this while looking this patch over:

::: toolkit/mozapps/update/content/updates.js
@@ +362,5 @@
>     * showUpdateError      nsIUpdate obj failed        either      complete  errors
>     * checkForUpdates      null          --            foreground  --        checking
>     * checkForUpdates      null          downloading   foreground  --        downloading
>     *
> +   * Note: the page returned (e.g. Result) for showUpdateAvaulable either

nit: s/showUpdateAvaulable/showUpdateAvailable/
Even though this removes a lot of code, it seems very self-contained. I'm ok if you'd like to land this on Oak. Fyi, SoftVision is actively testing bug 394984 on Oak.
Flags: needinfo?(spohl.mozilla.bugs)
Thanks for the drive by!
Attachment #8745808 - Attachment is obsolete: true
I'll get a browser peer to review the browser bits if you aren't a browser peer yet.
Comment on attachment 8745807 [details] [diff] [review]
patch browser changes rev1

Forgot about the pref changes. Will resubmit this patch.
Attachment #8745807 - Attachment is obsolete: true
Attachment #8745807 - Flags: review?(mhowell)
Comment on attachment 8745808 [details] [diff] [review]
patch toolkit changes rev1

Actually I'll submit a separate patch for the pref changes.
Attachment #8745808 - Attachment is obsolete: false
Attachment #8745808 - Flags: review?(mhowell)
Also, pushed to oak. Looks like there are problems with Mac xpcshell tests due to the work being done on oak.
Attachment #8745807 - Attachment is obsolete: false
Attachment #8745807 - Flags: review?(mhowell)
Attachment #8745808 - Attachment is obsolete: true
Attachment #8745808 - Flags: review?(mhowell)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #14)
> Also, pushed to oak. Looks like there are problems with Mac xpcshell tests
> due to the work being done on oak.

Thanks for the heads up! I missed this due to all the intermittents.
Comment on attachment 8745834 [details] [diff] [review]
patch toolkit changes rev2

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

For the record, this change was run by the firefox-dev mailing list [https://mail.mozilla.org/pipermail/firefox-dev/2016-April/004162.html] and did not draw any objections.
Attachment #8745834 - Flags: review?(mhowell) → review+
Attachment #8745916 - Flags: review?(mhowell) → review+
Attachment #8745807 - Flags: review?(mhowell) → review+
Comment on attachment 8745804 [details] [diff] [review]
patch test changes rev1

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

::: toolkit/mozapps/update/tests/chrome/utils.js
@@ +156,5 @@
>  
>  // Set to true to log additional information for debugging. To log additional
>  // information for an individual test set DEBUG_AUS_TEST to true in the test's
>  // onload function.
> +var DEBUG_AUS_TEST = true;

It looks like this should get flipped back to false.
Attachment #8745804 - Flags: review?(mhowell) → review-
Flipped DEBUG_AUS_TEST back to false
Attachment #8745804 - Attachment is obsolete: true
Attachment #8746117 - Flags: review?(mhowell)
Comment on attachment 8746117 [details] [diff] [review]
patch test changes rev2

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

Thanks!
Attachment #8746117 - Flags: review?(mhowell) → review+
Filed bug 1268176 for SeaMonkey
Filed bug 1268154 for Thunderbird
Just noticed that app.update.mode is sync'd so I removed that as well.
Attachment #8745916 - Attachment is obsolete: true
Attachment #8745916 - Flags: review?(felipc)
Attachment #8746355 - Flags: review?(felipc)
Attachment #8745807 - Flags: review?(felipc) → review+
Attachment #8746355 - Flags: review?(felipc) → review+
Blocks: 1269133
Depends on: 1269219
You need to log in before you can comment on or make changes to this bug.