LSNG: Get rid of the main thread dependency during datastore preparation
Categories
(Core :: Storage: localStorage & sessionStorage, enhancement, P2)
Tracking
()
People
(Reporter: janv, Assigned: janv, NeedInfo)
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.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years 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.
Updated•2 years ago
|
Assignee | ||
Comment 2•2 years ago
|
||
Assignee | ||
Comment 3•2 years ago
|
||
Assignee | ||
Comment 4•2 years ago
|
||
Assignee | ||
Comment 5•2 years ago
|
||
Assignee | ||
Comment 6•2 years ago
|
||
Assignee | ||
Comment 7•2 years ago
|
||
Assignee | ||
Comment 8•2 years 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.
Updated•2 years ago
|
Comment 9•2 years ago
|
||
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•2 years ago
|
||
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•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 12•2 years ago
|
||
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.
Assignee | ||
Comment 13•2 years 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•2 years 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.
Updated•2 years ago
|
Assignee | ||
Comment 15•2 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #10)
Comment on attachment 9034133 [details] [diff] [review]
Part 3: Move observer registration to InitializeLocalStorageReview 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•2 years 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 browsingReview 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•2 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #12)
Comment on attachment 9034136 [details] [diff] [review]
Part 6: Release content parent using NS_ProxyReleaseReview 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•2 years ago
|
||
I'm going to land first 6 patches and then request reviews for the rest in phabricator.
Assignee | ||
Updated•2 years ago
|
Comment 19•2 years 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
Comment 20•2 years ago
|
||
bugherder |
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
Assignee | ||
Comment 21•2 years ago
|
||
Assignee | ||
Comment 22•2 years ago
|
||
Assignee | ||
Comment 23•2 years ago
|
||
Assignee | ||
Comment 24•2 years ago
|
||
Assignee | ||
Comment 25•2 years ago
|
||
Assignee | ||
Comment 26•2 years ago
|
||
Assignee | ||
Comment 27•2 years ago
|
||
Assignee | ||
Comment 28•2 years ago
|
||
Assignee | ||
Comment 29•2 years ago
|
||
The special case is now handled in GetSpecialBaseDomain in ContentPrincipal.cpp
Assignee | ||
Comment 30•2 years ago
|
||
Assignee | ||
Comment 31•2 years ago
|
||
Comment 32•2 years 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
Comment 33•2 years ago
|
||
bugherder |
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
Assignee | ||
Updated•2 years ago
|
Description
•