1362461 - TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Various test failures in test-about-support.js:

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_display_about_support
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_accounts_in_order
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_modified_pref_on_whitelist
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_modified_pref_not_on_whitelist
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_modified_pref_on_blacklist
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_private_data
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_copy_to_clipboard_public
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_copy_to_clipboard_private
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_send_via_email_public
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_send_via_email_private

First seen: Fri May 5, 2017, 15:47:11

M-C last good: 0b255199db9d6a6f189b89b7906f99155b
M-C first bad: 9348b76977e833f108cf77dff75b0fab88

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b255199db9d6a6f189b89b7906f99155b&tochange=9348b76977e833f108cf77dff75b0fab88

Log says:
https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1493992046/comm-central_win7_ix_test-mozmill-bm112-tests1-windows-build27.txt.gz

INFO -  SUMMARY-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-tabs\test-about-support.js | test-about-support.js::test_display_about_support
INFO -    EXCEPTION: Timeout waiting for content page to load. Current URL is: chrome://messenger/content/about-support/aboutSupport.xhtml
INFO -      at: test-folder-display-helpers.js line 108
INFO -         do_throw test-folder-display-helpers.js:108 13
INFO -         mark_failure logHelper.js:649 3
INFO -         _wait_for_generic_load test-window-helpers.js:749 7
INFO -         wait_for_browser_load test-window-helpers.js:695 10
INFO -         wait_for_content_tab_load test-content-tab-helpers.js:262 3
INFO -         open_content_tab_with_click test-content-tab-helpers.js:214 3
INFO -         open_about_support test-about-support.js:64 13
INFO -         test_display_about_support test-about-support.js:111 13
INFO -         Runner.prototype.wrapper frame.js:585 9
INFO -         Runner.prototype._runTestModule frame.js:655 9
INFO -         Runner.prototype.runTestModule frame.js:701 3
INFO -         Runner.prototype.runTestDirectory frame.js:525 7
INFO -         runTestDirectory frame.js:707 3
INFO -         Bridge.prototype._execFunction server.js:179 10
INFO -         Bridge.prototype.execFunction server.js:183 16
INFO -         Session.prototype.receive server.js:283 3
INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3

Comment 1

[Jorg K (CEST = GMT+2)]

Fails here:
function open_about_support() {
  let tab = open_content_tab_with_click(mc.menus.helpMenu.aboutsupport_open, <=== Line 64.
                                        "about:support");

Comment 2

[:aceman]

The tabs <browser>.currentURI doen't return about:support anymore, but the real URI of "chrome://messenger/content/about-support/aboutSupport.xhtml". We test the URI to see if the right content tab has opened. Thus now the test is aborted (ehen though the right tab has opened when observed visually).

Can you spot a m-c change about this recently?

I think test-about-downloads.js has the same problem:
SUMMARY-UNEXPECTED-FAIL | test-about-downloads.js | test-about-downloads.js::setupModule
INFO -    EXCEPTION: Timeout waiting for content page to load. Current URL is: chrome://messenger/content/downloads/aboutDownloads.xul

Comment 3

[Frank-Rainer Grahl (:frg)]

Bug 1319111 is in this area but I don't see a direct connection.

Comment 4

[Jorg K (CEST = GMT+2)]

Why don't we ask? Boris, is this connected to bug 1319111? We get test failures, see comment #2.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1362461-about-fix.patch

Aceman, thanks for your help, this fixes the tests.

Comment 6

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/35cd2a0110f30b655ef10b5d2fa45494bd310f15

Landed this for now as a bustage-fix. If it turns out that we should do it differently, I'll back this out.

Comment 7

[:aceman]

I could have done these changes, but we wanted to ask Boris if they are really needed and why only these pages did get the URL change and not others (e.g. about:addons).

Comment 8

[Jorg K (CEST = GMT+2)]

I know. We can back it out if it wasn't right.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> The tabs <browser>.currentURI doen't return about:support anymore, but the real URI of "chrome://messenger/content/about-support/aboutSupport.xhtml".

That looks wrong to me, and definitely like a regression from bug 1319111.

The right fix is to fix the various comm-central protocol handlers to set resultPrincipalURI on the loadinfo as needed.  See the patches in bug 1319111.  You should be able to find most relevant places by searching for where you set originalURI but not LOAD_REPLACE in protocol handlers.

Most immediately, the protocol handler in mail/components/aboutRedirector.js is what affects "about:support", but you should audit all your protocol handlers.

Comment 10

[Honza Bambas (:mayhemer)]

Thanks Boris.  Yes, that's exactly that.

Comment 11

[Richard Marti (:Paenglab)]

Attachment: Bug1362461.patch

This fixes for me the about:support and about:preferences URIs. I don't know where to test the calendar and chat changes but they are completely similar to the aboutRedirector.js change.

I should have catched all occurrences in our tree. https://dxr.mozilla.org/comm-central/search?q=originalURI+-path%3Aobj-x86

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1362461-result-principal-uri.patch

OK, I'll back out the incorrect fix and land this instead. Direct port of this line:
https://hg.mozilla.org/mozilla-central/rev/f9abb9c83452#l13.12

We'll review our protocols separately as Boris suggested.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8865508 [details] [diff] [review]
1362461-result-principal-uri.patch

Richard was quicker ;-(

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8865507 [details] [diff] [review]
Bug1362461.patch

Thanks, I'll take the mail/ part from this patch.

I don't have review rights in chat/ and calendar/ and I don't think the patch is correct:

  newChannel: function SPH_newChannel(aURI) {
    return this.newChannel2(aURI, null);
  },
  newChannel2: function SPH_newChannel2(aURI, aLoadInfo) {
    let smile = aURI.spec.replace(kSmileRegexp, "");
    let uri = Services.io.newURI(getSmileRealURI(smile));
    let channel = Services.io.newChannelFromURIWithLoadInfo(uri, aLoadInfo);
    channel.originalURI = aURI;
    return channel;
  },

You call newChannel2 with aLoadInfo==null, and I don't think that
null.resultPrincipalURI = aURI;
will work :-(

Comment 15

[Richard Marti (:Paenglab)]

Attachment: Bug1362461.patch

Mail only patch.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> You call newChannel2 with aLoadInfo==null

Only if someone calls newChannel.  They're really not supposed to be doing that; everyone should be using newChannel2.

Put another way, the patches in bug 1319111 more or less break newChannel entirely for a bunch of protocols: it starts throwing exceptions when called.

Comment 17

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/35cd2a0110f30b655ef10b5d2fa45494bd310f15 (incorrect fix, comment #6)
https://hg.mozilla.org/comm-central/rev/f095a9b347c19cb2e82e6f9e2eefe943dd86db85 (backout of incorrect fix)
https://hg.mozilla.org/comm-central/rev/3ebe8487526dbc4542849fd81c937125dc5d899d (correct fix, mail only)

So as far as tests are concerned, we're good.

We should look at the calendar/ and chat/ protocol handlers that Richard found in attachment 8865507 [details] [diff] [review] and also see whether there are C++ protocol handlers.

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16)
> Only if someone calls newChannel.  They're really not supposed to be doing
> that; everyone should be using newChannel2.
Hmm, the mail/ fix *is* in newChannel():
https://hg.mozilla.org/comm-central/rev/3ebe8487526dbc4542849fd81c937125dc5d899d#l1.4

I'm really a bit confused what needs to be done here:

Audit all newChannel(2) (JS) and NewChannel(2) (C++) functions and those which set originalURI should also set loadInfo.resultPrincipalURI? BTW, LOAD_REPLACE is not used in C-C at all.

As far as I can see, that's only in chat/ and calendar/. No calls to SetOriginalURI() in C++ code, but IMAP, "message" and NNTP protocols provide those methods:
https://dxr.mozilla.org/comm-central/search?q=SetOriginalURI&redirect=false

So perhaps the two hunks from Richard's original patch is all we need, maybe like so:
if (aLoadInfo)
  aLoadInfo.resultPrincipalURI = aURI;
not to break newChannel() callers.

And then we should clarify why the about: redirector doesn't have/need a newChannel2() function. Boris can you please give us further hints.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Hmm, the mail/ fix *is* in newChannel():

That's not nsIProtocolHandler's newChannel.  It's nsIAboutModule's newChannel.  Not the same thing at all; it's called from both newChannel and newChannel2 in the actual about: protocol handler.

> So perhaps the two hunks from Richard's original patch is all we need, maybe like so

If you do it that way, then consumers that don't pass a loadinfo will end up with the wrong final document URI and possibly the wrong principal for the resulting document.  Whether that's OK in these cases very strongly depends on what the originalURI and channel URI are for the protocol handlers in question.  For example, if they're the same then there is no need to worry about this at all.

Just so we're clear what the change was, before bug 1319111 the document created from a channel would get the originalURI as its URI unless LOAD_REPLACE was set, in which case it would get the URI.

After bug 1319111 the document gets the loadinfo's resultPrincipalURI if set, otherwise the channel's URI.  Since comm-central doesn't use LOAD_REPLACE anywhere, the behavior changed for any place that sets originalURI to a value different from URI.

Comment 20

[Richard Marti (:Paenglab)]

Filed bug 1363113 for chat and bug 1363115 for calendar.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8865517 [details] [diff] [review]
Bug1362461.patch

Thanks, already landed.

Comment 22

[Jorg K (CEST = GMT+2)]

OK, since we have bugs for the other cases, we're done here.

Comment 23

[Ian Neal]

Should this be backed out since the fix to the ported bug 1319111 has been backed out?

Comment 24

[Jorg K (CEST = GMT+2)]

Bug 1319111 has been backed out(??!!). Why didn't I hear about that? I'm following this bug which blocks bug 1319111, so I should get a notification. Hmm.

Backout:
https://hg.mozilla.org/comm-central/rev/a4943acf76146f25dc5d2adb4826767ea3e47ffa

Thanks, Ian, I've backed it out since after reading bug 1319111 I got the impression that the |loadInfo.resultPrincipalURI = aURI;| requirement might not return. Besides, I needed something to land after 200+ changesets were merged to M-C.

Comment 25

[avada]

(In reply to Jorg K (GMT+2) from comment #24)
> Bug 1319111 has been backed out(??!!). Why didn't I hear about that? I'm
> following this bug which blocks bug 1319111, so I should get a notification.
> Hmm.

The backout wasn't actually performed yet. And the bug didn't change state.

Comment 26

[Jorg K (CEST = GMT+2)]

(In reply to avada from comment #25)
> The backout wasn't actually performed yet.
Right. Too bad, the C-C backout was performed though. Sigh.

> And the bug didn't change state.
Well, it did: Status: RESOLVED → REOPENED and REOPENED → ASSIGNED. Usually M-C policy is to only reopen if there was a backout, so that's why I didn't look so closely.

Comment 27

[Ian Neal]

(In reply to Jorg K (GMT+2) from comment #26)
> (In reply to avada from comment #25)
> > The backout wasn't actually performed yet.
> Right. Too bad, the C-C backout was performed though. Sigh.
> 
> > And the bug didn't change state.
> Well, it did: Status: RESOLVED → REOPENED and REOPENED → ASSIGNED. Usually
> M-C policy is to only reopen if there was a backout, so that's why I didn't
> look so closely.

Sorry, I also assumed that the change of state meant it had been backed out, my bad!

Comment 28

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/fa1bde14488c510094755090e002e9e2ec753f6d

We'll re-open as required.

Comment 29

[Jorg K (CEST = GMT+2)]

Bug 1319111 really got backed out now and the approach will not return. When M-I merges to M-C, I'll back this out again.

Comment 30

[Jorg K (CEST = GMT+2)]

Backout (take 2):
https://hg.mozilla.org/comm-central/rev/fcf3534bdd64fcf673c9a230a660468680da4de6