1246018 - Users with add-ons sometimes fail to have AddonsShouldHaveBlockedE10s

Status: RESOLVED INVALID

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Felipe asked me to look into this since KaiRo found a crash where AddonsShouldHaveBlockedE10s=0 but the user had add-ons installed.

I went through 500 crash reports from the 'experiment' group. Of those, there were 3 crashes where add-ons were installed and AddonsShouldHaveBlockedE10s=0.

96aa82f0-2da6-4db0-a894-c69232160129
0c6c3670-2ca9-40b6-9ef2-22c4d2160129
c2d4e0db-9dda-4248-9d1d-71e822160129

I did the same thing for the 'control' group and found 4 crashes:

f77b7965-fb6a-4c4e-abf5-ac0002160130
5e8c8f3c-132f-4789-88ef-1a2592160129
05439907-da7a-499d-9ba7-0d0542160131
180535e0-6cc5-472e-89d4-282aa2160130

Felipe has an idea how this could happen. I'll let him describe it.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

It looks like that if there are no changes in the add-ons database, there's nothing that triggers the add-on blocking code from updating the pref that is read on startup. It's easy to reproduce locally by reusing the profile in 44 and then 45.  It's unclear if the regular update path would trigger it, and early enough in startup, for that to be updated. 

Here are some STR. It doesn't actually need to use different versions:

- Using Fx45 in a fresh profile, install an add-on
- Clear the pref extensions.e10sBlockedByAddons, and set browser.tabs.remote.autostart to true
- Restart Firefox

FF will restart with e10s active even though there's an add-on installed. And this new run won't update the pref either (that is, it's not a one session thing). Only a new add-on install will.

This scenario looks artificial, but it's what will happen when an existing user (with add-ons installed) updates to a new version that suddenly has e10s turned on.

Luckily this code will be running on 45 while e10s is off, so users will have a chance to have the pref set before getting e10s on 46. But is it possible that a user will go through the entire length of 45 without having the add-ons db update? Could we add some code to guarantee it runs in advance of their upgrade?

Comment 2

[Dave Townsend [:mossop]]

Did you actually verify that starting Firefox 46 on a profile from 45 doesn't update the pref? We should go through the startup code to set the pref in that case.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Thanks for catching that, Dave. I had in fact only tested the same-version scenario as I described on comment 1, but didn't test the version change. I did the STRs with 44 going to 45 and it worked properly.

This problem can occur with our experiment, though, since there's no change of version there when we enable e10s through the experiment. Is there anything I can include in the experiment code to trigger this check when we toggle the e10s pref?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

So I did some more testing and the installation of the experiment does trigger the update of the pref..

My next guess was that this could be due to a lack of a savePrefFile(null) call, but that would be strange since the e10s pref, which is toggled after the add-on is installed and running, got saved..

So I don't know what else could cause the pref to be incorrect..

Comment 5

[Gabor Krizsanits (INACTIVE)]

We still have browser.tabs.remote.force-enable that overwrites everything... other than that crash during an add-on install process can leave an inconsistent state behind and could cause it as well. I'll try to look into those profiles too tomorrow.

Comment 6

[Dave Townsend [:mossop]]

(In reply to :Felipe Gomes (needinfo me!) from comment #4)
> So I did some more testing and the installation of the experiment does
> trigger the update of the pref..
> 
> My next guess was that this could be due to a lack of a savePrefFile(null)
> call, but that would be strange since the e10s pref, which is toggled after
> the add-on is installed and running, got saved..
> 
> So I don't know what else could cause the pref to be incorrect..

Is it possible that these users had the e10s pref enabled before installing the experiment? How are you verifying from the crash report that the pref is set?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

In the first crash report for example, https://crash-stats.mozilla.com/report/index/96aa82f0-2da6-4db0-a894-c69232160129

In the Metadata tab, there's
ActiveExperiment 	[e-mail]
ActiveExperimentBranch 	experiment

This means that the experiment installed successfully, because it's the bootstrap code that sets the ActiveExperimentBranch string.  And users part of the "experiment" group are the ones who were at default settings, i.e., no e10s before.  You can also tell that they are using e10s by looking at the DOMIpcEnabled flag in the metadata.

(As for AddonsShouldHaveBlockedE10s annotation, I can't see them either. They must be whitelisted in the crash-stats UI. I filed bug 1245981 for that)