1343731 - Remove the sync IPC during DataStorage initialization

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[(no longer active)]

See this profile: <https://perfht.ml/2mwkgJr>.  The sync IPC is taking 247ms under an Element.setAttribute() call from JS.  :(

This one is my fault, and I have a patch to fix it.  The try server is rather unhappy with it at the moment.  I will submit it when try is green.

Comment 1

[(no longer active)]

Attachment: Remove the sync IPC during DataStorage initialization

Instead of initializing DataStorage objects on demand in the content process, we initialize them at content process startup by getting the parent to send down the information about the existing DataStorages at child process startup.  After that point, the dynamic change notifications added in bug 1215723 will take care of keeping the information in sync.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8844888 [details] [diff] [review]
Remove the sync IPC during DataStorage initialization

Review of attachment 8844888 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have a sense of how much space this cache will take. It's a bit worrisome that we don't seem to have memory reporters for this stuff. Is it expected to be small?

::: dom/ipc/ContentParent.cpp
@@ +2190,5 @@
>  
> +  // Ensure the SSS is initialized before we try to use its storage.
> +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");
> +
> +  InfallibleTArray<nsString> storageFiles;

I generally use nsTArray over InfallibleTArray, but I'm not sure which one is preferred. Do you know?

::: security/manager/ssl/DataStorage.cpp
@@ +87,5 @@
>  }
>  
> +// static
> +void
> +DataStorage::GetAllFileNames(InfallibleTArray<nsString>* aItems)

I think a reference would be nicer here.

::: security/manager/ssl/DataStorage.h
@@ +102,5 @@
>  
>    // Initializes the DataStorage. Must be called before using.
>    // aDataWillPersist returns whether or not data can be persistently saved.
> +  nsresult Init(/*out*/bool& aDataWillPersist,
> +                const InfallibleTArray<mozilla::dom::DataStorageItem>* aItems = nullptr);

Please comment what aItems is for.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Oh, I guess I was only supposed to look at the sync-messages.ini change, which is obviously good. Sorry :-).

Comment 4

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

Comment on attachment 8844888 [details] [diff] [review]
Remove the sync IPC during DataStorage initialization

Review of attachment 8844888 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM given there's no concerns with the first two comments. In terms of the size, each data storage is limited to ~1MB, so I don't think we'll be using too much memory.

::: dom/ipc/ContentParent.cpp
@@ +2188,5 @@
>      }
>    }
>  
> +  // Ensure the SSS is initialized before we try to use its storage.
> +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");

In situations where there will be a profile directory, is it guaranteed to be available at this point?

@@ +2191,5 @@
> +  // Ensure the SSS is initialized before we try to use its storage.
> +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");
> +
> +  InfallibleTArray<nsString> storageFiles;
> +  DataStorage::GetAllFileNames(&storageFiles);

Note that this may or may not include the alternate services file (https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/netwerk/protocol/http/AlternateServices.cpp#949), but I think that's okay since that appears to be parent-process-only (although a necko peer would know for sure).

::: security/manager/ssl/DataStorage.cpp
@@ +87,5 @@
>  }
>  
> +// static
> +void
> +DataStorage::GetAllFileNames(InfallibleTArray<nsString>* aItems)

I agree.

@@ +100,5 @@
> +}
> +
> +// static
> +void
> +DataStorage::SetCachedStorageEntries(const InfallibleTArray<mozilla::dom::DataStorageEntry>& aEntries)

nit: long line

@@ +101,5 @@
> +
> +// static
> +void
> +DataStorage::SetCachedStorageEntries(const InfallibleTArray<mozilla::dom::DataStorageEntry>& aEntries)
> +{

Let's assert that this is the child process.

::: security/manager/ssl/DataStorage.h
@@ +102,5 @@
>  
>    // Initializes the DataStorage. Must be called before using.
>    // aDataWillPersist returns whether or not data can be persistently saved.
> +  nsresult Init(/*out*/bool& aDataWillPersist,
> +                const InfallibleTArray<mozilla::dom::DataStorageItem>* aItems = nullptr);

... and what the expectations are: null for parent process, non-null for child process

@@ +125,5 @@
>    // Read all of the data items.
>    void GetAll(InfallibleTArray<DataStorageItem>* aItems);
>  
> +  // Set the cached copy of our DataStorage entries in the content process.
> +  static void SetCachedStorageEntries(const InfallibleTArray<mozilla::dom::DataStorageEntry>& aEntries);

nit: it would be nice to keep lines to <80 characters (awkward breaking like putting 'const InfallibleTArray<mozilla::dom::DataStorageEntry>& aEntries);' by itself on the next line is fine)

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

1MB seems like a lot to me. Hopefully realistic usage is lower. We absolutely should have a memory reporter for this data. I don't want to hold up this bug, but getting a memory reporter should be a priority. The second biggest bucket in our content process memory overhead is heap-unclassified. We should be driving that down, rather than up (as this bug does).

Comment 6

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #2)
> I don't have a sense of how much space this cache will take. It's a bit
> worrisome that we don't seem to have memory reporters for this stuff. Is it
> expected to be small?

I honestly didn't know the data in question here is this big.  :-(  I will add a memory reporter for this, I have caused all of these troubles in the first place by requiring this data in the content process...  Filed bug 1346486.

> ::: dom/ipc/ContentParent.cpp
> @@ +2190,5 @@
> >  
> > +  // Ensure the SSS is initialized before we try to use its storage.
> > +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");
> > +
> > +  InfallibleTArray<nsString> storageFiles;
> 
> I generally use nsTArray over InfallibleTArray, but I'm not sure which one
> is preferred. Do you know?

I think I may have originally used the explicit type becuase the code was used to send the array over an IPC channel or something, and then I forgot to change the type?  Usually when dealing with IPC channels I try to be explicit about fallibility especially with arrays where the length is transmitted over IPC but here obviously it doesn't matter.

(In reply to Bill McCloskey (:billm) from comment #3)
> Oh, I guess I was only supposed to look at the sync-messages.ini change,
> which is obviously good. Sorry :-).

No I actually asked for a normal review, and thanks for taking the time.  I'm glad you caught the memory usage issue.  :-)

(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> ::: dom/ipc/ContentParent.cpp
> @@ +2188,5 @@
> >      }
> >    }
> >  
> > +  // Ensure the SSS is initialized before we try to use its storage.
> > +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");
> 
> In situations where there will be a profile directory, is it guaranteed to
> be available at this point?

99.99% sure, yes!  We read all sorts of things from the profile during the startup (which is why our startup time can be so slow! ;)

> @@ +2191,5 @@
> > +  // Ensure the SSS is initialized before we try to use its storage.
> > +  nsCOMPtr<nsISiteSecurityService> sss = do_GetService("@mozilla.org/ssservice;1");
> > +
> > +  InfallibleTArray<nsString> storageFiles;
> > +  DataStorage::GetAllFileNames(&storageFiles);
> 
> Note that this may or may not include the alternate services file
> (https://dxr.mozilla.org/mozilla-central/rev/
> 58753259bfeb3b818eac7870871b0aae1f8de64a/netwerk/protocol/http/
> AlternateServices.cpp#949), but I think that's okay since that appears to be
> parent-process-only (although a necko peer would know for sure).

Yes, that's parent only, I already checked it.

Will address the rest of the comments when landing.  David, thanks especially for asking for more assertions.  I remember how much I liked the existing assertions in this code when I did the previous change, I got sloppy this time!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf33ec027cea
Remove the sync IPC during DataStorage initialization; r=keeler,billm

Comment 8

[Carsten Book [:Tomcat]]

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