1510382 - PROCESS-CRASH | newmailaccount | application crashed [@ nsGlobalWindowOuter::SetOpenerWindow(nsPIDOMWindowOuter*, bool)]

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

In opt builds we see:
PROCESS-CRASH | newmailaccount | application crashed [@ nsGlobalWindowOuter::SetOpenerWindow(nsPIDOMWindowOuter*, bool)]

In debug builds we have:
Assertion failure: !aOriginalOpener || !aOpener || aOpener->GetBrowsingContext() == GetBrowsingContext()->GetOpener(), at /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:2272

https://taskcluster-artifacts.net/HWzyuLyCQU2ubUmfGcsJUQ/0/public/logs/live_backing.log show that it's from this test:
TEST-START | /builds/worker/workspace/build/tests/mozmill/newmailaccount/test-newmailaccount.js

M-C last good: ce39a152428a7f8ba5a4c82455dcf501c7
M-C first bad: f83dcdf697697c8708e8015cd5c7b0b49d
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ce39a152428a7f8ba5a4c82455dcf501c7&tochange=f83dcdf697697c8708e8015cd5c7b0b49d

That's new here:
https://hg.mozilla.org/mozilla-central/rev/cb9dec83210d#l6.12

For opt builds the crash is here:

https://taskcluster-artifacts.net/VQ68sxiZQdyhkWnL9szjnQ/0/public/logs/live_backing.log

INFO -   0  libxul.so!nsGlobalWindowOuter::SetOpenerWindow(nsPIDOMWindowOuter*, bool) [nsGlobalWindowOuter.cpp:fab70cf9e35485946adfc8a507210a6f837c64e0 : 2270 + 0x0]
INFO -   1  libxul.so!nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, bool, bool, nsDocShellLoadState*, mozIDOMWindowProxy**) [nsWindowWatcher.cpp:fab70cf9e35485946adfc8a507210a6f837c64e0 : 2075 + 0x17]
INFO -   2  libxul.so!non-virtual thunk to nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsISupports*, bool, bool, nsDocShellLoadState*, mozIDOMWindowProxy**) [nsWindowWatcher.cpp:fab70cf9e35485946adfc8a507210a6f837c64e0 : 413 + 0x37]

Andreas, that shouldn't crash under any circumstance, should it?

NIing some C-C people who might want to take a look too.

Comment 1

[Jorg K (CEST = GMT+2)]

Running
mozmake SOLO_TEST=newmailaccount/test-newmailaccount.js mozmill-one
locally I get this stack:

Assertion failure: !aOriginalOpener || !aOpener || aOpener->GetBrowsingContext() == GetBrowsingContext()->GetOpener(), at c:/mozilla-source/comm-central/dom/base/nsGlobalWindowOuter.cpp:2272
#01: nsWindowWatcher::ReadyOpenedDocShellItem (c:\mozilla-source\comm-central\toolkit\components\windowwatcher\nsWindowWatcher.cpp:2081)
#02: nsWindowWatcher::OpenWindowInternal (c:\mozilla-source\comm-central\toolkit\components\windowwatcher\nsWindowWatcher.cpp:998)
#03: nsWindowWatcher::OpenWindow2 (c:\mozilla-source\comm-central\toolkit\components\windowwatcher\nsWindowWatcher.cpp:413)
#04: nsGlobalWindowOuter::OpenInternal (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowOuter.cpp:7157)
#05: nsGlobalWindowOuter::OpenOuter (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowOuter.cpp:5561)
#06: nsGlobalWindowInner::Open (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowInner.cpp:4093)
#07: mozilla::dom::Window_Binding::open (c:\mozilla-source\comm-central\obj-i686-pc-mingw32\dom\bindings\WindowBinding.cpp:2423)
#08: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (c:\mozilla-source\comm-central\dom\bindings\BindingUtils.cpp:3376)
#09: CallJSNative (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:468)
#10: js::InternalCallOrConstruct (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:560)
#11: InternalCall (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:614)

and more, but then we're already in JS land.

Comment 2

[Ben Campbell]

I've been trying to bring gdb to bear on this, but I'm really struggling with it here. I can attach it while running, but it always seems to fail on me (a poll() call in the jsbridge times out - the debugger slows things down, so I suspect I can track down and increase the timeout as a workaround).

There's reference in the mozmill docs and source to passing in "--debugger=gdb" to run it via gdb (you can pass in extra params by setting MOZMILL_EXTRA).
But digging down through all the layers of python, I can see that that the code to use the "--debugger" option invoke this isn't actually being run.
Is this just a bug, or am I doing something wrong, or is there some fundamental reason mozmill can't run under gdb after all?

Comment 3

[Andreas Farre [:farre]]

Yep, this is new code intended to replace workings of nsDocShell. Good thing is that it isn't actually replacing it at the moment, it only does the work in parallel and checks what happens is consistent.

In this particular case we've re-written how we set the opener of a browsing context. Before the opener would be set on an outer window in a method call after nsDocShell construction, but now we make it possible to pass the opener on construction and set it then. But this is something we only do for the new BrowsingContext class that we're moving nsDocShell functionality into in increments, so the old setting of opener is still there, but we want to use the fact that we know the opener that we're bound to set.

So if that assert happens something is either missing or the assert is not complete. I'll build thunderbird and take a look.

Ben, can you use rr and debug after?

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1510382-disable-test.patch

Only one subtest fails. If you want to run/debug is locally, please remove the line before running the test. You could also try to disable/delete the other parts since it's a very long test.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/48522dae2fd4
temporarily disable failing part of newmailaccount/test-newmailaccount.js. rs=bustage-fix

Comment 6

[Ben Campbell]

(In reply to Andreas Farre [:farre] from comment #3)
> Ben, can you use rr and debug after?

I think I just hit the same issue: rr is invoked instead of the executable, just as gdb (or valgrind or whatever else) is, right? That's what I'm having trouble with - the runtest.py script that thunderbird uses to run the mozmill tests just doesn't seem to handle running any debugger tool at all. I'm digging into it, but it's all a little twisty-turny down there in the depths of the mozmill modules :-) Will post an update if I have any breakthroughs, otherwise I'll find some workaround.

Comment 7

[Andreas Farre [:farre]]

Yeah, that would be awesome. Given an rr trace this should definitely be investigatable.

Comment 8

[Jorg K (CEST = GMT+2)]

Hmm, only one subtest fails here:

https://searchfox.org/comm-central/rev/5f75e643c8b41a286005bb2b1558bf06edae18e7/mail/test/mozmill/newmailaccount/test-newmailaccount.js#948

Looks like the "horrible" thing it's doing is this:
  open_content_tab_with_click(newTabLink, function(aURL) {
    return aURL.host == "localhost" && aURL.pathQueryRef == "/target.html";
  });

So somehow that calls into here:
https://searchfox.org/comm-central/rev/5f75e643c8b41a286005bb2b1558bf06edae18e7/mail/base/content/mailWindow.js#627
note the nice comment I added back then ;-)

So I guess something around that code will be wrong now. Perhaps visual inspected gets us further quicker than painful debugging. Andreas, can you please take another look.

Comment 9

[Andreas Farre [:farre]]

Sure thing, looking. It makes sense that it has something to do with opening windows. 

I might end up just removing that assert. I still think that it should hold, but it currently isn't necessary to hold for gecko to function.

The nice thing will be that given the possibility to test it I'm going to be able to break in SetOpenerWindow where this assert doesn't hold and actually see what's going on.

Comment 10

[Ben Campbell]

OK, so I've got a couple of ways to get the mozmill run invoking a debugger:

1) rename the Thunderbird binary and replace it with a wrapper script, eg:

    #!/bin/bash
    here=$( dirname ${BASH_SOURCE[0]} )
    gdb -ex "run" --args $here/thunderbird2 $@

2) hack the mozmill package to pass debug_args in to the runner
(for me, it's in obj-x86_64-pc-linux-gnu/_tests/mozmill-virtualenv/lib/python2.7/site-packages/mozmill/__init__.py):

in MozMill.start(), change:
        self.runner.start()
 to:
        self.runner.start(debug_args = ['gdb', '-ex', 'run', '--args'])

or whatever args should be on the commandline before the thunderbird executable.
(ideally, we'd pass these in via MOZMILL_EXTRA, but there are sooooo many mozmill layers to pass the args through, so for now, it's nasty-hack time).

This still doesn't seem to work so well for gdb - I think stdin/stdout are being co-opted by mozmill, so it's hard to tell what's going on and it doesn't seem interactive. And I've had enough digging about in mozmill for now.

But it does mean that an rr run should work just fine!

If rr ran on my machine. Which - as it turns out - it doesn't (https://github.com/mozilla/rr/issues/2034). Sigh.

Maybe this is useful to you Andreas?

But - as Jorg said - you can't really beat visual inspection. And printfs everywhere. So that's what I'm doing now.

Comment 11

[Ben Campbell]

Oh, I had a quick stab at running under valgrind (not so much for this bug as for others), but no dice there either.

Valgrind complained about missing support for a syscall (seccomp), then segfaulted.
A valgrind bug rather than a TB bug, but frustrating anyway.

Comment 12

[Ben Campbell]

Attachment: skip-non-asserting-tests.patch

helper patch to skip all the non-crashy tests. "Crash early, crash often" (tm)

Comment 13

[Andreas Farre [:farre]]

I have the review for the patch that fixes this over in Bug 1510928.

Comment 14

[Jorg K (CEST = GMT+2)]

Andreas, that sadly doesn't fix the problem, the crash persists:

https://hg.mozilla.org/mozilla-central/rev/6f780fe1f6cc#l1.15
+    MOZ_DIAGNOSTIC_ASSERT(
+        !aOriginalOpener || !aOpener ||
+        nsGlobalWindowOuter::Cast(aOpener)->IsClosedOrClosing() ||
+        aOpener->GetBrowsingContext() == GetBrowsingContext()->GetOpener());

Assertion failure: !aOriginalOpener || !aOpener || nsGlobalWindowOuter::Cast(aOpener)->IsClosedOrClosing() || aOpener->GetBrowsingContext() == GetBrowsingContext()->GetOpener(), at c:/mozilla-source/comm-central/dom/base/nsGlobalWindowOuter.cpp:2238
#01: nsWindowWatcher::OpenWindowInternal (c:\mozilla-source\comm-central\toolkit\components\windowwatcher\nswindowwatcher.cpp:927)
#02: nsWindowWatcher::OpenWindow2 (c:\mozilla-source\comm-central\toolkit\components\windowwatcher\nswindowwatcher.cpp:364)
#03: nsGlobalWindowOuter::OpenInternal (c:\mozilla-source\comm-central\dom\base\nsglobalwindowouter.cpp:6677)
#04: nsGlobalWindowOuter::OpenJS (c:\mozilla-source\comm-central\dom\base\nsglobalwindowouter.cpp:5262)
#05: nsGlobalWindowOuter::OpenOuter (c:\mozilla-source\comm-central\dom\base\nsglobalwindowouter.cpp:5228)
#06: nsGlobalWindowInner::Open (c:\mozilla-source\comm-central\dom\base\nsglobalwindowinner.cpp:3677)
#07: mozilla::dom::Window_Binding::open (c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dom\bindings\windowbinding.cpp:2452)
#08: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (c:\mozilla-source\comm-central\dom\bindings\bindingutils.cpp:3063)
#09: CallJSNative (c:\mozilla-source\comm-central\js\src\vm\interpreter.cpp:443)
#10: js::InternalCallOrConstruct (c:\mozilla-source\comm-central\js\src\vm\interpreter.cpp:535)
and so on.

Comment 15

[Andreas Farre [:farre]]

Right, I'll take another look.

Comment 16

[Andreas Farre [:farre]]

Attachment: Bug 1510382 - Don't assert when setting opener of ChromeBrowsingContexts.

Comment 17

[Andreas Farre [:farre]]

Ok, taken a long a thorough look and this is what I've found; after calling createContentWindow[1] the returned mozIDOMWindowProxy has mDocShell->mBrowsingContext->mOpener == nullptr. Looking at createContentWindow I see that it calls getContentWindowOrOpenURI[2] which in turn calls tabmail.openTab[3] to get a new tab and with that a new window. But that call to openTab doesn't pass opener an opener. Looking at openTab[4] I see a further call to openTab[5] which finally creates a tab, but still without passing opener while creating a window. The assert we see happens here[6], and to make it work the opener must be threaded to this point and presetOpenerWindow[7] needs to be called to provide the opener before the window is created.

Since this is a Thunderbird bug I'm unassigning myself, but hopefully this is enough information to be able to fix it in the right place.

[1] https://searchfox.org/comm-central/source/mail/base/content/mailWindow.js#628
[2] https://searchfox.org/comm-central/source/mail/base/content/mailWindow.js#642
[3] https://searchfox.org/comm-central/source/mail/base/content/mailWindow.js#681
[4] https://searchfox.org/comm-central/source/mail/base/content/tabmail.xml#497
[5] https://searchfox.org/comm-central/source/mail/base/content/specialTabs.js#702
[6] https://searchfox.org/comm-central/source/mail/base/content/specialTabs.js#723
[7] https://searchfox.org/mozilla-central/source/dom/webidl/MozFrameLoaderOwner.webidl#8

Comment 18

[Jorg K (CEST = GMT+2)]

Thanks, Andreas. I assume the patch adding
  !XRE_IsContentProcess() ||
will be dropped.

Let's see whether we can get this fixed with the information you've provided.

Any takers?

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1510382-pass-opener.patch

This is based on comment #17 and "pattern matching". I have to build first to try it. Andreas, something like this?

Comment 20

[Andreas Farre [:farre]]

Comment on attachment 9030760 [details] [diff] [review]
1510382-pass-opener.patch

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

Yes, I'm fairly certain that this is how you should do it. Maybe we can get the opinion of some Firefox front end dev as well.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 9030760 [details] [diff] [review]
1510382-pass-opener.patch

Doesn't work :-(

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1510382-pass-opener.patch (v2)

The previous version didn't work, in fact, it obliterated the test completely, and this version doesn't work either. I'm not sure to which object the
  .presetOpenerWindow(aArgs.opener)
should be applied.

This really isn't my area of expertise, so I'm giving up.

Comment 23

[Jorg K (CEST = GMT+2)]

The crash from comment #14 is still up-to-date apart from the line numbers. Here a refresh:

Assertion failure: !aOriginalOpener || !aOpener || nsGlobalWindowOuter::Cast(aOpener)->IsClosedOrClosing() || aOpener->GetBrowsingContext() == GetBrowsingContext()->GetOpener(), at c:/mozilla-source/comm-central/dom/base/nsGlobalWindowOuter.cpp:2239
#01: nsWindowWatcher::OpenWindowInternal (c:\mozilla-source\comm-central\toolkit\components\windowwatcher\nsWindowWatcher.cpp:929)
#02: nsWindowWatcher::OpenWindow2 (c:\mozilla-source\comm-central\toolkit\components\windowwatcher\nsWindowWatcher.cpp:364)
#03: nsGlobalWindowOuter::OpenInternal (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowOuter.cpp:6682)
#04: nsGlobalWindowOuter::OpenOuter (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowOuter.cpp:5233)
#05: nsGlobalWindowInner::Open (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowInner.cpp:3695)
#06: mozilla::dom::Window_Binding::open (c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dom\bindings\WindowBinding.cpp:2452)
#07: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (c:\mozilla-source\comm-central\dom\bindings\BindingUtils.cpp:3066)
#08: CallJSNative (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:443)

This time I also got a JS stack using this:
  if (mDocShell) {
    xpc_DumpJSStack(true, true, true);
    MOZ_DIAGNOSTIC_ASSERT(
        !aOriginalOpener || !aOpener ||
        // TODO(farre): Allowing to set a closed or closing window as

This is what I get right before the crash:
0 onclick(event = [object MouseEvent]) ["http://localhost:43336/registration.html?product=personalized_email&quote=b28acb3c0a464d33af22":1:0]
    this = [object HTMLParagraphElement]
1 synthesizeMouse(aTarget = [cross-compartment wrapper], aOffsetX = 410.5, aOffsetY = 9, aEvent = [object Object], aWindow = [cross-compartment wrapper]) ["chrome://mozmill/content/stdlib/EventUtils.js":238:6]
    this = [object Object]
2 MozMillController.prototype.mouseEvent(aTarget = [object Object], aOffsetX = 410.5, aOffsetY = 9, aEvent = [object Object], aExpectedEvent = undefined) ["chrome://mozmill/content/modules/controller.js":516:4]
    this = [object Object]

Andreas, how does that relate to comment #17?

Comment 24

[Andreas Farre [:farre]]

Yeah, so that is definitely expected. That assert triggers due to mDocShell->mBrowsingContext->mOpener == nullptr, but aOpener->mDocShell->mBrowsingContext != nullptr.

So, the patch is almost correct, sloppy reviewing from my side, sorry. Since you haven't appended the cloned element yet you'll never find the element with the id 'browser', so what you need to do is to call presetOpenerWindow on the clone!

If I do:

       // First clone the page and set up the basics.
       let clone = document.getElementById("contentTab").firstChild.cloneNode(true);
 
+      if ("opener" in aArgs && aArgs.opener)
+        clone.querySelector("browser").presetOpenerWindow(aArgs.opener);
+

instead, all tests passes!

Comment 25

[Jorg K (CEST = GMT+2)]

Works for me, I'll attach the patch in your name and get it landed. I hope there's no objection. It's all your idea ;-)

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 1510382-pass-opener.patch (v3)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f5c4404b29d2
Pass opener to specialTabs.openTab(). r=jorgk

Comment 28

[Jorg K (CEST = GMT+2)]

Beta (TB 65):
https://hg.mozilla.org/releases/comm-beta/rev/89aae8487741a3d1371d4e2a47ff353d14399674