Closed Bug 1362461 Opened 3 years ago Closed 3 years ago

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

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: jorgk-bmo, Assigned: Paenglab)

References

Details

(Whiteboard: [Thunderbird-testfailure: M all])

Attachments

(2 files, 2 obsolete files)

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
Fails here:
function open_about_support() {
  let tab = open_content_tab_with_click(mc.menus.helpMenu.aboutsupport_open, <=== Line 64.
                                        "about:support");
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
Bug 1319111 is in this area but I don't see a direct connection.
Why don't we ask? Boris, is this connected to bug 1319111? We get test failures, see comment #2.
Flags: needinfo?(bzbarsky)
Aceman, thanks for your help, this fixes the tests.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8865248 - Flags: review?(acelists)
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.
Target Milestone: --- → Thunderbird 55.0
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).
I know. We can back it out if it wasn't right.
> 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.
Blocks: 1319111
Flags: needinfo?(bzbarsky)
Thanks Boris.  Yes, that's exactly that.
Attachment #8865248 - Flags: review?(acelists) → review-
Attached patch Bug1362461.patch (obsolete) — Splinter Review
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
Attachment #8865507 - Flags: review?(jorgk)
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 on attachment 8865508 [details] [diff] [review]
1362461-result-principal-uri.patch

Richard was quicker ;-(
Attachment #8865508 - Attachment is obsolete: true
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 :-(
Attachment #8865507 - Flags: review?(jorgk)
Attached patch Bug1362461.patchSplinter Review
Mail only patch.
Attachment #8865507 - Attachment is obsolete: true
Attachment #8865517 - Flags: review?(jorgk)
> 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.
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.
(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.
> 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.
Filed bug 1363113 for chat and bug 1363115 for calendar.
Comment on attachment 8865517 [details] [diff] [review]
Bug1362461.patch

Thanks, already landed.
Attachment #8865517 - Flags: review?(jorgk) → review+
OK, since we have bugs for the other cases, we're done here.
Assignee: jorgk → richard.marti
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Should this be backed out since the fix to the ported bug 1319111 has been backed out?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
(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.
(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!
https://hg.mozilla.org/comm-central/rev/fa1bde14488c510094755090e002e9e2ec753f6d

We'll re-open as required.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
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.
Duplicate of this bug: 1365367
You need to log in before you can comment on or make changes to this bug.