Replied with comments inline :-) On Tue, Oct 6, 2020 at 11:15 PM Geoff Lankow <ge...@thunderbird.net> wrote:
> I've borrowed this code from Firefox > < > https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#1832-1851> > > and that works perfectly (well, after I fix a few other things that > aren't relevant here). I can put that in a wrapper function to decide > which remote type to use, do the switch if necessary, and load the page. > If I switch to a non-remote browser then load a message, that works too. > The code path you ended up copying from tabbrowser is one of our older codepaths, which we are still keeping around for portability, but we may be removing it in the future. The C++ async API you're trying to use is the more modern one, and should generally be used instead. Unlike the JS `changeRemoteness` API, it is async and doesn't block the parent process while waiting for a new content process to spawn. It will be fine for now, but these APIs are unfortunately in a decent amount of flux with the active work on Fission, so don't be too surprised if they end up changing a bit. > This works, the browser changes remoteness. Now I need a docShell for > the next piece of the process, and this is where I'm stuck. > > I'm not sure I've got the arguments to ChangeRemoteness right. I made up > the second as a non-zero value is required, I think I understand the > third but browsingContext is discarded regardless, and I don't know > anything about the fourth. > In general, the ChangeRemoteness method is intended to be used by DocumentLoadListener, which will trigger a process switch in response to a navigation. This is the most common form of process switching in Firefox, so it's generally what the API is designed around. I unfortunately don't know a ton about how Thunderbird works internally, so I don't know whether loading a message occurs as a document navigation, but I'm going to continue assuming it doesn't and you need to do this manually. (if you can get away with using the same codepath as Firefox by doing a navigation, your life might be easier, though) The reason why you're seeing a discarded BrowsingContext is because the third argument (aReplaceBrowsingContext) only forces that the BC is replaced, and can't guarantee it isn't. For certain loads we always replace the BrowsingContext even if that parameter is false. Those cases are enumerated here: https://searchfox.org/mozilla-central/rev/165e8d8f80c02605c2f3e89c5de462abfce91c32/dom/base/nsFrameLoaderOwner.cpp#60-85. Specifically, we always replace the BrowsingContext when switching from a content process to the parent process (as content should ideally never have a reference to a document loaded in the parent process), as well as if the `fission.preserve_browsing_contexts` pref is `false`, though we're likely going to remove that pref in the near future. Fortunately, as you're in the parent process, you don't need to worry about the BrowsingContext not being preserved too much. You can get the new BrowsingContext after the remoteness change in one of two ways: // The browser frame element will be the same, so you can fetch the new BrowsingContext directly from it. RefPtr<nsFrameLoaderOwner> flo = do_QueryObject(el); RefPtr<BrowsingContext> browsingContext = flo->GetBrowsingContext(); // The BrowserID of the new BrowsingContext will be the same, so you can look it up by the same BrowserId. RefPtr<BrowsingContext> browsingContext = BrowsingContext::GetCurrentTopByBrowserId(browserId); >From this new BrowsingContext, you can then fetch the nsDocShell with the `BrowsingContext::GetDocShell()` method. RefPtr<mozilla::dom::XULFrameElement> frame = > mozilla::dom::XULFrameElement::FromNodeOrNull(el); > > uint64_t browserId = frame->BrowserId(); > RefPtr<CanonicalBrowsingContext> browsingContext = > CanonicalBrowsingContext::Cast( > BrowsingContext::GetCurrentTopByBrowserId(browserId)); > FWIW, you can do this a bit more simply by fetching the BC from the nsFrameLoaderOwner (leaving out null checks etc.): RefPtr<Element> el = ...; RefPtr<nsFrameLoaderOwner> flo = do_QueryObject(el); RefPtr<CanonicalBrowsingContext> bc = flo->GetBrowsingContext()->Canonical(); browsingContext->ChangeRemoteness(NOT_REMOTE_TYPE, 1, false, 0) > You ideally don't want to pass `1` as the pending load ID. This ID is a number which is used by `DocumentLoadListener` to pair up existing channels with the nsDocShell in the new process after a process switch has completed, so passing a dummy value can potentially cause you issues down the line. For print preview, we're passing in a `0` value as "this isn't due to an in-progress load" ( https://searchfox.org/mozilla-central/rev/165e8d8f80c02605c2f3e89c5de462abfce91c32/dom/ipc/ContentParent.cpp#3641), which is probably what you want to do here as well. Unfortunately, we explicitly check that it's non-zero for toplevel loads, as we currently have no uses for this type of process switch in Firefox ( https://searchfox.org/mozilla-central/rev/165e8d8f80c02605c2f3e89c5de462abfce91c32/docshell/base/CanonicalBrowsingContext.cpp#1193-1195). in order to remove that assertion, you'll need to change the logic here to instead force the creation of an initial about:blank document or similar, rather than calling `ResumeRedirectedLoad` on the docshell: https://searchfox.org/mozilla-central/rev/165e8d8f80c02605c2f3e89c5de462abfce91c32/docshell/base/CanonicalBrowsingContext.cpp#983-1023 . You're probably correct to pass `false, 0` for the last two arguments, unless you need to do specific things with BrowsingContextGroups such as allowing untrusted scripts in specific windows to communicate with one another. > ->Then( > GetMainThreadSerialEventTarget(), __func__, > [&](BrowserParent* aBrowserParent) { > fprintf(stderr, "success\n"); > > /* Can I get a docShell here?!! */ > > return NS_OK; > }, > [&](nsresult aStatusCode) { > fprintf(stderr, "failure\n"); > }); > You really don't want to be using the `[&]` capture mode here. This promise will be resolved long after the current stack frame has been dropped, so using a by-reference capturing method is very likely to lead to dangling references. You should capture relevant data by-value. -- Hopefully this is enough to get you started. I'm going to unfortunately be out most of next week, but feel free to reach out to me on Matrix if you have more specific questions. - Nika _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
