1458720 - fix RunOnAllContentParents (DataStorage) to be runnable from any thread

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to :Ehsan Akhgari from bug 1450630 comment #14)
...
> (In reply to David Keeler [:keeler] (use needinfo) from bug 1450630 comment #13)
> > I'm confused - DataStorage was written with the intent that it be safe to
> > access from multiple threads (the documentation even says so at the top of
> > DataStorage.h). From what I can tell from the code, the static
> > DataStorage::Get* meta-APIs that operate on the different types of
> > DataStorage we have are main-thread-only, but are the Get/Set/Remove APIs
> > that operate on actual data restricted as well? If so, we've strayed from
> > the original design requirements (e.g. nsSiteSecurityService is
> > fundamentally broken if we can't access its backing DataStorage off the main
> > thread).
> 
> Hmm...  I'm not sure what the API requirements here are, I'm basing my
> judgement on what the code is actually doing.  All calls to
> RunOnAllContentParents() require to be on the main thread (in general, our
> IPC interfaces are not thread safe so if the caller requires thread safety
> it is its own responsibility to take care of that at layers higher than IPC.)
> 
> This code is currently called from Put(), Remove() and Clear() so per above
> I think you're suggesting that this has all been broken since bug 1215723
> (Firefox 45)?  If yes, then the right fix isn't to fix the Necko consumers,
> but to rework the fix to bug 1215723 to make it not have a main-thread
> dependency.  I think doing so is actually relatively easy, all that should
> be required is to rewrite RunOnAllContentParents() to make it dispatch a
> runnable to the main thread if we aren't there currently, and make sure its
> callers capture their closure with a copy rather than by reference.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1458720 - make RunOnAllContentParents runnable from any thread

In bug 1215723 RunOnAllContentParents was added to the DataStorage
implementation so we could make more security state information available in
child processes. It uses IPC APIs, which in general are not thread-safe. We need
to make sure that these APIs are only accessed on the main thread, which means
we have to copy any necessary data, create a runnable, and send it to the main
thread to do the actual work. Note that the IPC APIs are async, so this dispatch
can be async as well.

Review commit: https://reviewboard.mozilla.org/r/241270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241270/

Comment 2

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8972737 [details]
bug 1458720 - make RunOnAllContentParents runnable from any thread

https://reviewboard.mozilla.org/r/241270/#review247146

Looks fine to me. r+ w/ nits addressed.

::: security/manager/ssl/DataStorage.cpp:717
(Diff revision 1)
>  template <class Functor>
>  static
>  void
>  RunOnAllContentParents(Functor func)
>  {
>    if (!XRE_IsParentProcess()) {

Perhaps we should assert `!NS_IsMainThread()` if possible?

::: security/manager/ssl/DataStorage.cpp:730
(Diff revision 1)
> -  ContentParent::GetAll(parents);
> +    ContentParent::GetAll(parents);
> -  for (auto& parent: parents) {
> +    for (auto& parent: parents) {
> -    func(parent);
> +      func(parent);
> -  }
> +    }
> +  });
> +  NS_DispatchToMainThread(r);

Probably should wrap this with `MOZ_ALWAYS_SUCCEEDS(...)` since this is a void function.

Comment 4

[(no longer active)]

Comment on attachment 8972737 [details]
bug 1458720 - make RunOnAllContentParents runnable from any thread

https://reviewboard.mozilla.org/r/241270/#review247146

> Perhaps we should assert `!NS_IsMainThread()` if possible?

I don't think we can assert that here, since we can indeed be off main thread, right?

Comment 5

[(no longer active)]

Comment on attachment 8972737 [details]
bug 1458720 - make RunOnAllContentParents runnable from any thread

https://reviewboard.mozilla.org/r/241270/#review247672

Thanks, looks great!

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8972737 [details]
bug 1458720 - make RunOnAllContentParents runnable from any thread

https://reviewboard.mozilla.org/r/241270/#review247146

Thanks for the reviews!

> I don't think we can assert that here, since we can indeed be off main thread, right?

Right - we can be either on or off the main thread in these functions.

> Probably should wrap this with `MOZ_ALWAYS_SUCCEEDS(...)` since this is a void function.

Ok - sounds good.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8972737 [details]
bug 1458720 - make RunOnAllContentParents runnable from any thread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241270/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38e60f49b316
make RunOnAllContentParents runnable from any thread r=Ehsan,jcj

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/38e60f49b316

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you're comfortable doing so.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8972737 [details]
bug 1458720 - make RunOnAllContentParents runnable from any thread

Approval Request Comment
[Feature/Bug causing the regression]: bug 1215723
[User impact if declined]: potential source of crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: localized, minimal changes, has tests
[String changes made/needed]: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8972737 [details]
bug 1458720 - make RunOnAllContentParents runnable from any thread

Avoid some possible crashes, approved for 61.0b5.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/47f7391d2bb0