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

Or should this getter live on Window? Or both?

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.

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.

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

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?

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

> conceptually the message manager is tied to the browsing context OK, that's a great reason to put this on docshell. ;)

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

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

FWIW, I was talking about the current state.

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.

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.

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

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

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

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

Pushed by bzbarsky@mozilla.com: 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

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

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

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.

Pushed by bzbarsky@mozilla.com: 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

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