1517089 - LSNG: Get rid of the main thread dependency during datastore preparation

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, enhancement, P2)

Description

[Jan Varga [:janv]]

I've been investigating ways how to solve issues like bug 1516136. We could do some hacks similar to how we handle CPOWs, but I think we should just get rid of the main thread dependency during datastore preparation.

A change like this involves multiple areas like:
- registering callbacks for prefs
- registering observers
- getting profile directory path
- services that must be initialized on the main thread first time
- using PBackground protocol instead of dispatching a runnable to PBackground thread directly
- getting quota manager info from PrincipalInfo directly

I think all of these are now doable, especially given there's the MozURL which can be used to verify that a spec can be parsed off the main thread.

Comment 1

[Jan Varga [:janv]]

I'm also investigating using dom/clients for PrincipalInfo verification off the main thread. It seems that ClientManagerService::MatchAll is very close to what we need.

Comment 2

[Jan Varga [:janv]]

Attachment: Part 1: Introduce InitializeLocalStorage and call it in nsLayoutStatics::Initialize

Comment 3

[Jan Varga [:janv]]

Attachment: Part 2: Move pref initialization to InitializeLocalStorage

Comment 4

[Jan Varga [:janv]]

Attachment: Part 3: Move observer registration to InitializeLocalStorage

Comment 5

[Jan Varga [:janv]]

Attachment: Part 4: Send an async IPC message instead of dispatching a runnable to the PBackground thread when clearing private browsing

Comment 6

[Jan Varga [:janv]]

Attachment: Part 5: Move storage service initialization to InitializeLocalStorage

Comment 7

[Jan Varga [:janv]]

Attachment: Part 6: Release content parent using NS_ProxyRelease

Comment 8

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #0)
> - getting quota manager info from PrincipalInfo directly

I'm still working on this, will send a patch for it later.

We also need to get rid of the main thread in QuotaManager::GetOrCreate, I'll file a separate bug for that.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9034132 [details] [diff] [review]
Part 2: Move pref initialization to InitializeLocalStorage

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

Long-term, let's not forget about https://searchfox.org/mozilla-central/source/modules/libpref/StaticPrefs.h and https://searchfox.org/mozilla-central/source/modules/libpref/init/StaticPrefList.h !

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9034133 [details] [diff] [review]
Part 3: Move observer registration to InitializeLocalStorage

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

Reviewer note: sInstance goes away in the next patch.

::: dom/localstorage/ActorsParent.cpp
@@ +7919,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  MOZ_ALWAYS_SUCCEEDS(obs->RemoveObserver(this, kPrivateBrowsingObserverTopic));
> +  MOZ_ALWAYS_SUCCEEDS(obs->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID));

Please add a comment that calls out that the instance will have died after this removal call and that it's not safe to do anything after this point.  It's not always super obvious later on when touching code like this.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9034134 [details] [diff] [review]
Part 4: Send an async IPC message instead of dispatching a runnable to the PBackground thread when clearing private browsing

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

::: dom/localstorage/ActorsParent.h
@@ +70,5 @@
>  
>  bool DeallocPBackgroundLSSimpleRequestParent(
>      PBackgroundLSSimpleRequestParent* aActor);
>  
> +bool RecvClearPrivateBrowsing();

(And this name will want to be updated when the ipdl method name gets namespaced too.)

::: ipc/glue/BackgroundParentImpl.cpp
@@ +397,5 @@
> +mozilla::ipc::IPCResult BackgroundParentImpl::RecvClearPrivateBrowsing() {
> +  AssertIsInMainProcess();
> +  AssertIsOnBackgroundThread();
> +
> +  if (!mozilla::dom::RecvClearPrivateBrowsing()) {

Arguably we should be doing a safety check that `!BackgroundParent::IsOtherProcessActor(this)` so that only the parent can tell us this.

::: ipc/glue/PBackground.ipdl
@@ +150,5 @@
>     * see `PBackgroundLSRequest.ipdl` for details.)
>     */
>    async PBackgroundLSSimpleRequest(LSSimpleRequestParams params);
>  
> +  async ClearPrivateBrowsing();

Please namespace this method somehow, as it's very generic.  Maybe something like LSClearPrivateBrowsing().

Also please add a comment here that explains what's going on.  Something like:

Asynchronously propagates the "last-pb-context-exited" observer notification to LocalStorage NextGen implementation so it can clear retained private-browsing in-memory Datastores.  Using a (same-process) IPC message avoids the need for the main-thread nsIObserver to have a reference to the PBackground thread and directly dispatch a runnable to it.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9034136 [details] [diff] [review]
Part 6: Release content parent using NS_ProxyRelease

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

r=asuth with the proposed change made.

::: dom/localstorage/ActorsParent.cpp
@@ +3131,2 @@
>        RefPtr<ContentParent> contentParent =
>            BackgroundParent::GetContentParent(aBackgroundActor);

You can just call `BackgroundParent::GetChildID(aBackgroundActor)` and avoid getting the ContentParent reference.  If you look at where it gets it at https://searchfox.org/mozilla-central/rev/ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/ipc/glue/BackgroundImpl.cpp#744 it's calling it on the same actor->mContent that GetContentParent() returns but without the refcount shenanigans.

Comment 13

[Jan Varga [:janv]]

Here's my current patch queue:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6795a5b3c1344f0344186d5d40411bbbffe7d73

In the last patch, I was finally able to get rid of the main thread in LSRequestBase/LSSimpleRequestBase.

However, there's still one spot in localstorage/ActorsParent.cpp which uses main thread.
It's the creation of old style originKey for which we use GenerateOriginKey which currently needs nsIPrincipal.
If we want to use MozURL instead of nsIPrincipal, we need to enhance MozURL a little bit.

Also, quota manager uses main thread in a couple of places, most of them are "easily" fixable with current infrastructure, but there's one spot where we need to get origin for a spec without the option to fall back to principal.originNoSuffix() for special schemes (for example when we are restoring the .metadata-v2 file).
The problem is that some schemes are currently not supported by MozURL's Origin method and since we don't have principal.originNoSuffix(), we have to teach MozURL to support them.

:asuth, I think I need your initial feedback for the current patch queue, especially regarding how to handle special schemes.
One option is to enhance MozURL::Origin to support them.

Comment 14

[Jan Varga [:janv]]

Actually, I think the current patch queue is enough for this bug, I'll file separate bugs for main thread usage described in comment 13.

Comment 15

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #10)

Comment on attachment 9034133 [details] [diff] [review]
Part 3: Move observer registration to InitializeLocalStorage

Review of attachment 9034133 [details] [diff] [review]:

Reviewer note: sInstance goes away in the next patch.

::: dom/localstorage/ActorsParent.cpp
@@ +7919,5 @@

• return NS_ERROR_FAILURE;
• }
• MOZ_ALWAYS_SUCCEEDS(obs->RemoveObserver(this, kPrivateBrowsingObserverTopic));
• MOZ_ALWAYS_SUCCEEDS(obs->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID));

Please add a comment that calls out that the instance will have died after
this removal call and that it's not safe to do anything after this point.
It's not always super obvious later on when touching code like this.

Ok, I added:
In general, the instance will have died after the latter removal call, so
it's not safe to do anything after that point.
However, Shutdown is currently called from Observe which is called by the
Observer Service which holds a strong reference to the observer while the
Observe method is being called.

Comment 16

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #11)

Comment on attachment 9034134 [details] [diff] [review]
Part 4: Send an async IPC message instead of dispatching a runnable to the
PBackground thread when clearing private browsing

Review of attachment 9034134 [details] [diff] [review]:

::: dom/localstorage/ActorsParent.h
@@ +70,5 @@

bool DeallocPBackgroundLSSimpleRequestParent(
PBackgroundLSSimpleRequestParent* aActor);

+bool RecvClearPrivateBrowsing();

(And this name will want to be updated when the ipdl method name gets
namespaced too.)

Ok.

::: ipc/glue/BackgroundParentImpl.cpp
@@ +397,5 @@

+mozilla::ipc::IPCResult BackgroundParentImpl::RecvClearPrivateBrowsing() {

• AssertIsInMainProcess();
• AssertIsOnBackgroundThread();
• if (!mozilla::dom::RecvClearPrivateBrowsing()) {

Arguably we should be doing a safety check that
!BackgroundParent::IsOtherProcessActor(this) so that only the parent can
tell us this.

Oh, good catch, fixed.

::: ipc/glue/PBackground.ipdl
@@ +150,5 @@

* see `PBackgroundLSRequest.ipdl` for details.)
*/

async PBackgroundLSSimpleRequest(LSSimpleRequestParams params);

• async ClearPrivateBrowsing();

Please namespace this method somehow, as it's very generic. Maybe something
like LSClearPrivateBrowsing().

Ok, changed.

Also please add a comment here that explains what's going on. Something
like:

Asynchronously propagates the "last-pb-context-exited" observer notification
to LocalStorage NextGen implementation so it can clear retained
private-browsing in-memory Datastores. Using a (same-process) IPC message
avoids the need for the main-thread nsIObserver to have a reference to the
PBackground thread and directly dispatch a runnable to it.

Added.

Comment 17

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #12)

Comment on attachment 9034136 [details] [diff] [review]
Part 6: Release content parent using NS_ProxyRelease

Review of attachment 9034136 [details] [diff] [review]:

r=asuth with the proposed change made.

::: dom/localstorage/ActorsParent.cpp
@@ +3131,2 @@

   RefPtr<ContentParent> contentParent =
       BackgroundParent::GetContentParent(aBackgroundActor);

You can just call BackgroundParent::GetChildID(aBackgroundActor) and avoid
getting the ContentParent reference. If you look at where it gets it at
https://searchfox.org/mozilla-central/rev/
ecf61f8f3904549f5d65a8a511dbd7ea4fd1a51d/ipc/glue/BackgroundImpl.cpp#744
it's calling it on the same actor->mContent that GetContentParent() returns
but without the refcount shenanigans.

Cool, that's even better.

Comment 18

[Jan Varga [:janv]]

I'm going to land first 6 patches and then request reviews for the rest in phabricator.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b51d5398126
Part 1: Introduce InitializeLocalStorage and call it in nsLayoutStatics::Initialize; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/09ac1f780e56
Part 2: Move pref initialization to InitializeLocalStorage; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/534dc9cee9ae
Part 3: Move observer registration to InitializeLocalStorage; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c48efbd5486
Part 4: Send an async IPC message instead of dispatching a runnable to the PBackground thread when clearing private browsing; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9e487f5d16
Part 5: Move storage service initialization to InitializeLocalStorage; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d6d7646f18
Part 6: Use BackgroundParent::GetChildID (which doesn't need releasing content parent on the main thread) for getting content parent id; r=asuth

Comment 20

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/9b51d5398126
https://hg.mozilla.org/mozilla-central/rev/09ac1f780e56
https://hg.mozilla.org/mozilla-central/rev/534dc9cee9ae
https://hg.mozilla.org/mozilla-central/rev/2c48efbd5486
https://hg.mozilla.org/mozilla-central/rev/6e9e487f5d16
https://hg.mozilla.org/mozilla-central/rev/08d6d7646f18

Comment 21

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 7: Pass originKey through IPC and get privateBrowsingId directly from ContentPrincipalInfo; r=asuth

Comment 22

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 8: Add GetBaseDomainFromSchemeHost to ThirdPartyUtil and make ThirdPartyUtil ref counting thread safe; r=asuth

Comment 23

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 9: Add baseDomain to ContentPrincipalInfo; r=asuth

Comment 24

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 10: Implement QuotaManager::IsPrincipalInfoValid; r=asuth

Comment 25

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 11: Verify principalInfo before creating any parent actors; r=asuth

Comment 26

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 12: Implement ClientManagerService::HasWindow; r=asuth

Comment 27

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 13: Use separate IPC params and response for datastore preloading; r=asuth

Comment 28

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 14: Use ClientManagerService for client validation; r=asuth

Comment 29

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 15: Remove a hack for JetPack and DevTools fron QuotaManager:GetInfoFromPrincipal; r=asuth

The special case is now handled in GetSpecialBaseDomain in ContentPrincipal.cpp

Comment 30

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 16: Implement QuotaManager::GetInfoFromValidatedPrincipalInfo; r=asuth

Comment 31

[Jan Varga [:janv]]

Attachment: Bug 1517089 - Part 17: Avoid main thread during LSRequestBase/LSSimpleRequestBase processing; r=asuth

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b99eeb0467
Part 7: Pass originKey through IPC and get privateBrowsingId directly from ContentPrincipalInfo; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dab164824a8
Part 8: Add GetBaseDomainFromSchemeHost to ThirdPartyUtil and make ThirdPartyUtil ref counting thread safe; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5adb364e4ddf
Part 9: Add baseDomain to ContentPrincipalInfo; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1169146b0409
Part 10: Implement QuotaManager::IsPrincipalInfoValid; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b51bc2a466c
Part 11: Verify principalInfo before creating any parent actors; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/a87a6189d20b
Part 12: Implement ClientManagerService::HasWindow; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc30924bd97b
Part 13: Use separate IPC params and response for datastore preloading; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d430843fe847
Part 14: Use ClientManagerService for client validation; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8acfb77b2f1
Part 15: Remove a hack for JetPack and DevTools fron QuotaManager:GetInfoFromPrincipal; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/696bd0721718
Part 16: Implement QuotaManager::GetInfoFromValidatedPrincipalInfo; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb1b8fcb03cb
Part 17: Avoid main thread during LSRequestBase/LSSimpleRequestBase processing; r=asuth

Comment 33

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b5b99eeb0467
https://hg.mozilla.org/mozilla-central/rev/6dab164824a8
https://hg.mozilla.org/mozilla-central/rev/5adb364e4ddf
https://hg.mozilla.org/mozilla-central/rev/1169146b0409
https://hg.mozilla.org/mozilla-central/rev/5b51bc2a466c
https://hg.mozilla.org/mozilla-central/rev/a87a6189d20b
https://hg.mozilla.org/mozilla-central/rev/dc30924bd97b
https://hg.mozilla.org/mozilla-central/rev/d430843fe847
https://hg.mozilla.org/mozilla-central/rev/e8acfb77b2f1
https://hg.mozilla.org/mozilla-central/rev/696bd0721718
https://hg.mozilla.org/mozilla-central/rev/eb1b8fcb03cb

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

(the comment 13 discussion about URIs moved elsewhere, clearing needinfo)