1792028 - Apply the lazy defineLazyPreferenceGetter workaround to browser-sidebar.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Andrew McCreight [:mccr8]]

For some reason, this has flared up again (bug 1762665).

Looking at about a dozen of the failures, they are all happening while running toolkit/components/extensions/test/browser/browser_ext_themes_sidebars.js

Applying my logging patch from bug 1652531, while running that test I get:
ZZZ defineLazyPreferenceGetter with callback _positionStart sidebar.position_start

That appears to be the browser-sidebar.js lazy preference getter.

Hopefully I'll be able to land my cycle collector fix from bug 1543537 in the next few weeks, but for now I'll apply the lazy preference getter workaround for hopefully the last time.

I wasn't able to reproduce the crash locally. That test actually fails for me locally, but with this patch it continues to fail in the same way.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1792028 - Apply the lazy defineLazyPreferenceGetter workaround to browser-sidebar.js.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8e4168c161a
Apply the lazy defineLazyPreferenceGetter workaround to browser-sidebar.js. r=Gijs

Comment 3

[Cristian Tuns]

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

Comment 4

[Andrew McCreight [:mccr8]]

Bug 1762665 is happening on beta. It looks like the same test that my patch here should fix.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 9295793 [details]
Bug 1792028 - Apply the lazy defineLazyPreferenceGetter workaround to browser-sidebar.js.

Beta/Release Uplift Approval Request

• User impact if declined: Probably none. This fixes crashes that happen when you flip the pref sidebar.position_start, which is likely mostly a testing issue. The testing failure manifests as the intermittent bug 1762665.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This was on Nightly for a few days without issues (it has since been backed out because I fixed the crash another way) and it should only affect what happens when this pref is flipped, which presumably most users aren't doing.
• String changes made/needed: none
• Is Android affected?: No

Comment 6

[Andrew McCreight [:mccr8]]

it should only affect what happens when this pref is flipped

Also, to be clear here, the issue doesn't arise due to the pref having a specific value, but rather the actual flipping of the pref leaves the browser in a dangerous state for the current browsing session. The worst that would likely happen is that a user goes into about:config, flips the pref sidebar.position_start, crashes, then they restart the browser and things should be fine again (until they flip the pref again). We have no specific evidence of this happening in the wild, but in a non-debug build the crash would likely manifest as a weird null deref, so it would be hard to identify.

Comment 7

[Pascal Chevrel:pascalc]

Comment on attachment 9295793 [details]
Bug 1792028 - Apply the lazy defineLazyPreferenceGetter workaround to browser-sidebar.js.

We have our last 106 betas this week and there is no end-user impact, let's have it ride the train and the fix will be picked up soon with 107.0b1, thanks.