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

RESOLVED FIXED in Thunderbird 45.0

Status

Thunderbird
General
--
blocker
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Magnus Melin, Assigned: aleth)

Tracking

({intermittent-failure})

Trunk
Thunderbird 45.0
intermittent-failure

Thunderbird Tracking Flags

(thunderbird43 unaffected, thunderbird44 fixed, thunderbird45 fixed, thunderbird_esr38 unaffected)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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

2 years ago
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
(Assignee)

Comment 2

2 years ago
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.)
(Assignee)

Comment 3

2 years ago
(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

2 years ago
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

2 years ago
See bug 1216401 comment #12.

Comment 6

2 years ago
Created attachment 8688172 [details] [diff] [review]
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.
Attachment #8688172 - Flags: review?(aleth)
(Assignee)

Comment 7

2 years ago
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.
Attachment #8688172 - Flags: review?(aleth) → review?(mkmelin+mozilla)
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.
Attachment #8688172 - Flags: feedback-
(Assignee)

Comment 9

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

Comment 10

2 years ago
Created attachment 8688175 [details] [diff] [review]
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.
Attachment #8688172 - Attachment is obsolete: true
Attachment #8688172 - Flags: review?(mkmelin+mozilla)
Attachment #8688175 - Flags: review?(mkmelin+mozilla)
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.
Severity: normal → blocker

Comment 12

2 years ago
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.
Calls from JS go through a different codepath that is both not affected by the changes and is used in Firefox.

Comment 14

2 years ago
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

2 years ago
(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.
(Assignee)

Comment 16

2 years ago
(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!
(Reporter)

Comment 17

2 years ago
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?
Attachment #8688175 - Flags: review?(mkmelin+mozilla) → review+

Comment 18

2 years ago
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.
(Assignee)

Comment 19

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

Comment 20

2 years ago
Created attachment 8688529 [details] [diff] [review]
Fix remaining "Assertion failure: IsOuterWindow()"
Attachment #8688529 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

2 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 21

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

Comment 22

2 years ago
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
(Reporter)

Comment 23

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

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

Thx, r=mkmelin
Attachment #8688529 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 24

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

Comment 25

2 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
(Assignee)

Updated

2 years ago
Blocks: 1216401
(Assignee)

Comment 26

2 years ago
I can't seem to set uplift flags on these patches, but they are needed in c-a.
Flags: needinfo?(rkent)

Updated

2 years ago
Flags: needinfo?(rkent)
Attachment #8688175 - Flags: approval-comm-aurora?

Comment 27

2 years ago
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.
Attachment #8688529 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 28

2 years ago
(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.
(Reporter)

Comment 29

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

Yes this is supposed to be approval-comm-aurora
Attachment #8688529 - Flags: approval-mozilla-aurora? → approval-comm-aurora?

Updated

2 years ago
status-thunderbird44: --- → affected
status-thunderbird45: --- → fixed
tracking-thunderbird44: --- → ?

Comment 30

2 years ago
https://hg.mozilla.org/releases/comm-aurora/rev/c87549fd9bd7
https://hg.mozilla.org/releases/comm-aurora/rev/9f1e5fcd89ba
status-thunderbird43: --- → unaffected
status-thunderbird44: affected → fixed
status-thunderbird_esr38: --- → unaffected
tracking-thunderbird44: ? → ---

Updated

2 years ago
Attachment #8688175 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

2 years ago
Attachment #8688529 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

2 years ago
See Also: → bug 1239896
You need to log in before you can comment on or make changes to this bug.