Closed Bug 1562942 Opened 3 months ago Closed 3 months ago

Disable LSNG in 68

Categories

(Core :: DOM: Web Storage, task, P1, major)

task

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr68 68+ fixed
firefox68 blocking fixed

People

(Reporter: janv, Assigned: janv)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

We've decided to disable LSNG in 68 due to bug 1560697. Any fixes/changes at this point (one week till release) would be too risky.

See Also: → 1560697

Comment on attachment 9075448 [details]
Bug 1562942 - Disable LSNG in 68; r=asuth

sorry, wrong bug.

Attachment #9075448 - Flags: approval-mozilla-release?
Attachment #9075448 - Flags: approval-mozilla-esr68?
Attachment #9075448 - Flags: approval-mozilla-release?
Attachment #9075448 - Flags: approval-mozilla-esr68?

Jan, care to request release and esr68 approval on this patch?

Flags: needinfo?(jvarga)

Comment on attachment 9075448 [details]
Bug 1562942 - Disable LSNG in 68; r=asuth

Beta/Release Uplift Approval Request

  • User impact if declined: If LSNG is enabled, quota manager storage initialization time is increased by ~30%
    We don't want to ship LSNG until this regression is fixed.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • 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 patch just disables LSNG, so the old implementation of LocalStorage will be used in FF 68. We fully support downgrading of LocalStorage data, so if some users already tried a release build, they should be safe. We actually disabled LSNG in previous beta cycle and there were no problems reported after the switch.
  • String changes made/needed: None

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: If LSNG is enabled, quota manager storage initialization time is increased by ~30%
    We don't want to ship LSNG until this regression is fixed.
  • User impact if declined: See above.
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch just disables LSNG, so the old implementation of LocalStorage will be used in ESR 68. We fully support downgrading of LocalStorage data, so if some users already tried an ESR build, they should be safe. We actually disabled LSNG in previous beta cycle and there were no problems reported after the switch.
  • String or UUID changes made by this patch: None
Flags: needinfo?(jvarga)
Attachment #9075448 - Flags: approval-mozilla-release?
Attachment #9075448 - Flags: approval-mozilla-esr68?

Comment on attachment 9075448 [details]
Bug 1562942 - Disable LSNG in 68; r=asuth

flip a pref to disable a feature we're not ready to ship; approved for 68 rc2.

Attachment #9075448 - Flags: approval-mozilla-release?
Attachment #9075448 - Flags: approval-mozilla-release+
Attachment #9075448 - Flags: approval-mozilla-esr68?
Attachment #9075448 - Flags: approval-mozilla-esr68+
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

This breaks dom/localstorage/test/unit/test_uri_encoding_edge_cases.js, see e.g. https://taskcluster-artifacts.net/bAR_Ti8dSWePpUazKti-Ig/0/public/logs/live_backing.log

Flags: needinfo?(jvarga)

I'm testing a patch to fix that.

Flags: needinfo?(jvarga)

Actually should I have left this bug open, and have the patch land on 69 at least until 1560697 is fixed? Or is the plan to uplift a fix for 1560697 to beta69?

Flags: needinfo?(jvarga)

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

Actually should I have left this bug open, and have the patch land on 69 at least until 1560697 is fixed? Or is the plan to uplift a fix for 1560697 to beta69?

I'm working on bug 1563023. We think it can be uplifted to beta69.

Flags: needinfo?(jvarga)
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c223dcee1ce8
Followup patch, fix a failing test; r=asuth

If I'm understanding, we want to disable LSNG on 69 as well now? Please nominate this patch for Beta approval if that's the case.

Flags: needinfo?(jvarga)
Flags: needinfo?(jvarga)
See Also: → 1570644
You need to log in before you can comment on or make changes to this bug.