1644466 - Convert dom.storage.next_gen from a VarCache pref to something else

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task, P2)

Description

[Aria Beingessner [:Gankra]]

Forked off of Bug 1642344 so we could land the other changes now.

• Current proposed patch
• Current issue with patch
• Full try push to check if issue comes up

Comment 1

[Aria Beingessner [:Gankra]]

Attachment: Bug 1644466 - Convert dom.storage.next_gen into a once StaticPref. r?kriswright,janv

Comment 2

[Aria Beingessner [:Gankra]]

Hey janv, can we get a decision on this?

To refresh your memory:

• This is one of the last two uses of VarCache in the codebase, a problematic and buggy utility we want to remove
• Access to the pref is latched with a Mutex simulating the same basic functionality as AtStartup (afaict)
  • However you look at whether you're the parent or child process, the former using VarCache, the latter using GetBool
  • Based on the changelog this seems to be justified by making it usable on any thread, but if so this is a misunderstanding
    of the constraints of VarCache. It's not better at being accessed like that, it's just not going to stop you.
• You were concerned that AtStartup could cause a deadlock if it needs to be initialized for this use
  • I did several full try pushes and found no obvious issues
  • kriswright contends that AtStartup should be initialized already once you reach this point (I agree, but am less certain)

So there's three options in my mind:

• Land my patch to use AtStartup and see if anything breaks
• Change the VarCache use to a GetBool, on the premise that the VarCache use would be broken in the same ways GetBool would be anyway
• You propose a third, better, solution

Comment 3

[Jan Varga [:janv]]

(In reply to Alexis Beingessner [:Gankra] from comment #2)

• Change the VarCache use to a GetBool, on the premise that the VarCache use would be broken in the same ways GetBool would be anyway

Let me check this option ...

Comment 4

[Jan Varga [:janv]]

Ok, thanks for the detailed explanation. I previously somehow missed that the primary goal is to get rid of the VarCache use.

I think it would be safer to do:

-   name: dom.storage.next_gen
    type: RelaxedAtomicBool
    value: value: @IS_EARLY_BETA_OR_EARLIER@
    mirror: always

NextGenLocalStorageEnabled can then stay as it is except that GetCurrentNextGenPrefValue call would be replaced with:
StaticPrefs::dom_storage_next_gen()

Again, a green try push doesn't guarantee that everything is ok. We had big problems with deadlocks in the past.

Does this sound good to you ?

Comment 5

[Jan Varga [:janv]]

This would be even better:

-   name: dom.storage.next_gen
    type: RelaxedAtomicBool
    value: value: @IS_EARLY_BETA_OR_EARLIER@
    mirror: always
    do_not_use_directly: true

Comment 6

[Aria Beingessner [:Gankra]]

Attachment: Bug 1644466 - Convert dom.storage.next_gen into a StaticPref. r?kriswright,janv

The kinda hacky mutex latch is preserved to avoid concerns with AtStartup prefs having
potential to deadlock when used in this subsystem.

Comment 7

[Aria Beingessner [:Gankra]]

sorry for the delay, here's the suggested approach!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ae2d87b6e59
Convert dom.storage.next_gen into a StaticPref. r=janv

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0ae2d87b6e59