Closed Bug 1517089 Opened 6 years ago Closed 6 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: janv, Assigned: janv)

References

Details

Attachments

(17 files)

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
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.
Blocks: 1517090
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
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
(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+

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.

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)

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

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

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

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

Keywords: leave-open
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

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

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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Flags: needinfo?(bugmail)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: