1603649 - Port bug 1594288 - Remove nsIDocShellTreeItem.findChildWithName

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Geoff Lankow (:darktrojan)]

The build is on fire! :-(

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1603649-findchildwithname-1.diff

I think this works. It compiles anyway.

Comment 2

[Ben Campbell]

Attachment: 1603649-findchildwithname-2-tweakage.diff

a couple of tweaks:
in nsMsgWindow::GetMessageWindowDocShell:

• Fixes the rootShell/docShell one we both found.
• avoids one intermediate var (nsIDocShell is already an nsIDocShellItem, so no need to cast).
• added a comment about a dodgy-looking code path (which was already in there).

In nsMessenger::SetWindow() there was a bare nsIDocShell ptr which could have had some odd refcount issues. Changed it to the standard nsCOMPtr<nsIDocShell> and getter_AddRefs() pattern.

Geoff: You mentioned you'd spotted two mistakes... I feel like I might have missed one :-)

Compiles for me, haven't done any further testing on it yet.

Comment 3

[Ben Campbell]

(just a progress dump for Geoff)

OK, so nsMsgWindow::GetMessageWindowDocShell() doesn't find the "messagepane" it should be finding.
For a start, I don't think the patch is doing a recursive search down through the BrowsingContext tree, but there is a function for that -
BrowsingContext::FindWithNameInSubtree() instead of BrowsingContext::FindChildWithName().

When I use the recursive search, the entire tree is:

- ""  (root)
  - "accountCentralPane"
  - "multimessage"
  - "dummychromebrowser"
  - "" (no name)
  - "customizeToolbarSheetIFrame"

No messagepane.

Logging out all the BrowsingContext (look at the "Creating" lines):

[(null) 104518: Main Thread]: D/BrowsingContext Creating '' 0x00000001 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000001 to 0x00000000
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000001
[(null) 104518: Main Thread]: D/BrowsingContext Creating '' 0x00000002 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000002 to 0x00000000
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000002
[104518, Main Thread] WARNING: Workers don't support the 'mem.mem.' preference!: file /fast/ben/tb/mozilla/dom/workers/RuntimeService.cpp, line 540
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000001
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000002
[(null) 104518: Main Thread]: D/BrowsingContext Creating '' 0x00000003 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000003 to 0x00000000
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000003
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000003
[(null) 104518: Main Thread]: D/BrowsingContext Creating '' 0x00000004 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000004 to 0x00000000
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000004
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000004
[(null) 104518: Main Thread]: D/BrowsingContext Creating 'accountCentralPane' 0x00000005 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000005 to 0x00000002
[(null) 104518: Main Thread]: D/BrowsingContext Creating 'multimessage' 0x00000006 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000006 to 0x00000002
[(null) 104518: Main Thread]: D/BrowsingContext Creating 'messagepane' 0x00000007 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000007 to 0x00000000
[(null) 104518: Main Thread]: D/BrowsingContext Creating 'conv-log-browser' 0x00000008 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000008 to 0x00000000
[(null) 104518: Main Thread]: D/BrowsingContext Creating 'dummycontentbrowser' 0x00000009 in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x00000009 to 0x00000000
[(null) 104518: Main Thread]: D/BrowsingContext Creating 'dummychromebrowser' 0x0000000a in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x0000000a to 0x00000002
[(null) 104518: Main Thread]: D/BrowsingContext Creating '' 0x0000000b in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x0000000b to 0x00000002
[(null) 104518: Main Thread]: D/BrowsingContext Creating 'preferencesbrowser' 0x0000000c in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x0000000c to 0x00000000
[(null) 104518: Main Thread]: D/BrowsingContext Creating 'customizeToolbarSheetIFrame' 0x0000000d in Parent
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Connecting 0x0000000d to 0x00000002
[(null) 104518: Main Thread]: D/BrowsingContext Parent: Caching children of 0x00000007

(Browsingcontext doesn't log names by default - I had to hack it to do so)

So messagepane is there, ID 0x00000007.
But it's parented to 0x00000000, whereas we seem to be searching from 0x00000002.
So our rootDocshell is wrong, I think...

Comment 4

[Ben Campbell]

To turn on BrowsingContent logging: export MOZ_LOG="BrowsingContext:5"

Comment 5

[Ben Campbell]

OK, I think I've figured out the root cause of the problem.
"messagepane" isn't being found because it is type="content" rather than "chrome". This places it in a different group in the BrowsingContext tree, so it's not getting picked up from our starting search point (which is chrome).
no fix yet, but getting there :-)

Comment 6

[Ben Campbell]

Attachment: 1603649-findchildwithname-3.diff

Comment 7

[Ben Campbell]

Still not a working patch.

What I think is happening:
The GUI we're working with is being created via XUL: comm/mail/base/content/messenger.xul.
There are 4 or five things in there that end up with a type::Chrome BrowsingContext, but the one we're trying to get is "messagepane", which is type::Content, and is placed into a different BrowsingContentGroup. I can find the things that went into the chrome group, but I don't know to to search the content group instead.
(my attempt so far is in the patch, under nsMsgWindow::GetMessageWindowDocShell())

Andreas: Have you got any suggestions? (Just pinging you because you were on Bug 1594288, but feel free to pass me on to someone else!)

Comment 8

[Magnus Melin [:mkmelin]]

To clarify the question, we need the <browser name="messagepane" type="content"> https://searchfox.org/comm-central/rev/023f60c129f6d7eceed681e41916a24e8dd225f7/mail/base/content/messenger.xul#656 and since that is not in the chrome context, it's not being found. How to do that now.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: bug1603649_interateshells.patch

Looks like we could iterate over the available docshells like this. At least this seems to work for the stand-alone message window so I imagine you can adapt it for the general case. Getting late here, so over to you Ben.

Comment 10

[Jorg K (CEST = GMT+2)]

More inspiration here with green Xpcshell and MozMill:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a50c8c9f83867d0d21f9d773e1642e79fe093f6c
No idea why the Mochitests (bct) all failed, the X1 comes from bug 1603649 whose patch you need to apply to compile on current M-C trunk, see:
https://hg.mozilla.org/try-comm-central/graph/0c65361d7123dce27f9917b20fc9cdf59f450db4

I went through JS/XPCOM since I couldn't get the following to compile :-( - Yes, it should be RefPtr, but that doesn't compile either.
In the JS hack, you can dump out the object and you'll see that it's in fact a XULFrameElement.

NS_IMETHODIMP nsMsgWindow::GetMessageWindowDocShell(nsIDocShell **aDocShell) {
  *aDocShell = nullptr;
  nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mMessageWindowDocShellWeak));
  if (!docShell) {
    // if we don't have a docshell, then we need to look up the message pane
    // docshell
    nsCOMPtr<nsIDocShell> rootShell(do_QueryReferent(mRootDocShellWeak));
    if (rootShell) {
      nsCOMPtr<mozilla::dom::Element> el =
          rootShell->GetDocument()->GetElementById(
              NS_LITERAL_STRING("messagepane"));
      nsCOMPtr<mozilla::dom::XULFrameElement> frame = do_QueryObject(el);
      docShell = frame->GetContentDocument()->GetDocShell();  // EDIT: of course GetContentDocument()
      // we don't own mMessageWindowDocShell so don't try to keep a reference to
      // it!
      mMessageWindowDocShellWeak = do_GetWeakReference(docShell);
    }
  }
  docShell.forget(aDocShell);
  return NS_OK;
}

Comment 11

[Jorg K (CEST = GMT+2)]

You're going to add a break here, right? https://hg.mozilla.org/try-comm-central/rev/5affee694e5d2e56ce53030446d8fdb71c8b2a66#l2.34

Comment 12

[Ben Campbell]

Oh - well spotted, thanks! I'll add it in (Magnus's patch works for me locally with a tweak or two. Just setting up a try run now).

(edit: I got really confused there - I didn't realise those changes had made it up to try!)

Comment 13

[Ben Campbell]

Attachment: 1603649-findchildwithname-4.diff

This patch merges in Geoff's original work, some tweaks from me, and the working nsMsgWindow::GetMessageWindowDocShell() from Magnus.

Runs for me locally, and seems fine at first glance (runs, checks for new email, can pop up a compose window, etc...)

Try run here has a few oranges, but not sure if they are related to this or some other changes...

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5affee694e5d2e56ce53030446d8fdb71c8b2a66&selectedJob=281281112

(try run has this patch split into two commits - I screwed up the try push, but the end result is the same as this patch)

Comment 14

[Intermittent Failures Robot]

1 failures in 4867 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Platform breakdown:

• linux32: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1603649&startday=2019-12-09&endday=2019-12-15&tree=all

Comment 15

[Jorg K (CEST = GMT+2)]

Ben, the failure due to IsStreamUTF8() was already covered in
https://hg.mozilla.org/try-comm-central/graph/0c65361d7123dce27f9917b20fc9cdf59f450db4
see comment #10.

Comment 16

[Jorg K (CEST = GMT+2)]

Also, did you look at my suggestion in comment #10? Can that be made to work? Just like Magnus' approach, the Mochitests (bct) are all failing (not to mention the debug Z's which are all orange).

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9116042 [details] [diff] [review]
1603649-findchildwithname-4.diff

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

::: mailnews/base/src/nsMessenger.cpp
@@ +18,5 @@
>  #include "nsIMutableArray.h"
>  #include "mozilla/Path.h"
>  #include "mozilla/Services.h"
>  #include "mozilla/dom/LoadURIOptionsBinding.h"
> +#include "mozilla/dom/BrowsingContext.h"

Not needed, please remove.

@@ +101,5 @@
>  #include "nsIChannel.h"
>  #include "nsIOutputStream.h"
>  #include "nsIPrincipal.h"
>  
> +using mozilla::dom::BrowsingContext;

And this.

::: mailnews/base/src/nsMsgPrintEngine.cpp
@@ +40,5 @@
>  #include "nsContentUtils.h"
>  #include "nsIChannel.h"
>  #include "nsServiceManagerUtils.h"
>  
> +using mozilla::dom::BrowsingContext;

Can we lose this and stick it into RefPtr<BrowsingContext> instead. Anyway, code using this likely won't work.

@@ +231,2 @@
>  
> +  mDocShell = childContext->GetDocShell();

Have you tested this? I assume it won't work.

::: mailnews/base/src/nsMsgWindow.cpp
@@ +37,5 @@
>  #include "mozilla/dom/LoadURIOptionsBinding.h"
>  #include "mozilla/Components.h"
> +#include "mozilla/dom/BrowsingContext.h"
> +
> +using mozilla::dom::BrowsingContext;

Not needed, please remove.

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +48,5 @@
>  #include "mimemoz2.h"
>  #include "nsIArray.h"
>  #include "nsArrayUtils.h"
>  #include "nsIURIMutator.h"
> +#include "mozilla/dom/BrowsingContext.h"

And this.

@@ +68,5 @@
>  #  include <shellapi.h>
>  #  include "nsIWidget.h"
>  #endif
>  
> +using mozilla::dom::BrowsingContext;

And this.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9116042 [details] [diff] [review]
1603649-findchildwithname-4.diff

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

nsMessenger.cpp and nsMsgWindow.cpp seem to have operated on slightly different windows, so I'm not quite convinced it can all be put into the same method. (I could be wrong for sure.)

::: mailnews/base/src/nsMsgWindow.cpp
@@ +92,5 @@
>        mMessageWindowDocShellWeak = do_GetWeakReference(docShell);
>      }
> +    // TODO: if rootShell is null, we'll be returning a null docShell...
> +    // is NS_OK still the right returncode for this case or should it be
> +    // considered an error?

Null out should be error per xpcom conventions. Looks like the old code got away with it though.

Comment 19

[Jorg K (CEST = GMT+2)]

I'm not quite convinced it can all be put into the same method.

Indeed. And what is this code about that we're just removing now?
https://searchfox.org/comm-central/rev/023f60c129f6d7eceed681e41916a24e8dd225f7/mailnews/base/src/nsMessenger.cpp#231-239

Comment 20

[Magnus Melin [:mkmelin]]

That's the difference yes.
I've a new new patch that doesn't do the single method thing, and it seems to be working. At least spot checking some of the mochitest failures the other patch had, tests seem happy. I still need to clean up the patch a bit, and then I'll send it to try.

Comment 21

[Magnus Melin [:mkmelin]]

Let's see then: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c45a0d594d68b481fc5bb364cfe8db0d0402c422

Comment 22

[Jorg K (CEST = GMT+2)]

That won't compile.

While you're there, can you fix the commit message:
Bug 1603649 - Port bug 1594288: Replace use of nsIDocShellTreeItem.findChildWithName

You're not removing it, it's already been removed.

Comment 23

[Magnus Melin [:mkmelin]]

Why not? Compiles locally at least.

Comment 24

[Magnus Melin [:mkmelin]]

/builds/worker/workspace/build/src/comm/mailnews/base/util/nsMsgUtils.cpp:1751:13: error: unused function 'IsStreamUTF8' [-Werror,-Wunused-function]

I see, sorry I didn't notice that already was mentioned earlier. Too many balls in the air.
Why I don't get it locally, I don't know. Is there a flag I should add? Anyway, will fold that removal into bug 1602816.

New try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0db2d160793fe733dd7c60fdd2a12d5f3cd7182e

Comment 25

[Jorg K (CEST = GMT+2)]

Too many balls in the air.

Welcome to the life I had from 2016 until 20th Nov. 2019. If you think that two C++ bustages spread over a few days are too much, then you should remember that there were times when we had seven bustages within 24 hours, albeit, not all as complicated as this one.

Comment 26

[Magnus Melin [:mkmelin]]

Attachment: bugbug1603649_findshell.patch

As landing, please check

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f301cabc3820
Port bug 1594288: Replace use of nsIDocShellTreeItem.findChildWithName. rs=bustage-fix

Comment 28

[Jorg K (CEST = GMT+2)]

What do you guys think about the stuff in comment #10?

Comment 29

[Magnus Melin [:mkmelin]]

Could probably also have worked (if we get it to compile). Would probably make most sense to get feedback from someone who knows docshell.
I do think, the (non working) attempts to go trough browsingcontext were indeed wrong.

Comment 30

[Magnus Melin [:mkmelin]]

At least with the current approach we're sure to get the docshell of a content frame since we only iterate through those. If you look up by id, some other checks would be needed if we care to check that.

Comment 31

[Jorg K (CEST = GMT+2)]

Frankly, I don't understand why you didn't ask Boris. He would have given you an answer on Friday the latest.

Comment 32

[Ben Campbell]

Comment on attachment 9116090 [details] [diff] [review]
bugbug1603649_findshell.patch

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

::: mailnews/base/src/nsMessenger.cpp
@@ +234,5 @@
> +    nsTArray<RefPtr<nsIDocShell>> docShells;
> +    rootShell->GetAllDocShellsInSubtree(
> +        nsIDocShell::typeContent, nsIDocShell::ENUMERATE_FORWARDS, docShells);
> +    for (auto &docShell : docShells) {
> +      nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(docShell);

This casting isn't needed. `docShell` is already a `nsIDocShellTreeItem` by inheritance.

@@ +236,5 @@
> +        nsIDocShell::typeContent, nsIDocShell::ENUMERATE_FORWARDS, docShells);
> +    for (auto &docShell : docShells) {
> +      nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(docShell);
> +      bool childNameEquals = false;
> +      child->NameEquals(NS_LITERAL_STRING("messagepane"), &childNameEquals);

As a side issue, are "magic" identifiers like this (and some of the others) documented anywhere? Seems like the C++ has some specific requirements that the xul layout is expected to meet...

@@ -235,5 @@
> -    NS_ENSURE_TRUE(docShellAsItem, NS_ERROR_FAILURE);
> -
> -    nsCOMPtr<nsIDocShellTreeItem> rootDocShellAsItem;
> -    docShellAsItem->GetInProcessSameTypeRootTreeItem(
> -        getter_AddRefs(rootDocShellAsItem));

Is it an issue that we're no longer traversing up the tree to get the root docshell?
The fact that it seems to work fine suggests not.
But does it mean we're relying on the current structure in the xul layout which could change unexpectedly?

::: mailnews/base/src/nsMsgPrintEngine.cpp
@@ +216,5 @@
>    nsCOMPtr<nsPIDOMWindowOuter> window = nsPIDOMWindowOuter::From(mWindow);
>  
>    window->GetDocShell()->SetAppType(nsIDocShell::APP_TYPE_MAIL);
>  
> +  nsIDocShell *rootShell = window->GetDocShell();

again: should we be traversing up to root?

@@ +221,5 @@
> +  nsTArray<RefPtr<nsIDocShell>> docShells;
> +  rootShell->GetAllDocShellsInSubtree(
> +      nsIDocShell::typeContent, nsIDocShell::ENUMERATE_FORWARDS, docShells);
> +  for (auto &docShell : docShells) {
> +    nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(docShell);

again: cast not required.

@@ +225,5 @@
> +    nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(docShell);
> +    bool childNameEquals = false;
> +    child->NameEquals(NS_LITERAL_STRING("content"), &childNameEquals);
> +    if (childNameEquals) {
> +      mDocShell = do_QueryInterface(child);

probably don't need the QI here either.

::: mailnews/base/src/nsMsgWindow.cpp
@@ +76,5 @@
> +        rootShell->GetAllDocShellsInSubtree(nsIDocShell::typeContent,
> +                                            nsIDocShell::ENUMERATE_FORWARDS,
> +                                            docShells);
> +        for (auto &ds : docShells) {
> +          nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(ds);

again: casting not needed

@@ +80,5 @@
> +          nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(ds);
> +          bool childNameEquals = false;
> +          child->NameEquals(NS_LITERAL_STRING("messagepane"), &childNameEquals);
> +          if (childNameEquals) {
> +            docShell = do_QueryInterface(child);

again: qi not needed

::: mailnews/compose/src/nsMsgComposeService.cpp
@@ +252,5 @@
> +  nsTArray<RefPtr<nsIDocShell>> docShells;
> +  rootShell->GetAllDocShellsInSubtree(
> +      nsIDocShell::typeContent, nsIDocShell::ENUMERATE_FORWARDS, docShells);
> +  for (auto &docShell : docShells) {
> +    nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(docShell);

again: casting not needed

@@ +256,5 @@
> +    nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(docShell);
> +    bool childNameEquals = false;
> +    child->NameEquals(NS_LITERAL_STRING("messagepane"), &childNameEquals);
> +    if (childNameEquals) {
> +      messagePaneDocShell = do_QueryInterface(child);

again: QI not needed

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/321b8dd3f296
followup to remove unnecessary do_QueryInterface calls. r=benc

Comment 34

[Magnus Melin [:mkmelin]]

(In reply to Ben Campbell from comment #32)

•  child->NameEquals(NS_LITERAL_STRING("messagepane"), &childNameEquals);
  

As a side issue, are "magic" identifiers like this (and some of the others)
documented anywhere? Seems like the C++ has some specific requirements that
the xul layout is expected to meet...

Not really documented - you'd have to look with the dev tools inspector. But the name hasn't ever changed either. The

Is it an issue that we're no longer traversing up the tree to get the root
docshell?

I would think not, since it's now working.

The fact that it seems to work fine suggests not.
But does it mean we're relying on the current structure in the xul layout
which could change unexpectedly?

It can only change so much though. I'm not sure what one would have to do to make it not work, but a structure where it wouldn't get found sounds pretty weird.

Comment 35

[Andreas Farre [:farre]]

Comment on attachment 9116090 [details] [diff] [review]
bugbug1603649_findshell.patch

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

So this is a Fission thing. We're in the progress of removing nsIDocShellTreeItem, because in the Fission context, there may not be a complete nsDosShell tree available in a content process. What is available though is the replacement class: `BrowsingContext`. This should solve all your traversing needs. You can find the webidl interface here: https://searchfox.org/mozilla-central/source/dom/chrome-webidl/BrowsingContext.webidl. Another big difference between `nsDocShell` and `BrowsingContext`: all nodes in the BrowsingContext tree have the same type, i.e. there is a separation between content and chrome type browsing contexts.

::: mailnews/base/src/nsMessenger.cpp
@@ +231,5 @@
>      NS_ENSURE_TRUE(aWin, NS_ERROR_FAILURE);
>      nsCOMPtr<nsPIDOMWindowOuter> win = nsPIDOMWindowOuter::From(aWin);
> +    nsIDocShell *rootShell = win->GetDocShell();
> +    nsTArray<RefPtr<nsIDocShell>> docShells;
> +    rootShell->GetAllDocShellsInSubtree(

This will most certainly stop working post-Fission. You probably want `BrowsingContext::PreOrderWalk`

@@ +236,5 @@
> +        nsIDocShell::typeContent, nsIDocShell::ENUMERATE_FORWARDS, docShells);
> +    for (auto &docShell : docShells) {
> +      nsCOMPtr<nsIDocShellTreeItem> child = do_QueryObject(docShell);
> +      bool childNameEquals = false;
> +      child->NameEquals(NS_LITERAL_STRING("messagepane"), &childNameEquals);

There is also a method called `BrowsingContext::FindWithName`. Would that work here?

::: mailnews/base/src/nsMsgPrintEngine.cpp
@@ +218,5 @@
>    window->GetDocShell()->SetAppType(nsIDocShell::APP_TYPE_MAIL);
>  
> +  nsIDocShell *rootShell = window->GetDocShell();
> +  nsTArray<RefPtr<nsIDocShell>> docShells;
> +  rootShell->GetAllDocShellsInSubtree(

Same here.

Comment 36

[Andreas Farre [:farre]]

Hope this helps, otherwise just ping me in on irc or slack.

Comment 37

[Boris Zbarsky [:bzbarsky]]

all nodes in the BrowsingContext tree have the same type

That's actually a problem for Thunderbird: they are starting at the (chrome-type) root, and want to find the content-type docshell. See comment 5.

I'm not sure Fission is relevant to them, because they're not even using e10s...

Comment 38

[Jorg K (CEST = GMT+2)]

Exactly what Boris said, see comment #5 and comment #7. I looked through the available interfaces to see whether we could jump from the chrome tree to the content tree, but didn't see anything. Hence my attempt in comment #10, also see the linked try run there.

Comment 39

[Boris Zbarsky [:bzbarsky]]

to see whether we could jump from the chrome tree to the content tree

So just to be clear, in Firefox that's not even a thing, because those are in separate processes.

In the Thunderbird context, are there root content docshells other than the "messagepane" you want here? That is, could you get the treeowner for whatever docshell you do have and then GetPrimaryContentShell on it to get the right thing?

Comment 40

[Jorg K (CEST = GMT+2)]

Boris, what's wrong with the approach in comment #10. I didn't get it to compile, but hacking my way through JS worked:
https://hg.mozilla.org/try-comm-central/rev/0c65361d7123dce27f9917b20fc9cdf59f450db4
In JS you can go .docShell on a XULFrameElement, in C++ I failed to cast my Element to that class.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Boris, what's wrong with the approach in comment #10

Nothing a priori, except for the fact that you can't QI to XULFrameElement. If you know the thing you want is in an iframe directly in that root document, that seems like an OK approach.

Making that work will probably require adding a FromNode implementation on XULFrameElement. Something like:

   NS_IMPL_FROMNODE_HELPER(XULFrameElement, IsAnyOfXULElements(nsGkAtoms::iframe, nsGkAtoms::browser, nsGkAtoms::editor))

or so.

Comment 42

[Jorg K (CEST = GMT+2)]

Thanks. Personally I'd pursue this approach since it uses "standard" building blocks like GetElementById(). I don't understand why we need to dig around in docShells or browsing contexts if we can avoid it.

Comment 43

[Jorg K (CEST = GMT+2)]

Attachment: 1603649-replace-iteration.patch - WIP

That's my attempt. I'll submit the M-C part in a minute. But it still doesn't compile :-(

 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/RefPtr.h(50,38): error: member access into incomplete type 'nsFrameLoader'
 0:11.01   static void Release(U* aPtr) { aPtr->Release(); }
 0:11.01                                      ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/RefPtr.h(379,62): note: in instantiation of member function 'mozilla::RefPtrTraits<nsFrameLoader>::Release' requested here
 0:11.01     static void Release(U* aPtr) { mozilla::RefPtrTraits<U>::Release(aPtr); }
 0:11.01                                                              ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/RefPtr.h(81,37): note: in instantiation of member function 'RefPtr<nsFrameLoader>::ConstRemovingRefPtrTraits<nsFrameLoader>::Release' requested here
 0:11.01       ConstRemovingRefPtrTraits<T>::Release(mRawPtr);
 0:11.01                                     ^
 0:11.01 c:/mozilla-source/comm-central/dom/base\nsFrameLoaderOwner.h(69,11): note: in instantiation of member function 'RefPtr<nsFrameLoader>::~RefPtr' requested here
 0:11.01   virtual ~nsFrameLoaderOwner() = default;
 0:11.01           ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/mozilla/dom/Document.h(89,7): note: forward declaration of 'nsFrameLoader'
 0:11.01 class nsFrameLoader;
 0:11.01       ^
 0:11.01 In file included from c:/mozilla-source/comm-central/comm/mailnews/base/src/nsMsgWindow.cpp:6:
 0:11.01 In file included from c:/mozilla-source/comm-central/comm/mailnews/base/src/nsMsgWindow.h:13:
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/nsCOMPtr.h(1447,25): error: member 'COMTypeInfo' found in multiple base classes of different types
 0:11.01   if (NS_FAILED(aHelper(NS_GET_TEMPLATE_IID(T), &newRawPtr))) {
 0:11.01                         ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsID.h(180,45): note: expanded from macro 'NS_GET_TEMPLATE_IID'
 0:11.01 #define NS_GET_TEMPLATE_IID(T) (T::template COMTypeInfo<T, void>::kIID)
 0:11.01                                             ^
 0:11.01 c:/mozilla-source/comm-central/comm/mailnews/base/src/nsMsgWindow.cpp(80,55): note: in instantiation of member function 'RefPtr<mozilla::dom::XULFrameElement>::RefPtr' requested here
 0:11.01         RefPtr<mozilla::dom::XULFrameElement> frame = do_QueryObject(el);
 0:11.01                                                       ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/nsStyledElement.h(55,3): note: member found by ambiguous name lookup
 0:11.01   NS_DECLARE_STATIC_IID_ACCESSOR(NS_STYLED_ELEMENT_IID)
 0:11.01   ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsID.h(158,10): note: expanded from macro 'NS_DECLARE_STATIC_IID_ACCESSOR'
 0:11.01   struct COMTypeInfo;
 0:11.01          ^
 0:11.01 c:/mozilla-source/comm-central/dom/base\nsFrameLoaderOwner.h(39,3): note: member found by ambiguous name lookup
 0:11.01   NS_DECLARE_STATIC_IID_ACCESSOR(NS_FRAMELOADEROWNER_IID)
 0:11.01   ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsID.h(158,10): note: expanded from macro 'NS_DECLARE_STATIC_IID_ACCESSOR'
 0:11.01   struct COMTypeInfo;
 0:11.01          ^
 0:11.01 In file included from c:/mozilla-source/comm-central/comm/mailnews/base/src/nsMsgWindow.cpp:6:
 0:11.01 In file included from c:/mozilla-source/comm-central/comm/mailnews/base/src/nsMsgWindow.h:13:
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/nsCOMPtr.h(1447,25): error: implicit instantiation of undefined template 'nsStyledElement::COMTypeInfo<mozilla::dom::XULFrameElement, void>'
 0:11.01   if (NS_FAILED(aHelper(NS_GET_TEMPLATE_IID(T), &newRawPtr))) {
 0:11.01                         ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsID.h(180,36): note: expanded from macro 'NS_GET_TEMPLATE_IID'
 0:11.01 #define NS_GET_TEMPLATE_IID(T) (T::template COMTypeInfo<T, void>::kIID)
 0:11.01                                    ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/nsStyledElement.h(55,3): note: template is declared here
 0:11.01   NS_DECLARE_STATIC_IID_ACCESSOR(NS_STYLED_ELEMENT_IID)
 0:11.01   ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsID.h(158,10): note: expanded from macro 'NS_DECLARE_STATIC_IID_ACCESSOR'
 0:11.01   struct COMTypeInfo;
 0:11.01          ^
 0:11.01 In file included from c:/mozilla-source/comm-central/comm/mailnews/base/src/nsMsgWindow.cpp:6:
 0:11.01 In file included from c:/mozilla-source/comm-central/comm/mailnews/base/src/nsMsgWindow.h:13:
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/nsCOMPtr.h(1447,25): error: incomplete definition of type 'nsStyledElement::COMTypeInfo<mozilla::dom::XULFrameElement, void>'
 0:11.01   if (NS_FAILED(aHelper(NS_GET_TEMPLATE_IID(T), &newRawPtr))) {
 0:11.01       ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsID.h(180,65): note: expanded from macro 'NS_GET_TEMPLATE_IID'
 0:11.01 #define NS_GET_TEMPLATE_IID(T) (T::template COMTypeInfo<T, void>::kIID)
 0:11.01                                                                 ^
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsError.h(32,65): note: expanded from macro 'NS_FAILED'
 0:11.01 #define NS_FAILED(_nsresult) ((bool)MOZ_UNLIKELY(NS_FAILED_impl(_nsresult)))
 0:11.01                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
 0:11.01 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/Likely.h(17,48): note: expanded from macro 'MOZ_UNLIKELY'
 0:11.01 #  define MOZ_UNLIKELY(x) (__builtin_expect(!!(x), 0))

Looks like this is related to this inheritance:
class XULFrameElement final : public nsXULElement, public nsFrameLoaderOwner.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: Bug 1603649 - Add NS_IMPL_FROMNODE_HELPER for XULFrameElement (idea by :bz). r=farre

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9117102 [details] [diff] [review]
1603649-replace-iteration.patch - WIP

Again, do_QueryObject won't work here.

Either nsFrameLoaderOwner.h needs to include nsFrameLoader.h (if that doesn't lead to include cycles), or this code needs to include nsFrameLoader.h.

GetContentDocument() can return null.  This code should handle that.

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 1603649-replace-iteration.patch (v2) - WIP

Oops, we wanted to use the FromNode here, no query object. Still doesn't compile, frame loader still gets in the way:

 0:11.09 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/RefPtr.h(50,38): error: member access into incomplete type 'nsFrameLoader'
 0:11.09   static void Release(U* aPtr) { aPtr->Release(); }
 0:11.09                                      ^
 0:11.09 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/RefPtr.h(379,62): note: in instantiation of member function 'mozilla::RefPtrTraits<nsFrameLoader>::Release' requested here
 0:11.09     static void Release(U* aPtr) { mozilla::RefPtrTraits<U>::Release(aPtr); }
 0:11.09                                                              ^
 0:11.09 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/RefPtr.h(81,37): note: in instantiation of member function 'RefPtr<nsFrameLoader>::ConstRemovingRefPtrTraits<nsFrameLoader>::Release' requested here
 0:11.09       ConstRemovingRefPtrTraits<T>::Release(mRawPtr);
 0:11.09                                     ^
 0:11.09 c:/mozilla-source/comm-central/dom/base\nsFrameLoaderOwner.h(69,11): note: in instantiation of member function 'RefPtr<nsFrameLoader>::~RefPtr' requested here
 0:11.09   virtual ~nsFrameLoaderOwner() = default;
 0:11.09           ^
 0:11.09 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include/mozilla/dom/Document.h(89,7): note: forward declaration of 'nsFrameLoader'
 0:11.09 class nsFrameLoader;
 0:11.09       ^

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: 1603649-replace-iteration.patch (v3) - WIP

Sorry about the noise, this compiles now. I'll fix the other call sites.

Comment 48

[Boris Zbarsky [:bzbarsky]]

I still think you should null-check both the return value of GetElementByID and the return value of GetContentDocument.

Comment 49

[Jorg K (CEST = GMT+2)]

Attachment: 1603649-replace-iteration.patch (v4)

OK, this is the final version with the checks added that Boris suggested.
M-C changes here:
https://hg.mozilla.org/try/rev/6a55e3f727c8901dd59da7083192dcac20007665

C-C try pinned to that here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=055a48bcd7201ef0df0f41e715f34bedd2d3a54b

Comment 50

[Jorg K (CEST = GMT+2)]

Comment on attachment 9117133 [details] [diff] [review]
1603649-replace-iteration.patch (v4)

Green try. Ben, what do you think of this? (I messed around with this last weekend while Magnus prepared the r- solution).

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9117133 [details] [diff] [review]
1603649-replace-iteration.patch (v4)

In nsMessenger::SetWindow, should no element be a failure return or just skip setting mDocShell?

Comment 52

[Jorg K (CEST = GMT+2)]

Good question. You're referring to this:

+    RefPtr<mozilla::dom::Element> el = rootShell->GetDocument()->GetElementById(
+        NS_LITERAL_STRING("messagepane"));
+    NS_ENSURE_TRUE(el, NS_ERROR_FAILURE);  <== check here or not?
+    RefPtr<mozilla::dom::XULFrameElement> frame =
+        mozilla::dom::XULFrameElement::FromNode(el);
+    RefPtr<mozilla::dom::Document> doc = frame->GetContentDocument();
+    if (doc) mDocShell = doc->GetDocShell();

Well, the messagepane element should be there, or something is not correctly configured. I could be wrong. Nothing popped up in the try.

Comment 53

[Jorg K (CEST = GMT+2)]

Thanks, Andreas. I assume the Phab review is coming since it won't work without it ;-)

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db4fb3041de5
Add NS_IMPL_FROMNODE_HELPER for XULFrameElement (idea by :bz). r=farre

Comment 55

[Ben Campbell]

Comment on attachment 9117133 [details] [diff] [review]
1603649-replace-iteration.patch (v4)

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

Definitely a much nicer approach. I like it.
Looks good to me. Compiles (with the m-c patch) and seems to run fine (at least it starts up :- ).

Possibly could be a little more resilient to errors in xul, but I'm happy with it as is.

::: mailnews/base/src/nsMsgPrintEngine.cpp
@@ +223,5 @@
> +  RefPtr<mozilla::dom::Element> el =
> +      rootShell->GetDocument()->GetElementById(NS_LITERAL_STRING("content"));
> +  NS_ENSURE_TRUE(el, NS_ERROR_FAILURE);
> +  RefPtr<mozilla::dom::XULFrameElement> frame =
> +      mozilla::dom::XULFrameElement::FromNode(el);

Can FromNode() ever fail?
(eg I'm imagining a (far-fetched) case where someone accidentally plonks another id="content" element into a XUL file, one which isn't a XULFrameElement)
For things that can be screwed up by mistakes in data files, I feel it's probably best to fail deliberately. But the main thing is that it at least dies obviously, rather than causing mysterious problems down the line, and a nullptr access will do that :-)

::: mailnews/compose/src/moz.build
@@ +50,5 @@
>    '/%s/dom/base' % CONFIG['mozreltopsrcdir'],
>  ]
>  
> +include('/ipc/chromium/chromium-config.mozbuild')
> +

I'm curious to know what this is for. I'm imagining it's to deal with some exciting and exotic frame processs-isolation stuff, but it's probably more mundane :-)

Comment 56

[Jorg K (CEST = GMT+2)]

Can FromNode() ever fail?

I don't think so. It's there to "cast" in cases where we can. Check use of that in the C-C source:
https://searchfox.org/comm-central/search?q=%3A%3AFromNode&path=mailnews

include('/ipc/chromium/chromium-config.mozbuild')

... is already present here: https://searchfox.org/comm-central/rev/033441198427ba2e9be601abf19487f0e143e3b4/mailnews/base/src/moz.build#72

I don't have the exact details, but without it, it doesn't compile. I think the frame loader stuff is responsible for that.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9117133 [details] [diff] [review]
1603649-replace-iteration.patch (v4)

Oh, yeah.  It might make more sense to remove the null-check for "el", use FromNodeOrNull, then null-check the result...  That will catch both the null case and the "not a XULFrameElement" case.

Comment 58

[Jorg K (CEST = GMT+2)]

Alright. Thanks. So the helper implements this as well, I take it.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Yep, the macros implement all sorts of versions of FromNode and FromNodeOrNull.

Comment 60

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/db4fb3041de5

Comment 61

[Jorg K (CEST = GMT+2)]

Attachment: 1603649-replace-iteration.patch (v5)

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a4ce41420231
Follow-up: replace docShell iteration. r=farre,benc