Closed Bug 1571203 Opened 5 years ago Closed 5 years ago

TEST-UNEXPECTED-FAIL | [snip]/mozmill/newmailaccount/test-newmailaccount.js

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 70.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])

Attachments

(2 files, 1 obsolete file)

1571203-disable-test.patch
1.01 KB, patch
Details | Diff | Splinter Review
1571203-browsing-context.patch (v2)
3.22 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_window_open_link_opening_behaviour
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_external_link_opening_behaviour
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_return_to_provisioner_on_error_XML
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_disabled_fields_when_searching
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_search_button_disabled_if_no_lang_support
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_provider_language_wildcard
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_search_button_disabled_if_no_query_on_init
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_get_new_account_focuses_existing_ap_tab
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1564833101/build/tests/mozmill/newmailaccount/test-newmailaccount.js | test-newmailaccount.js::test_per_address_prices

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=37229cef2cc79d44470afc9e04016bac8d&tochange=7c5c34ef4efc5fc762398d7111850f7119

I've already fixed two total bustages today, so I'm a bit tired :-(

Log says:
https://taskcluster-artifacts.net/YX3o_cLoQJ2jHLB3fuil_Q/0/public/logs/live_backing.log

EXCEPTION: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host]
and then some of:
EXCEPTION: Timeout waiting for modal dialog to open.

Where did that host go? I'm going to the beach now.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)
Flags: needinfo?(acelists)

This is ugly, I ran this locally and saw:

TEST-START | c:\mozilla-source\comm-central\comm\mail\test\mozmill\newmailaccount\test-newmailaccount.js | test_external_link_opening_behaviour
[10556, Main Thread] WARNING: '!focusNode', file c:/mozilla-source/comm-central/editor/libeditor/HTMLEditor.cpp, line 4851
[10556, Main Thread] WARNING: '!focusElement', file c:/mozilla-source/comm-central/editor/libeditor/HTMLAnonymousNodeEditor.cpp, line 337
[10556, Main Thread] WARNING: HTMLEditRules::BeforeEdit() failed to handle something: 'NS_SUCCEEDED(rv)', file c:/mozilla-source/comm-central/editor/libeditor/HTMLEditor.cpp, line 3754
Assertion failure: !aComposerCommandsUpdater || !mComposerCommandsUpdater || aComposerCommandsUpdater == mComposerCommandsUpdater, at c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/HTMLEditor.h:500
#01: nsEditingSession::OnStateChange (c:\mozilla-source\comm-central\editor\composer\nsEditingSession.cpp:684)
#02: nsDocLoader::DoFireOnStateChange (c:\mozilla-source\comm-central\uriloader\base\nsDocLoader.cpp:1333)
#03: nsDocLoader::doStopDocumentLoad (c:\mozilla-source\comm-central\uriloader\base\nsDocLoader.cpp:891)
#04: nsDocLoader::DocLoaderIsEmpty (c:\mozilla-source\comm-central\uriloader\base\nsDocLoader.cpp:716)
#05: nsDocLoader::OnStopRequest (c:\mozilla-source\comm-central\uriloader\base\nsDocLoader.cpp:615)
#06: mozilla::net::nsLoadGroup::RemoveRequest (c:\mozilla-source\comm-central\netwerk\base\nsLoadGroup.cpp:568)
#07: mozilla::dom::Document::DoUnblockOnload (c:\mozilla-source\comm-central\dom\base\Document.cpp:10739)
#08: mozilla::dom::Document::DispatchContentLoadedEvents (c:\mozilla-source\comm-central\dom\base\Document.cpp:7150)
#09: mozilla::detail::RunnableMethodImpl<mozilla::dom::Document ,void (mozilla::dom::Document::)(),1,mozilla::RunnableKind::Standard>::Run (c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\nsThreadUtils.h:1179)
#10: nsThread::ProcessNextEvent (c:\mozilla-source\comm-central\xpcom\threads\nsThread.cpp:1213)
#11: XPTC__InvokebyIndex[c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\bin\xul.dll +0x84c0492]
#12: CallMethodHelper::Call (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNative.cpp:1195)
#13: XPCWrappedNative::CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNative.cpp:1157)
#14: XPC_WN_CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedNativeJSOps.cpp:943)
#15: ??? (???:???)

And we crash here:
https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/editor/libeditor/HTMLEditor.h#499

What has that suddenly changed? And how does this relate to the error printed on the console:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\newmailaccount\test-newmailaccount.js | test-newmailaccount.js::test_window_open_link_opening_behaviour
  EXCEPTION: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host]
    at: nonesuch line 963
       test_window_open_link_opening_behaviour/< test-newmailaccount.js:963 5
       isLoadedChecker test-window-helpers.js:740 12
       waitFor utils.jsm:380 44
       _wait_for_generic_load test-window-helpers.js:744 11
       wait_for_browser_load test-window-helpers.js:694 10
       wait_for_content_tab_load test-content-tab-helpers.js:260 6
       open_content_tab_with_click test-content-tab-helpers.js:212 28
       test_window_open_link_opening_behaviour test-newmailaccount.js:962 30

Mystery. The nsIURI.host comes from the callback here:
https://searchfox.org/comm-central/rev/a2f5fb50e09945b629efa137df65814f3e648063/mail/test/mozmill/newmailaccount/test-newmailaccount.js#962

Flags: needinfo?(masayuki)

Just disabling test_window_open_link_opening_behaviour() makes the whole thing pass.

At the line the test fails (test_window_open_link_opening_behaviour/< test-newmailaccount.js:964) the code is:
return aURL.host == "localhost" && aURL.pathQueryRef == "/target.html";

Before running that like the aURL is "about:blank" and calling .host on that produces the error (it can be tested in Error console with Services.io.newURI("about:blank").host).

Is there something in the m-c merge log resembling a change in this area?

Services.io.newURI("about:blank").host fails in TB 68, so what are we looking for?

Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/7268611e9df3 disable failing test-newmailaccount.js::test_window_open_link_opening_behaviour. rs=bustage-fix

(In reply to :aceman from comment #4)

Before running that line the aURL is "about:blank" and calling .host on that produces the error (it can be tested in Error console with Services.io.newURI("about:blank").host).

I reverted m-c 2 days back and really the aURL is now http://localhost:43336/target.html at that point and the test passes.

Brilliant :-) - Now find the regression and fix it ;-)

I once had to bisect something like this with try runs. If you do this, take care, bug 1561015 needed changes in C-C, so you need to back those out when trying without the changesets of that bug.

Remember that you can edit .gecko_rev.yml to pin to certain M-C versions. You can find it with ~20 minutes of work spread over the entire afternoon/evening if you do something else in the meantime. Please paste the try runs here.

Her's the first one:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3b864558dd6e87f8ea5685950909e0f8288ac3a3 with M-C at 156072a55a9ad15c70670bb9d2b1a6ad0d569066. That splits the regression interval in half. I suspect some side effect of bug 1561015.
Ignore https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=33ffff5c25bbdaf02b7718a4e835fecaa9db9654, that's M-C at d1fe64b4ba20b5b30863512dfcdd8b5ef3f4eb61 and won't compile since I didn't back out 1571194 despite my own advice.

Gotta go, back tonight.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(masayuki)
Flags: needinfo?(acelists)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=799c4fbd63bd9e529628454ddcff1cb0dfcb0a5c with M-C at d1fe64b4ba20b5b30863512dfcdd8b5ef3f4eb61, so before bug 1561015 with the additional C-C backout.

OK, worked at M-C d1fe64b4ba20b5b30863512dfcdd8b5ef3f4eb61, doesn't work at M-C 156072a55a9ad15c70670bb9d2b1a6ad0d569066, so it's bug 1561015. Before we ask Boris or Kris, I'd like to work out what's going on in our test. Do we get the wrong windows with the wrong URL now? Can you take a look, Aceman?

Flags: needinfo?(acelists)
Regressed by: 1561015

Changing the code here
https://searchfox.org/comm-central/rev/a2f5fb50e09945b629efa137df65814f3e648063/mail/test/mozmill/newmailaccount/test-newmailaccount.js#962
to

  open_content_tab_with_click(newTabLink, function(aURL) {
    dump(`=== got ${aURL.spec}, will set to http://localhost:43336/target.html\n`);
    aURL = Services.io.newURI("http://localhost:43336/target.html");
    return aURL.host == "localhost" && aURL.pathQueryRef == "/target.html";
  });

makes the test pass, debug output is: "got about:blank, will set to http://localhost:43336/target.html"

Sadly it still crashes later with
[7172, Main Thread] WARNING: HTMLEditRules::BeforeEdit() failed to handle something: 'NS_SUCCEEDED(rv)', file c:/mozilla-source/comm-central/editor/libeditor/HTMLEditor.cpp, line 3754
Assertion failure: !aComposerCommandsUpdater || !mComposerCommandsUpdater || aComposerCommandsUpdater == mComposerCommandsUpdater, at c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/HTMLEditor.h:500
#01: nsEditingSession::OnStateChange (c:\mozilla-source\comm-central\editor\composer\nsEditingSession.cpp:684)
as already pasted into comment #1

Looking at the debug output some more, I also saw:
[7172, Main Thread] ###!!! ASSERTION: Why is there a document channel?: '!chan', file c:/mozilla-source/comm-central/toolkit/components/windowwatcher/nsWindowWatcher.cpp, line 2047
#01: nsWindowWatcher::OpenWindow2 (c:\mozilla-source\comm-central\toolkit\components\windowwatcher\nsWindowWatcher.cpp:374)
#02: nsGlobalWindowOuter::OpenInternal (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowOuter.cpp:7281)
#03: nsGlobalWindowOuter::OpenOuter (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowOuter.cpp:5738)
#04: nsGlobalWindowInner::Open (c:\mozilla-source\comm-central\dom\base\nsGlobalWindowInner.cpp:3758)
#05: mozilla::dom::Window_Binding::open (c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dom\bindings\WindowBinding.cpp:2637)
#06: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (c:\mozilla-source\comm-central\dom\bindings\BindingUtils.cpp:3165)
#07: CallJSNative (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:448)

Watching the test go past locally, it first opens the "new provider" panel, then the fake providers registration page
https://searchfox.org/comm-central/source/mail/test/mozmill/newmailaccount/html/registration.html

Maybe what goes wrong is that clicking to open target.html here:
https://searchfox.org/comm-central/rev/a2f5fb50e09945b629efa137df65814f3e648063/mail/test/mozmill/newmailaccount/html/registration.html#20
doesn't work, although I see a blank tab opening, but not:
https://searchfox.org/comm-central/source/mail/test/mozmill/newmailaccount/html/target.html

So the issue appears to be: We open registration.html, click into the link that's meant to open target.html in a tab, but instead "about:blank" opens and it's downhill from there.

Also, and in the end, a compose window opens and that crashes the editor. I have no idea what triggers the compose window. May a follow-on issue?

I don't have a good idea how to look into this further. Somehow bug 1561015 changed the way windows are opened the the result is returned.

Since no one else answered so far, let's ask Boris, although I feel, well, how to say, that it's a bit inappropriate, since there are so many application details. But let's see whether he can ask some questions that will clarify the issue for him and us.

Flags: needinfo?(bzbarsky)

I tried openContentTab("file://C/mozilla-source/comm-central/mail/test/mozmill/newmailaccount/html/registration.html", "tab"); to load the registration page to try clicking the link, but it shows up as blank, even in TB 68.

Works OK when just opening file:///C:/mozilla-source/comm-central/comm/mail/test/mozmill/newmailaccount/html/registration.html in FF. Clicking on the "Should open in a new content tab." text does open a new content tab with content, even in a FF Nightly of today. So something strange is happening in TB here.

Bug 1561015 changed the return types of nsIWindowProvider.provideWindow, nsIBrowserDOMWindow.createContentWindow, nsIBrowserDOMWindow.createContentWindowInFrame, and nsIBrowserDOMWindow.openURI.

It looks like Thunderbird does not implement nsIWindowProvider directly. It does, however, implement nsIBrowserDOMWindow in mail/base/content/mailWindow.js; see the nsBrowserAccess bit. The code in getContentWindowOrOpenURI there needs to be updated to return the browsing context, not the window. I don't know whether that will fix the problem you're running into here, but it's got at least a chance of doing that. And it's a change that's clearly needed anyway.

Flags: needinfo?(bzbarsky)

Thanks, Boris, how funny the comment even mentions the test:

  // The following function may be called during account creation, it is called by
  // the Mozmill test test-newmailaccount.js::test_window_open_link_opening_behaviour.
  createContentWindow(aURI, aOpener, aWhere, aFlags, aTriggeringPrincipal = null) {
    return this.getContentWindowOrOpenURI(null, aOpener, aWhere, aFlags,
                                          aTriggeringPrincipal);
  },
Attached patch 1571203-browsing-context.patch (obsolete) — — Splinter Review

This fixes the test:

-    return newWindow;
+    return BrowsingContext.getFromWindow(newWindow);

This is a total copy/paste job, I wasn't able to work out where BrowsingContext comes from. But it works.

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(geoff)
Flags: needinfo?(acelists)
Attachment #9083100 - Flags: review?(bzbarsky)
Comment on attachment 9083100 [details] [diff] [review] 1571203-browsing-context.patch `BrowsingContext` and this method comes from https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/dom/chrome-webidl/BrowsingContext.webidl#9,12 so as long as you are running in window scope this will work. It might be safer to do: ``` return newWindow && BrowsingContext.getFromWindow(newWindow); ``` to avoid exceptions if/when newWindow is null.
Attachment #9083100 - Flags: review?(bzbarsky) → review+

Thanks, you think it can be null given:
https://searchfox.org/comm-central/rev/b781990cc914c195aa2b1fdfb78dc2fe470b8c04/mail/base/content/mailWindow.js#717
newWindow = newTab.browser.docShell.domWindow;

Keywords: leave-open

Boris suggested something neater on IRC, so let's go with that.

Attachment #9083100 - Attachment is obsolete: true
Attachment #9083127 - Flags: review+
Target Milestone: --- → Thunderbird 70.0

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f9417be34348
Port bug 1561015: return browsing context, not the window, from nsBrowserAccess.getContentWindowOrOpenURI(). r=bz

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Keywords: regression
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: