Ship Baidu searchplugin code update via SAO to Firefox 45+

RESOLVED FIXED

Status

()

enhancement
--
major
RESOLVED FIXED
3 months ago
14 days ago

People

(Reporter: hectorz, Unassigned, NeedInfo)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr6068+ affected, firefox66blocking fixed)

Details

(Whiteboard: [go-faster-system-addon])

Attachments

(5 attachments)

Reporter

Description

3 months ago

As seen in bug 1540017, Baidu is asking us to replace the search code "monline_dg" in vanilla desktop Firefox with a new one "monline_7_dg".

In addition to the in-tree update, we'd like to use system addons to make the same change in previous versions of Firefox.

Reporter

Comment 1

3 months ago
Posted file GitHub Pull Request

Extensions based on google-code-correction was sent in this PR for review.

Comment on attachment 9055387 [details]
Unsigned baidu-code-update-2.66.xpi, for Fx 60+, webext with experiment

Please sign this system add-on update, thanks!

Flags: needinfo?(wezhou)

Comment on attachment 9055388 [details]
Unsigned baidu-code-update-1.66.xpi, for Fx 45-60, bootstrapped

Please sign this system add-on update, thanks!

Comment 6

3 months ago

Signed file attached. Please test.

Flags: needinfo?(wezhou)

Comment 7

3 months ago

The second signed file attached. Please test.

Rehan, could you please host both XPIs attache to this bug on the test channel? Thanks!

Flags: needinfo?(rdalal)

The 1.66 XPI is now on release-sysaddon 45.* through 59.*
The 2.66 XPI is now on release-sysaddon 60.* through 66.*

Flags: needinfo?(rdalal)

Hector, this is ready to ship and you mentioned to me that this already went through QA but it would be worth double-checking by:

  1. changing the Firefox update channel to release-sysaddon
  2. in about:config, set extensions.logging.enabled to true
  3. in the Browser Console, run: Cu.import("resource://gre/modules/AddonManager.jsm"); AddonManagerPrivate.backgroundUpdateCheck();

I'll run through this as well and ensure that we're serving up the right XPIs, but I thought you might better be able to tell if it is having the intended effect.

Rehan, per the "intent to ship" email that Hector sent this should be targeted to the zh-CN locale only, would you mind making this change?

Flags: needinfo?(rdalal)
Flags: needinfo?(bzhao)

It is possible --- but extremely difficult (and potentially buggy) --- to filter by locale in Balrog.

If we can avoid that it would be my strong preference.

At present we are recommending that addons be shipped to 100% of users and use Normandy to filter with a pref flip. This pattern has proved to be more flexible in terms of how we filter (including geo-targeting) and less error-prone.

@Hector would we be able to go down this route instead of filtering by locale in Balrog?

Flags: needinfo?(rdalal)
Reporter

Comment 12

3 months ago

(In reply to Robert Helmer [:rhelmer] from comment #10)

Hector, this is ready to ship and you mentioned to me that this already went through QA but it would be worth double-checking by:

  1. changing the Firefox update channel to release-sysaddon
  2. in about:config, set extensions.logging.enabled to true
  3. in the Browser Console, run: Cu.import("resource://gre/modules/AddonManager.jsm"); AddonManagerPrivate.backgroundUpdateCheck();

I'll run through this as well and ensure that we're serving up the right XPIs, but I thought you might better be able to tell if it is having the intended effect.

Thanks for the instructions. We might not be able to cover all versions during the weekend, but we’ll try this on Fx 66/60esr/52esr at the very least.

Flags: needinfo?(bzhao)
Reporter

Comment 13

3 months ago

(In reply to Rehan Dalal [:rehan, :rdalal] from comment #11)

It is possible --- but extremely difficult (and potentially buggy) --- to filter by locale in Balrog.

If we can avoid that it would be my strong preference.

At present we are recommending that addons be shipped to 100% of users and use Normandy to filter with a pref flip. This pattern has proved to be more flexible in terms of how we filter (including geo-targeting) and less error-prone.

@Hector would we be able to go down this route instead of filtering by locale in Balrog?

Current versions will return early for non-zh-CN locale builds since they don’t ship with a Baidu searchplugin. I’m actually okay with shipping them to 100% since system addons are not visible to ordinary users in about:addons . If you have concerns about the unnecessary additional observer, I can update the patch to include a locale check first thing Monday morning.

Reporter

Comment 14

3 months ago

(In reply to Hector Zhao [:hectorz] from comment #12)

Thanks for the instructions. We might not be able to cover all versions during the weekend, but we’ll try this on Fx 66/60esr/52esr at the very least.

And I’ve verified by following the instructions, Baidu searchplugins are updated as expected in the three Fx versions mentioned above.

If we're pushing this to non-zh-CN locales, should we have our regular QA team take a look?

(In reply to Julien Cristau [:jcristau] from comment #15)

If we're pushing this to non-zh-CN locales, should we have our regular QA team take a look?

That would be ideal to look out for any unintended side effects.


(In reply to Hector Zhao [:hectorz] from comment #13)

Current versions will return early for non-zh-CN locale builds since they don’t ship with a Baidu searchplugin. I’m actually okay with shipping them to 100% since system addons are not visible to ordinary users in about:addons . If you have concerns about the unnecessary additional observer, I can update the patch to include a locale check first thing Monday morning.

I am not too concerned with the additional observer, especially if QA is taking a look.

Mike, Reese, shipping this system add-on to all Firefox users means it will show up in about:support.

I want to check in with you about that, since many users will notice the change.
We should likely talk with the comms team to plan to explain this to users.

Flags: needinfo?(mconnor)
Flags: needinfo?(marissawood)

Hello,

I checked the following versions:

  • 45 ru (Windows 10 x64)
  • 53 fr (Windows 10 x64)
  • 59 pl (mac OS 10.13.6)
  • 60 ru (Windows 10 x64)
  • 63 es-ES (Windows 10 x64)
  • 66 ru (mac OS 10.13.6)

The search codes were all correct and the builds older than 60 showed 1.66 XPI and for builds newer than 60 showed 2.66 Xpi.

Reporter

Comment 19

3 months ago

(In reply to Rehan Dalal [:rehan, :rdalal] from comment #9)

The 1.66 XPI is now on release-sysaddon 45.* through 59.*
The 2.66 XPI is now on release-sysaddon 60.* through 66.*

I think this could be further limited to Fx 60.0 - 66.0.2, now that the in-tree patch was uplifted into Fx 66.0.3 (bug 1540017 comment 17)

Is this now ready to ship since we have QA signoff?

From Hector in email:

With the about:support concerns gone, I'd say the extensions are ready to be shipped.

The code is based on google-code-correction around Fx 57 and reviewed by :rhelmer;
The extensions were verified against each and every compatible Fx versions by Yanfang Liu, desktop QA in Beijing office and they work as expected;
The installation through release-sysaddon channel was verified by release QA :dcicas, who also verified the extensions have not inadvertently changed any other searchplugins.

Mike, who gives the final say here for implementation and what is your planned timing?
We only have a couple more weeks left in the 66 release cycle. I'd like to avoid this launch falling in the most active part of the 67 cycle (i.e. RC week).

Bridget, can you help clarify the plan for QA and for launch? Thanks!

Flags: needinfo?(bkaluzny)

Comment 23

2 months ago

Working on this - thanks for the ping Lizzard.

Flags: needinfo?(bkaluzny)

Rehan, I think this is ready to ship at this point. Is there any other signoff that you need?

Flags: needinfo?(rdalal)

Does this need to be confidential? Bug 1540017 is not.

Blocks: 1548578
No longer depends on: 1548578

Comment 26

2 months ago

@mconnor, please confirm that the QA performed is sufficient in testing potential neg impact of our regl. browser activity (non-CN)

Reporter

Comment 27

2 months ago

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #25)

Does this need to be confidential? Bug 1540017 is not.

Not really, opening this up.

Group: mozilla-employee-confidential

Based on what Hector's said in email, and what I've seen of the code, I'm confident the QA performed is sufficient (and risk is appropriately scoped to the Baidu engine only).

Flags: needinfo?(mconnor)

Shipped in bug 1548578.

Status: NEW → RESOLVED
Closed: 2 months ago
Flags: needinfo?(rdalal)
Flags: needinfo?(marissawood)
Resolution: --- → FIXED

From conversation with Hector, we are going to want to ship this to ESR users as well.
Note that uptake for ESR users is often low (as these installs are often in enterprise environments where system addons are not accepted). ESR usage for the population we want to get baidu's update to may vary, of course!

signed.baidu-code-update-1.66.xpi no longer installs on ESR 60.0.2 or earlier because of armagadd-on-2.0.

signed.baidu-code-update-2.66.xpi did not install, either.

OK. There is a bug/project underway to ship a hotfix to older versions for the cert2019 issues; let me find that bug and connect this as a dependency as, once that fix is out, we can ship this one.

Flags: needinfo?(lhenry)
Depends on: 1549604
Flags: needinfo?(lhenry)

That will only get to users from 52 and up. So, users on ESR versions before 52 may not be able to get the baidu update.

We will have to re-sign this SAO for 52-60. We can't depend on bug 1549604 because this SAO prevents from installing other SAOs (including a hotfix from bug 1549604).

Bridget can you help out here to move this along or find someone to be responsible for it?
I notice that it is still unassigned. Thanks!

Flags: needinfo?(bkaluzny)

Hector, are you working on this or should it be assigned to someone else?
Mike, Bridget asked me to pass this along to you as she is busy this week with onboarding.

Flags: needinfo?(mconnor)
Flags: needinfo?(bzhao)
Reporter

Comment 38

Last month

Sorry I only tested v2.66 on Fx 60esr and missed bug 1454820 which means it might not work on Fx 60 release.

Estimation of current status, of all release & esr channel zh-CN users:

  • ~78% users already updated to Fx 66.0.3+, with the fix in bug 1540017;
  • ~5% users on Fx 61+, should be covered by v2.66 shipped in bug 1548578;
  • <1% users on Fx 60, I think we should switch them to v1.66 due to the above-mentioned bug;
  • ~10.5% users on Fx 52esr or Fx 52.0 ~ 59.*, I'd like to ship v1.66, but this needs a new signature, see bug 1541316 comment 35 above;
  • ~2.5% users on Fx 45esr or Fx 45.0 ~ 51.*, if we can also ship v1.66 to them;
  • ~2% users on Fx 60esr, should be fixed by tomorrow's 60.7.0 update.

Actions we could take:

  • Change v2.66 to ship in Fx 61.0 ~ 66.0.2, instead of the current Fx 60.0 ~ 66.*;
  • Re-sign and ship v1.66 to Fx 52.0 ~ 60.* on both release and esr channels;
  • Extend v1.66 to Fx 45.0 ~ 60.* if possible.
Flags: needinfo?(bzhao)
Reporter

Comment 39

Last month

(In reply to Hector Zhao [:hectorz] from comment #38)

Actions we could take:

  • Change v2.66 to ship in Fx 61.0 ~ 66.0.2, instead of the current Fx 60.0 ~ 66.*;
  • Re-sign and ship v1.66 to Fx 52.0 ~ 60.* on both release and esr channels;
  • Extend v1.66 to Fx 45.0 ~ 60.* if possible.

It should be Fx 60.6.* to avoid installing this system addon for those already updated to Fx 60.7.0+

Why are you trying to ship to 45? is there really that much volume there?

rhelmer is the right person for this discussion since he had to go through this pain for the addon fix.

Flags: needinfo?(rhelmer)

(In reply to Mike Kaply [:mkaply] from comment #40)

Why are you trying to ship to 45? is there really that much volume there?

rhelmer is the right person for this discussion since he had to go through this pain for the addon fix.

45 is technically how far back we can go with system add-ons, but yes I'd like to see data justifying going back this far. The QA burden is quite high, and there is a cost to shipping add-ons that don't install (it keeps others in the set from installing, by design)

Flags: needinfo?(rhelmer)
Reporter

Comment 42

Last month

(In reply to Robert Helmer [:rhelmer] from comment #41)

(In reply to Mike Kaply [:mkaply] from comment #40)

Why are you trying to ship to 45? is there really that much volume there?

Fx 45 was originally chosen mostly because it's the first ESR compatible with google-code-correction.

......

45 is technically how far back we can go with system add-ons, but yes I'd like to see data justifying going back this far. The QA burden is quite high, and there is a cost to shipping add-ons that don't install (it keeps others in the set from installing, by design)

v1.66 was verified against Fx 45-60 by our desktop QA, Yanfang Liu. It should install fine with no code change but an updated signature.

(In reply to Hector Zhao [:hectorz] from comment #38)

Estimation of current status, of all release & esr channel zh-CN users:

"Estimation" because these ratios were calculated based on profiles with Fx China repack. I don't know if the search related telemetry dataset goes back that far, but I think I can get Fx versions based on profiles with vanilla zh-CN builds.

......

  • ~2.5% users on Fx 45esr or Fx 45.0 ~ 51.*, if we can also ship v1.66 to them;
    ......
  • Extend v1.66 to Fx 45.0 ~ 60.* if possible.

I understand shipping to Fx 45.0 ~ 51.* may not worth the effort, so I listed it separately below the Fx 52.0 ~ 60.6.* one.

Reporter

Comment 43

Last month

(In reply to Hector Zhao [:hectorz] from comment #42)

...... but I think I can get Fx versions based on profiles with vanilla zh-CN builds.

Please have a look at https://sql.telemetry.mozilla.org/queries/62888#161365 , if I did the SQL correctly, about 3.8% of vanilla zh-CN users are using Fx 45.0 ~ 51.*.

Blocks: 1553936

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1553936. This is what we are installing to complete this Baidu install. Discussed between mconnor, RituK and Rdalal.

Flags: needinfo?(bkaluzny)
You need to log in before you can comment on or make changes to this bug.