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

VERIFIED FIXED in Firefox -esr60

Status

P1
major
VERIFIED FIXED
10 months ago
9 months ago

People

(Reporter: cbadescu, Assigned: kmag)

Tracking

({regression})

unspecified
mozilla62
x86_64
All
regression
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr6061+ verified, firefox60 wontfix, firefox61+ verified, firefox62+ verified)

Details

Attachments

(2 attachments)

[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.
Jonathan, can you please take a look at this issue and see what might have caused it?

Thanks.
Flags: needinfo?(jkt)
From what I can tell the addon is signed, I'm not really sure at the moment why this would be happening.
Flags: needinfo?(kmaglione+bmo)
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.
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.
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.
Severity: normal → major
Flags: needinfo?(aswan)
Priority: -- → P1
Target Milestone: --- → mozilla61
Comment hidden (mozreview-request)
(Assignee)

Comment 7

10 months ago
Jonathan, can you check that this patch fixes the problem?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)

Comment 8

10 months ago
mozreview-review
Comment on attachment 8983938 [details]
Bug 1467113: Save signedState in startup data.

https://reviewboard.mozilla.org/r/249796/#review255994
Attachment #8983938 - Flags: review?(aswan) → review+
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!
Flags: needinfo?(jkt)
Attachment #8983938 - Flags: feedback+
Assignee: nobody → kmaglione+bmo
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!
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 11

10 months ago
(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
Flags: needinfo?(kmaglione+bmo)

Comment 13

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e9349d82f264
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: mozilla61 → mozilla62
(Assignee)

Comment 14

10 months ago
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.
Attachment #8983938 - Flags: approval-mozilla-esr60?
Attachment #8983938 - Flags: approval-mozilla-beta?
This was reported to be a regression from bug 1454820 (which landed in 61). Is ESR60 uplift needed?
Blocks: 1454820
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 16

10 months ago
(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.
Flags: needinfo?(kmaglione+bmo)
Thanks for the info.
status-firefox60: unaffected → wontfix
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → affected
tracking-firefox61: --- → +
tracking-firefox62: --- → +
tracking-firefox-esr60: --- → 61+
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.
Attachment #8983938 - Flags: approval-mozilla-esr60?
Attachment #8983938 - Flags: approval-mozilla-esr60+
Attachment #8983938 - Flags: approval-mozilla-beta?
Attachment #8983938 - Flags: approval-mozilla-beta+
Regretfully, this needs rebased patches for Beta/ESR60 :(
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 20

10 months ago
uplift
https://hg.mozilla.org/releases/mozilla-beta/rev/8d528de4b44338c90c033032b05abc0b700d9802
https://hg.mozilla.org/releases/mozilla-esr60/rev/fa1c76b6cea15fc0cbdea4a5406034e1066d1476
status-firefox61: affected → fixed
status-firefox-esr60: affected → fixed
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

10 months ago
Blocks: 1456485
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.
status-firefox62: fixed → verified
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!
status-firefox61: fixed → verified
status-firefox-esr60: fixed → verified
Status: RESOLVED → VERIFIED

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.