1224840 - mozmill run: Assertion failure: IsOuterWindow(), at /builds/slave/tb-c-cen-lx-d-0000000000000000/build/mozilla/dom/base/nsGlobalWindow.cpp:7678

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Magnus Melin [:mkmelin]]

Seems mozmill tests are hitting this


Assertion failure: IsOuterWindow(), at /builds/slave/tb-c-cen-lx-d-0000000000000000/build/mozilla/dom/base/nsGlobalWindow.cpp:7678
TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed

    908352 Permanent orange on Windows mozmill across many tests: TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed

OSError: [Errno 2] No such file or directory

    566330 Linux64 Talos failure: OSError: [Errno 2] No such file or directory

TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
Assertion failure: IsOuterWindow(), at /builds/slave/tb-c-cen-lx-d-0000000000000000/build/mozilla/dom/base/nsGlobalWindow.cpp:7678
TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed

    908352 Permanent orange on Windows mozmill across many tests: TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed

OSError: [Errno 2] No such file or directory

    566330 Linux64 Talos failure: OSError: [Errno 2] No such file or directory

TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run 

http://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-linux-debug/1447499202/comm-central_ubuntu32_vm-debug_test-mozmill-bm01-tests1-linux32-build6.txt.gz

Comment 1

[Stefan Sitter [:ssitter]]

Assertion happens in nsGlobalWindow::OpenDialog() and was added with Bug 1216401.
Assertion seem to happen during the following tests:

TEST-START | C:\slave\test\build\tests\mozmill\composition\test-attachment-reminder.js | test_attachment_reminder_dismissal 

TEST-START | C:\slave\test\build\tests\mozmill\folder-display\test-message-commands-on-msgstore.js | test_mark_messages_replied

Comment 2

[aleth [:aleth]]

Running test-attachment-reminder.js locally, I see a couple of
JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 4851: TypeError: gMsgCompose is null
which is in InitEditor(), along with a bunch of timeouts. (May or may not be related to the assertion.)

Comment 3

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #2)
> which is in InitEditor(), along with a bunch of timeouts. (May or may not be
> related to the assertion.)

The timeouts are related to waiting for windows/dialogs to open or focus, which could be connected to the bug mentined in comment 1.

Comment 4

[Jorg K (CEST = GMT+2)]

Sadly I'm getting the MOZ_ASSERT(IsOuterWindow()); from dom/base/nsGlobalWindow.cpp:7678 when trying to send a message in a debug build. Stack is:

nsGlobalWindow::OpenDialog() Line 7678	C++
nsMsgProgress::OpenProgressDialog() Line 80	C++
nsMsgCompose::SendMsg() Line 1246	C++

In SendMsg at this point it tries display the progress dialog:
OpenProgressDialog(... "chrome://messenger/content/messengercompose/sendProgress.xul", ...

Since this is MOZ_ASSERT, it's only triggered in a debug build. That explains why the "official" Daily I'm using works fine.

Altogether we're pretty much busted with this assertion in place.

Comment 5

[Jorg K (CEST = GMT+2)]

See bug 1216401 comment #12.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: Bustage fix.

As per bug 1216401 comment #13.

Aleth, can you please run the test, review, approve and land.

I checked that with this patch I can send my message again, since the progress dialog opens successfully.

Comment 7

[aleth [:aleth]]

Comment on attachment 8688172 [details] [diff] [review]
Bustage fix.

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

LGTM, but I'm not a peer of this code, so forwarding.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8688172 [details] [diff] [review]
Bustage fix.

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

::: mailnews/base/src/nsMsgProgress.cpp
@@ +55,1 @@
>    NS_ENSURE_ARG_POINTER(parent);

fwiw, if the original null check here was necessary, this really should be

nsCOMPtr<nsPIDOMWindow> parent(do_QueryInterface(parentDOMWindow));
NS_ENSURE_ARG_POINTER(parent);
parent = parent->GetOuterWindow();
NS_ENSURE_ARG_POINTER(parent);

It's not immediately obvious that using parent without null checking it first is same (because someone could in theory pass in a null parentDOMWindow.

Comment 9

[aleth [:aleth]]

This patch doesn't appear to fix the test failure from comment 2. Maybe there are other instances of the same problem?

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Bustage fix (v2).

OK, added one more check as per Kyle's comment, thanks for watching!

Aleth, yes, most likely there are other cases that need the same fix.

Those tests (like test-attachment-reminder.js, see comment #1) most likely display a dialog.

Comment 11

[Wayne Mery (:wsmwk)]

Awesome this is getting quick, solid attention.

I'm changing this to blocker, because it is blocking chiaki's development work that we're trying to get in for version 45.

Comment 12

[Jorg K (CEST = GMT+2)]

Aleth, can you please run the test(s) with the debugger attached and a breakpoint on nsGlobalWindow::OpenDialog() Line 7678	C++

From the call stack it should be obvious where we need to fix.

I've looked for calls to OpenDialog() in C-C and there aren't many in C++ code, but heaps in JS code:
http://mxr.mozilla.org/comm-central/search?string=OpenDialog

Wayne: This was holding up *my* development work, so I started looking at it on a side line.

Please note bug 1216401 comment #13: (Quote):
[The assertion was added] because nothing in mozilla-central made that call on an inner window.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Calls from JS go through a different codepath that is both not affected by the changes and is used in Firefox.

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks, shouldn't be to hard then, I only see these in C++ code:
/mailnews/addrbook/src/nsAbContentHandler.cpp
/mailnews/base/src/nsMsgProgress.cpp -- fixed already
/mailnews/news/src/nsNNTPNewsgroupList.cpp

Which makes me doubt that this is the cause of the test failure, since from the name of the test it's neither 'address book' nor 'news'. Hmm.

Anyway, a run with a debugger attached should give us clarity pretty quickly.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #2)
> Running test-attachment-reminder.js locally, ...

Please let me know how to run the test locally. I'm not familiar with Mozmill testing at all.

Comment 16

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #15)
> (In reply to aleth [:aleth] from comment #2)
> > Running test-attachment-reminder.js locally, ...
> 
> Please let me know how to run the test locally. I'm not familiar with
> Mozmill testing at all.

e.g., from the objdir, run
make SOLO_TEST=composition/test-attachment-reminder.js mozmill-one

Thanks for looking into this!

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8688175 [details] [diff] [review]
Bustage fix (v2).

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

rs=me
Are you going to do the other cases (comment 14) in this bug too?

Comment 18

[Jorg K (CEST = GMT+2)]

Not right now, I got side-tracked to bug 653342. I haven't tested the other cases from comment #14.

Also, there is still the issue of the test failing, comment #9, which may have other reasons.
I don't want to read up on Mozmill testing right now. The fix should be easy: Catch it in the debugger.

I'd be happy to land this patch and leave the bug open for someone else to continue.

Comment 19

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/dcb45e37d6dced19f7499135dc89caa1276c48ec
Bug 1224840 - Fix Assertion failure: IsOuterWindow() in nsMsgProgress.cpp. r=mkmelin

Comment 20

[aleth [:aleth]]

Attachment: Fix remaining "Assertion failure: IsOuterWindow()"

Comment 21

[aleth [:aleth]]

Let's leave test failures not due to this assertion for another bug.

Comment 22

[Jorg K (CEST = GMT+2)]

Sure, but the ones from comment #1 are fixed or not? Or is Stefan pointing the finger at the wrong tests:

TEST-START | C:\slave\test\build\tests\mozmill\composition\test-attachment-reminder.js | test_attachment_reminder_dismissal 

TEST-START | C:\slave\test\build\tests\mozmill\folder-display\test-message-commands-on-msgstore.js | test_mark_messages_replied

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 8688529 [details] [diff] [review]
Fix remaining "Assertion failure: IsOuterWindow()"

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

Thx, r=mkmelin

Comment 24

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/7824b6bb1869a884b2a6849f55bf36716527f079
Bug 1224840 - Fix remaining "Assertion failure: IsOuterWindow()". r=mkmelin

Comment 25

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #22)
> Sure, but the ones from comment #1 are fixed or not? Or is Stefan pointing
> the finger at the wrong tests:
> 
> TEST-START |
> C:\slave\test\build\tests\mozmill\composition\test-attachment-reminder.js |
> test_attachment_reminder_dismissal 
> 
> TEST-START |
> C:\slave\test\build\tests\mozmill\folder-display\test-message-commands-on-
> msgstore.js | test_mark_messages_replied

Looks like they are gone.

Comment 26

[aleth [:aleth]]

I can't seem to set uplift flags on these patches, but they are needed in c-a.

Comment 27

[Kent James (:rkent)]

Comment on attachment 8688529 [details] [diff] [review]
Fix remaining "Assertion failure: IsOuterWindow()"

I seem to be able to set them. Not sure  why Aleth had issues.

Comment 28

[aleth [:aleth]]

(In reply to Kent James (:rkent) from comment #27)
> Attachment #8688529 [details] [diff] - Flags: approval-mozilla-aurora?

Shouldn't it be approval-comm-aurora? Maybe I'm just confused.

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 8688529 [details] [diff] [review]
Fix remaining "Assertion failure: IsOuterWindow()"

Yes this is supposed to be approval-comm-aurora

Comment 30

[Kent James (:rkent)]

https://hg.mozilla.org/releases/comm-aurora/rev/c87549fd9bd7
https://hg.mozilla.org/releases/comm-aurora/rev/9f1e5fcd89ba