Changing browser remoteness

This is a multi-part message in MIME format.
--------------87AE05BE8182D00C39B69D1C
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Context: I'm trying to make Thunderbird E10s-compatible. We have 
<browser>s that can display web content or email messages. For now, 
email messages are going to have to be in the parent process. So, I need 
to be able to switch the remoteness of a <browser> in both directions.

I may be barking up the wrong tree completely here. Don't laugh at me. :-)

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.

However. The code for displaying messages is C++ all the way down. I 
want to be able to switch to the parent process and continue as before. 
Here's what I've managed to figure out:

   nsCOMPtr<nsPIDOMWindowOuter> win = nsPIDOMWindowOuter::From(mWindow);
   nsIDocShell* rootShell = win->GetDocShell();
   RefPtr<mozilla::dom::Element> el =
       rootShell->GetDocument()->GetElementById(u"messagepane"_ns);
   RefPtr<mozilla::dom::XULFrameElement> frame =
       mozilla::dom::XULFrameElement::FromNodeOrNull(el);

   uint64_t browserId = frame->BrowserId();
   RefPtr<CanonicalBrowsingContext> browsingContext =
       CanonicalBrowsingContext::Cast(
           BrowsingContext::GetCurrentTopByBrowserId(browserId));

   browsingContext->ChangeRemoteness(NOT_REMOTE_TYPE, 1, false, 0)
       ->Then(
           GetMainThreadSerialEventTarget(), __func__,
           [&](BrowserParent* aBrowserParent) {
             fprintf(stderr, "success\n");

             /* Can I get a docShell here?!! */

             return NS_OK;
           },
           [&](nsresult aStatusCode) {
             fprintf(stderr, "failure\n");
           });

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.

What do I do here?

GL


--------------87AE05BE8182D00C39B69D1C
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 7bit

<html>
  <head>

    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>Context: I'm trying to make Thunderbird E10s-compatible. We have
      <font face="monospace">&lt;browser&gt;</font>s that can display
      web content or email messages. For now, email messages are going
      to have to be in the parent process. So, I need to be able to
      switch the remoteness of a <font face="monospace">&lt;browser&gt;</font>
      in both directions.</p>
    <p>I may be barking up the wrong tree completely here. Don't laugh
      at me. :-)<br>
    </p>
    <p>I've borrowed <a moz-do-not-send="true"
href="https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#1832-1851">this
        code from Firefox</a> 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.<br>
    </p>
    <p>However. The code for displaying messages is C++ all the way
      down. I want to be able to switch to the parent process and
      continue as before. Here's what I've managed to figure out:</p>
    <pre>  nsCOMPtr&lt;nsPIDOMWindowOuter&gt; win = nsPIDOMWindowOuter::From(mWindow);
  nsIDocShell* rootShell = win-&gt;GetDocShell();
  RefPtr&lt;mozilla::dom::Element&gt; el = 
      rootShell-&gt;GetDocument()-&gt;GetElementById(u"messagepane"_ns);
  RefPtr&lt;mozilla::dom::XULFrameElement&gt; frame =
      mozilla::dom::XULFrameElement::FromNodeOrNull(el);

  uint64_t browserId = frame-&gt;BrowserId();
  RefPtr&lt;CanonicalBrowsingContext&gt; browsingContext =
      CanonicalBrowsingContext::Cast(
          BrowsingContext::GetCurrentTopByBrowserId(browserId));

  browsingContext-&gt;ChangeRemoteness(NOT_REMOTE_TYPE, 1, false, 0)
      -&gt;Then(
          GetMainThreadSerialEventTarget(), __func__,
          [&amp;](BrowserParent* aBrowserParent) {
            fprintf(stderr, "success\n");

            /* Can I get a docShell here?!! */

            return NS_OK;
          },
          [&amp;](nsresult aStatusCode) {
            fprintf(stderr, "failure\n");
          });
</pre>
    <p>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.</p>
    <p>I'm not sure I've got the arguments to <font face="monospace">ChangeRemoteness</font>
      right. I made up the second as a non-zero value is required, I
      think I understand the third but <font face="monospace">browsingContext</font>
      is discarded regardless, and I don't know anything about the
      fourth.</p>
    <p>What do I do here?</p>
    <p>GL<br>
    </p>
  </body>
</html>

--------------87AE05BE8182D00C39B69D1C--
0
Geoff
10/7/2020 3:12:04 AM
mozilla.dev.platform 6644 articles. 0 followers. Post Follow

1 Replies
11 Views

Similar Articles

[PageSpeed] 2

Replied with comments inline :-)

On Tue, Oct 6, 2020 at 11:15 PM Geoff Lankow <geoff@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
0
Nika
10/9/2020 5:08:13 PM
Reply: