1751306 - Make RuntimeSettings.Pref thread safe

Status: NEW

(GeckoView :: General, defect, P2)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Break-out from bug 1750896 – I was in a good position to fix this portion of the bug so I took a stab at it.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Note: an alternative to making this class thread safe is to confine usage of the Pref class to specific threads only. If we decide to go that route in bug 1750896, this patch isn't needed (though maybe we'd want to use the ConcurrentTestRule elsewhere).

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1751306 - add ConcurrentTestRule. r=agi

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1751306 - add thread safety test for RuntimeSettings.Pref. r=agi

This test is @Ignore in this commit because it introduces an intentional test
failure that uncovers the concurrency bug.

Depends on D136546

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1751306 - synchronize access to RuntimeSettings.Pref. r=agi

The class is technically not thread safe yet because a reference to the
unsynchronized class is leaked from the constructor: this will be addressed in
the next commit for clarity.

Depends on D136547

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1751306 - add factory method for RuntimeSettings.Pref. r=agi

This completes making RuntimeSettings.Pref thread safe.

As far as I can tell in the current implementation, the values passed to Pref
(of type T) are all thread safe because they are immutable so there are no
additional concurrency bugs created by those values.

Depends on D136548

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Fixing concurrency bugs isn't my main focus so I'm concerned changing the concurrency test to fit existing GV patterns would take too long for me. We could land the code without the test or someone can pick up where I left off on the test.

I'm not actively working on moving this forward so unassigning.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D136548	Bug 1751306 - synchronize access to RuntimeSettings.Pref. r=agi	mcomella	agi: Inactive

:mcomella, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Without checking the code, I assume ths bug is still relevant but I'm too far away from the code to continue to work on this. My patches are probably still relevant too but they've probably bitrotted a bit.