1467113 - The Side View sidebar cannot be resized to its maximum width after restarting the browser if Dark Mode add-on is installed

Status: VERIFIED FIXED

(WebExtensions :: Experiments, defect, P1)

Description

[Cristina Badescu [:cbadescu], Experiments QA]

Attachment: SV - sidebar cannot be resized.gif

[Affected versions]:
- Firefox Developer Edition 610b.11
- Nightly 62.0a1 (Build ID: 20180605141053)
- Side View 0.4.3

[Affected Platforms]:
- All Windows
- All Linux

[Prerequisites]:
- Have a clean Firefox profile with the latest Dark Mode v0.2.6 installed and active (https://addons.mozilla.org/en-US/firefox/addon/dark-mode-webextension/?src=search).

Steps to reproduce:
1. Open the browser with the profile from prerequisites.
2. Navigate to https://testpilot.firefox.com/experiments/side-view and install the Side View add-on.
3. Resize the sidebar to its maximum width.
4. Restart the browser and observe the behavior.

[Expected results]:
- The sidebar is displayed to its maximum width.

[Actual results]:
- The sidebar is displayed to a different width and it can not be resized to its maximum width even if you drag it.

[Regression]:
This issue is not reproducible on latest Firefox Release 60.0.1. Considering this, I've ran the Mozregression tool, in order to find a regression-window.
Here are the results:
Last good revision: 7f92f91fa85d78c434709ec162d4320c5ee686fd
First bad revision: fa02de71ebf73aea9f5d30b591aacd83b3440bd3
Pushlog: https://goo.gl/zkySkF

From the provided pushlog it seems that bug 1454820 introduced this issue.

[Notes]:
- This issue is reproducible on Arch Linux 4.12.
- This issue is not reproducible on Mac 10.13 and Ubuntu 16.04.
- The issue is also reported in the Side View GitHub repository, here: https://github.com/mozilla/side-view/issues/274
- Attached is a screen recording with this issue.

Comment 1

[Cristina Badescu [:cbadescu], Experiments QA]

Jonathan, can you please take a look at this issue and see what might have caused it?

Thanks.

Comment 2

[Jonathan Kingston [:jkt] he/him]

From what I can tell the addon is signed, I'm not really sure at the moment why this would be happening.

Comment 3

[Jonathan Kingston [:jkt] he/him]

I'm getting signedState == undefined in Extension.jsm on startup of the browser which would be causing this issue. However I'm struggling to work out why that would be the case right now.

Comment 4

[Jonathan Kingston [:jkt] he/him]

So on the install path the code checks the signedState in XPIInstall and sets the state on the addon. This permits the extension to use experiments.

However on reinstantiation of the addon on browser statup this step doesn't happen causing signedState to be undefined and the browser prevents the use of this feature.

Comment 5

[Jonathan Kingston [:jkt] he/him]

This will likely impact other studies that we are using, this makes it pretty critical to get fixed.

I'm struggling to get the correct flow of what is happening here, however it appears on browser startup the reading from the manifest in XPIInstall happens after the addon has started, the experiment code doesn't appear to be waiting for this.


:aswan can someone from your team look into this also, my knowledge of the code isn't as great as you or :kmag's thanks.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1467113: Save signedState in startup data.

Review commit: https://reviewboard.mozilla.org/r/249796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249796/

Comment 7

[Kris Maglione [:kmag]]

Jonathan, can you check that this patch fixes the problem?

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8983938 [details]
Bug 1467113: Save signedState in startup data.

https://reviewboard.mozilla.org/r/249796/#review255994

Comment 9

[Jonathan Kingston [:jkt] he/him]

Yeah I can't any longer see the effects I was before of undefined signedState and the sidebar addon mentioned on restart is working again.

Thanks for the prompt fix!

Comment 10

[Jonathan Kingston [:jkt] he/him]

Hey :kmag,
Is there anything blocking this from being landed now? This is blocking a study that we are doing at the moment also.

Thanks!

Comment 11

[Kris Maglione [:kmag]]

(In reply to Jonathan Kingston [:jkt] from comment #10)
> Is there anything blocking this from being landed now? This is blocking a
> study that we are doing at the moment also.

Nope. I was just waiting to confirm that this actually fixes the problem first.

Thanks

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9349d82f264efc86931bf60b2009af0e0d6d7cc
Bug 1467113: Save signedState in startup data. r=aswan

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/e9349d82f264

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8983938 [details]
Bug 1467113: Save signedState in startup data.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: This prevents us from shipping certain privileged add-ons, such as SHIELD studies, since their privileged portions stop working after a browser restart.
[Is this code covered by automated tests?]: Yes, though not this specific bug.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a pretty trivial change that adds another piece of metadata from the add-ons DB to the startup metadata cache.
[String changes made/needed]: None.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This was reported to be a regression from bug 1454820 (which landed in 61). Is ESR60 uplift needed?

Comment 16

[Kris Maglione [:kmag]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> This was reported to be a regression from bug 1454820 (which landed in 61).
> Is ESR60 uplift needed?

I'm honestly not sure about the details of the specific issue being reported. aswan told me to ignore it.

The underlying problem, though, has existed as long as we've supported special privileges for specially-signed extensions, which goes back to before 60. And I suspect we'll have occasion to ship such add-ons to ESR before the 60 cycle ends.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Thanks for the info.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8983938 [details]
Bug 1467113: Save signedState in startup data.

Small fix needed for us to ship privileged extensions. Approved for 61.0b13 and ESR 60.1.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Regretfully, this needs rebased patches for Beta/ESR60 :(

Comment 20

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8d528de4b44338c90c033032b05abc0b700d9802
https://hg.mozilla.org/releases/mozilla-esr60/rev/fa1c76b6cea15fc0cbdea4a5406034e1066d1476

Comment 21

[Cristina Badescu [:cbadescu], Experiments QA]

I have verified this issue and is no longer reproducible on latest Nightly build 62.0a1 (Build ID: 20180610220159), on Windows 10 x64 and Arch Linux 4.12.

I will verify this issue also on Firefox Beta 61.0b13 as soon as it will be released and Firefox ESR 60.1.

Comment 22

[Cristina Badescu [:cbadescu], Experiments QA]

I have verified this issue and is not reproducible on latest Firefox Beta 61.0b13 (Build ID: 20180611134439) and latest Firefox ESR 60.0.3 (Build ID: 20180608192033), which I've downloaded from here https://goo.gl/qL2RVi. Tested on Windows 10 x64 and Arch Linux 4.12.

Thanks!