The default bug view has changed. See this FAQ.

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
a year ago
a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
See bug 1216401 comment #12.

Comment 6

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

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

Comment 20

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

Updated

a year ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 21

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

Comment 22

a year 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

a year 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

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

Comment 25

a year 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: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
(Assignee)

Updated

a year ago
Blocks: 1216401
(Assignee)

Comment 26

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

Updated

a year ago
Flags: needinfo?(rkent)
Attachment #8688175 - Flags: approval-comm-aurora?
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

a year 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

a year 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

a year ago
status-thunderbird44: --- → affected
status-thunderbird45: --- → fixed
tracking-thunderbird44: --- → ?
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

a year ago
Attachment #8688175 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

a year ago
Attachment #8688529 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

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