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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P2
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: janv, Assigned: janv, NeedInfo)

Tracking

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 wontfix, firefox67 fixed)

Details

Attachments

(17 attachments)

3.83 KB, patch
asuth
: review+
Details | Diff | Splinter Review
2.33 KB, patch
asuth
: review+
Details | Diff | Splinter Review
8.98 KB, patch
asuth
: review+
Details | Diff | Splinter Review
11.66 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.84 KB, patch
asuth
: review+
Details | Diff | Splinter Review
4.75 KB, patch
asuth
: review+
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Assignee

Description

6 months ago
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.
Assignee

Updated

6 months ago
Blocks: 1517090
Assignee

Updated

6 months ago
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Assignee

Comment 1

6 months ago
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.
Priority: -- → P2
Assignee

Comment 8

6 months ago
(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.
Attachment #9034131 - Flags: review?(bugmail) → review+
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 !
Attachment #9034132 - Flags: review?(bugmail) → review+
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.
Attachment #9034133 - Flags: review?(bugmail) → review+
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.
Attachment #9034134 - Flags: review?(bugmail) → review+
Attachment #9034135 - Flags: review?(bugmail) → review+
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.
Attachment #9034136 - Flags: review?(bugmail) → review+
Assignee

Comment 13

5 months ago

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.

Assignee

Comment 14

5 months ago

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.

Flags: needinfo?(bugmail)
Assignee

Comment 15

4 months ago

(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.

Assignee

Comment 16

4 months ago

(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.

Assignee

Comment 17

4 months ago

(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.

Assignee

Comment 18

4 months ago

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

Assignee

Updated

4 months ago
Keywords: leave-open

Comment 19

4 months ago
Pushed by jvarga@mozilla.com:
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
Assignee

Comment 29

4 months ago

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

Comment 32

3 months ago
Pushed by jvarga@mozilla.com:
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
Assignee

Updated

3 months ago
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.