1479569 - Add a nsIContentFrameMessageManager getter on nsIDocShell

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

We have various code doing getInterface(Ci.nsIContentFrameMessageManager) to get this thing.  Seems a bit silly.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Or should this getter live on Window?  Or both?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Interestingly, some people getting a message manager seem to go to the sameTypeRootTreeItem and get it there, and some get it directly from the window's docshell....

Looking at the actual implementation of getInterface(Ci.nsIContentFrameMessageManager), in the "TabChild::From returns non-null" case, it's the same TabChild whether you go  to the sameTypeRootTreeItem or not.  And in the other case we go to the parent target of the window, which is not going to be the tabchild, presumably, so goes off to GetScriptableTopInternal and gets its frameElement, and if there isn't one falls back on mChromeEventHandler.  Seems like all that would be the same whether one does .sameTypeRootTreeItem or not.

Comment 3

[:Gijs (he/him)]

From my understanding of the DOM talk from the SF all-hands, we're going to start doing IPC on a per-content-frame basis soon, so code that currently tries to use the toplevel might need changing so it uses the one off its "own" frame...

If we can avoid asking for the docshell by moving the getter to `window`, that seems preferable to me.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> Or should this getter live on Window?  Or both?

Just docShell sounds fine to me. This probably isn't used often enough to warrant a getter on window.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> Interestingly, some people getting a message manager seem to go to the
> sameTypeRootTreeItem and get it there, and some get it directly from the
> window's docshell....
> 
> Looking at the actual implementation of
> getInterface(Ci.nsIContentFrameMessageManager), in the "TabChild::From
> returns non-null" case, it's the same TabChild whether you go  to the
> sameTypeRootTreeItem or not.  And in the other case we go to the parent
> target of the window, which is not going to be the tabchild, presumably, so
> goes off to GetScriptableTopInternal and gets its frameElement, and if there
> isn't one falls back on mChromeEventHandler.  Seems like all that would be
> the same whether one does .sameTypeRootTreeItem or not.

It's probably just cargo culted, but if it does the right thing for any docshell in the same tree, we should probably update all users to get it from the nearest docshell. That will be more likely to do the right thing post-Fission.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So most of the consumers start off with a Window.  A few start off with a docshell.  

It's easy to go back and forth between them using .docShell and .domWindow, I guess.  But in general, I am leaning toward putting this on Window.  Especially because conceptually the message manager is tied to the Window, not the browsing context, right?

Comment 6

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> It's easy to go back and forth between them using .docShell and .domWindow,
> I guess.  But in general, I am leaning toward putting this on Window. 
> Especially because conceptually the message manager is tied to the Window,
> not the browsing context, right?

At least as things stand now, conceptually the message manager is tied to the browsing context, not the window. When a docShell navigates, we still have the same parent and child message managers, and any event or message listeners registered on the message manager stay registered.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> conceptually the message manager is tied to the browsing context

OK, that's a great reason to put this on docshell.  ;)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Bound to the top level browsing context, not any browsing context.

Comment 9

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Bound to the top level browsing context, not any browsing context.

(In reply to Kris Maglione [:kmag] from comment #4)
> we should probably update all users to get it
> from the nearest docshell. That will be more likely to do the right thing
> post-Fission.

I'm having trouble reconciling these statements. As I noted in comment #3, aren't we intending to do per-frame processes, and thus we'll need per-frame parent->content communication, be it via IPC or message managers? I don't understand how that'd work if we could only get a message manager from a toplevel docshell / browsing context. (Yes, I know we might want to remove message managers and switch to IPC or something similar wholesale, but the same paradigm remains and it'd be easier to convert code if it already used a per-docshell message manager accessor - even if today, that accessor returns the same MM for all docshells in a tree.)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I was talking about the current state.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

And even then, at least at some point the idea was to have mm per top level abstract browsing context tree in Fission, since per docshell would be too heavy.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a ContentFrameMessageManager getter on nsIDocShell

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use the new messageManager getter on docshell

I generally tried to preserve the behavior of consumers where they treated an
exception from getInterface(Ci.nsIContentFrameMessageManager) as a signal to use
some sort of fallback.

I did change the behavior of consumers that walked up to the root same-type
docshell before getting the message manager to just get it directly from the
docshell they have.  Please review those parts carefully, and let me know if you
want me to ask some subject area experts to review those.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove nsIContentFrameMessageManager

Comment 15

[Kris Maglione [:kmag]]

(In reply to :Gijs (Not available 3-19 Aug; he/him) from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > Bound to the top level browsing context, not any browsing context.
> 
> (In reply to Kris Maglione [:kmag] from comment #4)
> > we should probably update all users to get it
> > from the nearest docshell. That will be more likely to do the right thing
> > post-Fission.
> 
> I'm having trouble reconciling these statements. As I noted in comment #3,
> aren't we intending to do per-frame processes, and thus we'll need per-frame
> parent->content communication, be it via IPC or message managers? I don't
> understand how that'd work if we could only get a message manager from a
> toplevel docshell / browsing context.

I don't think it's entirely clear at this point. In any case, even with OOP iframes, we'll still have conceptual frame trees which may include parent frames that live in a different process. And same-origin frames will still always live in the same process.

(In reply to Olli Pettay [:smaug] from comment #11)
> And even then, at least at some point the idea was to have mm per top level
> abstract browsing context tree in Fission, since per docshell would be too
> heavy.

Once bug 1480244 and bug 1472491 are fixed, I think we'll actually be able to afford doing them per-docshell.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8997124 [details] [diff] [review]
part 1.  Add a ContentFrameMessageManager getter on nsIDocShell

Review of attachment 8997124 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +3975,5 @@
> +nsDocShell::GetMessageManager(ContentFrameMessageManager** aMessageManager)
> +{
> +  RefPtr<TabChild> tabChild = TabChild::GetFrom(this);
> +  RefPtr<ContentFrameMessageManager> mm;
> +  if (tabChild) {

Nit: Maybe make this `if (RefPtr<TabChild> tabChild = ...)` to match the style of the next if condition?

Also... I'm *pretty* sure this doesn't need a RefPtr as things currently stand. But the TabChild::GetFrom(nsIDocShell) code looks pretty buggy[1] as-is. It calls docShell->GetTabChild(), which returns an already_AddRefed, then static_casts it and returns it as a raw pointer. If GetTabChild ever returns an instance with a single reference, that's going to lead to a UAF... So I guess we should change TabChild to return an already_AddRefed instead of changing this.

[1]: https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/dom/ipc/TabChild.h#519-520

::: docshell/base/nsIDocShell.idl
@@ +1205,5 @@
> +  /**
> +   * The message manager for this docshell.  This does not throw, but
> +   * can return null if the docshell has no message manager.
> +   */
> +  [infallible] readonly attribute ContentFrameMessageManager messageManager;

Oh, can non-integer getters be infallible now?

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8997125 [details] [diff] [review]
part 2.   Use the new messageManager getter on docshell

Review of attachment 8997125 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/test/chrome/file_bug549682.xul
@@ +146,1 @@
>      ok(cfmm, "Should have content messageManager");

Hm. This would have always been true in the old code... But it makes sense now, at least.

::: dom/ipc/remote-test.js
@@ +25,1 @@
>                              sendSyncMessage("linkclick", { href: e.target.href });

Missing "." + automatic semicolon insertion = working code that does the wrong thing. Please the dot to the start of this line rather than the end of the previous line, though.

::: mobile/android/modules/FxAccountsWebChannel.jsm
@@ +195,5 @@
>          };
>  
>          switch (command) {
>            case COMMAND_LOADED:
> +            let mm = sendingContext.browser.docShell.messageManager;

Hm. I guess this is different from `sendingContext.browser.messageManager`, but confusingly similar. Might warrant a comment that we're getting the child side of the message manager here rather than the parent.

::: toolkit/components/extensions/extension-process-script.js
@@ +88,5 @@
>                                       matcher);
>  });
>  
>  function getMessageManager(window) {
> +  return window.docShell.messageManager;

I'd rather we just inline these two callers at this point. One less function script we need to keep in memory for each process.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +153,5 @@
>  observer.onPrefChange(); // read initial values
>  
>  
>  function messageManagerFromWindow(win) {
> +  return win.docShell.messageManager;

I'd lean toward inlining these calls, too. It actually winds up being shorter. But I don't have as strong an opinion on it.

::: toolkit/modules/E10SUtils.jsm
@@ +310,1 @@
>      let sessionHistory = aDocShell.getInterface(Ci.nsIWebNavigation).sessionHistory;

Need to change the `getInterface` to a `QueryInterface` since you removed the QI to nsIInterfaceRequestor on the previous line.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8997126 [details] [diff] [review]
part 3.  Remove nsIContentFrameMessageManager

Review of attachment 8997126 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/shistory/ChildSHistory.cpp
@@ +138,5 @@
> +  if (mDocShell) {
> +    mm = mDocShell->GetMessageManager();
> +  }
> +  // else we must be unlinked... can that happen here?
> +  return ToSupports(mm);

This should probably return an already_AddRefed... though I guess it's no worse now than it was before.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Nit: Maybe make this `if (RefPtr<TabChild> tabChild = ...)` to match the style of the next if condition?

Done.

> So I guess we should change TabChild to return an already_AddRefed instead of changing this.

Bug 1480567 filed.

> Oh, can non-integer getters be infallible now?

Yep.  I even checked the generated code and it looks good.  ;)

> Missing "." + automatic semicolon insertion

Ugh.  ASI strikes again.  I'm surprised our eslint job doesn't go orange when it detects ASI!

Anyway, fixed with '.' at start of line.  Thank you for catching this!

> Might warrant a comment that we're getting the child side of the message manager

Done.

> I'd rather we just inline these two callers at this point

Done.

> I'd lean toward inlining these calls, too.

Done.

> Need to change the `getInterface` to a `QueryInterface`

Good catch, done.

> This should probably return an already_AddRefed...

GetParentObject can't really do that, unfortunately...

Comment 20

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)
> Ugh.  ASI strikes again.  I'm surprised our eslint job doesn't go orange
> when it detects ASI!

It does, but dom/ipc is on the .eslintignore list :(

Comment 21

[Boris Zbarsky [:bzbarsky]]

Try showed some leaks from not adding mMessageManager to the CC bis for outer window.  Fixing that makes the leaks go away.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97041ef8f311
part 1.  Add a ContentFrameMessageManager getter on nsIDocShell.  r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/b828a58404e5
part 2.   Use the new messageManager getter on docshell.  r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/e123d0aa911c
part 3.  Remove nsIContentFrameMessageManager.  r=kmag

Comment 23

[Natalia Csoregi [:nataliaCs]]

Backed out for devtools/client/responsive.html leaks

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/90193bf8d83d86f9e4f3a07de1c258dc4a77e008

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e123d0aa911c4fa3501042d2febecac1a2766b60&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191797542&repo=mozilla-inbound&lineNumber=11796

[task 2018-08-03T06:34:12.051Z] 06:34:12     INFO - TEST-INFO | Main app process: exit 0
[task 2018-08-03T06:34:12.052Z] 06:34:12    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_network_throttling.js | leaked 1 window(s) until shutdown [url = data:text/html;charset=utf-8,Network throttling test]
[task 2018-08-03T06:34:12.053Z] 06:34:12     INFO - TEST-INFO | devtools/client/responsive.html/test/browser/browser_network_throttling.js | windows(s) leaked: [pid = 1989] [serial = 92]
[task 2018-08-03T06:34:12.054Z] 06:34:12    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_network_throttling.js | leaked 1 docShell(s) until shutdown
[task 2018-08-03T06:34:12.055Z] 06:34:12     INFO - TEST-INFO | devtools/client/responsive.html/test/browser/browser_network_throttling.js | docShell(s) leaked: [pid = 1989] [id = {648880c1-a490-4b0b-9e71-710b8f5d53fc}]
[task 2018-08-03T06:34:12.056Z] 06:34:12    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_window_close.js | leaked 2 window(s) until shutdown [url = about:newtab]
[task 2018-08-03T06:34:12.057Z] 06:34:12     INFO - TEST-INFO | devtools/client/responsive.html/test/browser/browser_window_close.js | windows(s) leaked: [pid = 1961] [serial = 151], [pid = 1961] [serial = 153]
[task 2018-08-03T06:34:12.058Z] 06:34:12     INFO - runtests.py | Application ran for: 0:09:01.129240

Comment 24

[Kris Maglione [:kmag]]

This shaved another 10KB off base content JS memory usage.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Looks like in devtools/server/actors/targets/browsing-context.js this.docShell can be null and the try/catch was covering that up.  I'll just put the try/catch back in, like so:

    try {
      return this.docShell.messageManager;
    } catch (e) {
      // In some cases we can't get a docshell.  We just have no message manager
      // then,
      return null;
    }

and that fixes the leak.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21ea4758f515
part 1.  Add a ContentFrameMessageManager getter on nsIDocShell.  r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd76057ca094
part 2.   Use the new messageManager getter on docshell.  r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/21cbd88d4ea1
part 3.  Remove nsIContentFrameMessageManager.  r=kmag

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/21ea4758f515
https://hg.mozilla.org/mozilla-central/rev/cd76057ca094
https://hg.mozilla.org/mozilla-central/rev/21cbd88d4ea1