1491406 - Total MozMill failure on 2018-09-14 - No messages displayed, neither in preview pane nor when opened in tab

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Looks like about *all* Mozmill tests fail. With only a few changesets in the range, I didn't even want to do a push. Now I'm glad I did.

M-C last good: 28baa3df5f0f530faee1900f43c463f6b7
M-C first bad: 5274e2dda972320ab72be12d0b9064d58e

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=28baa3df5f0f530faee1900f43c463f6b7&tochange=5274e2dda972320ab72be12d0b9064d58e

Comment 1

[Jorg K (CEST = GMT+2)]

In my local compile the message pane is empty and opening a messages gives an empty tab :-( - Not so good for a mail client.

Comment 2

[Jorg K (CEST = GMT+2)]

I suspect it's this:
416aff73b2ee Jan de Mooij - Bug 1487032 - Store origin/site info in CompartmentPrivate. r=bholley
Will confirm via local backout.

Comment 3

[Jorg K (CEST = GMT+2)]

Jan and Bobby: Bug 1487032 completely obliterated TB. No messages displayed neither in the message pane nor in a tab.

What can we do to restore that functionality?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jorg K (GMT+2) from comment #3)
> Jan and Bobby: Bug 1487032 completely obliterated TB. No messages displayed
> neither in the message pane nor in a tab.
> 
> What can we do to restore that functionality?

There's nothing in that patch that I would expect to be a problem for TB. You should debug it.

Comment 5

[Jorg K (CEST = GMT+2)]

Hmm, you made changes to how documents with code-based principals work. We display all out messages with code-based principals, so I assume that's where the problem is. Also, our messages don't have site data.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1491406-ignore-empty-base.patch (v1)

This does the trick. Most of our URLs set this flag, see:
https://searchfox.org/comm-central/search?q=URI_NORELATIVE&case=false&regexp=false&path=mailnews%2F
https://searchfox.org/comm-central/search?q=URI_NORELATIVE&case=false&regexp=false&path=calendar%2F

No idea why it was missing from the mailbox: URL.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9da9156deb5f6eba8473d83f084f7a6bc9bef3db

Oh, yes, debugging. Well, debugging showed that |return thirdPartyUtil->GetBaseDomain(aCodebase, aBaseDomain);| was executed at the end of GetBaseDomainHelper() and failed.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 9009252 [details] [diff] [review]
1491406-ignore-empty-base.patch (v1)

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

It sounds plausible that this fix makes sense, but I don't know enough about the TB model to review this.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1491406-mailbox-URI_NORELATIVE.patch (v2)

IMAP needed the same treatment. Now IMAP message display again.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c45f8abeb17641ceb323faa15cec9f0c87d2b525

I guess the test results will be the same since there are no tests that display messages from IMAP folders. Well, I could be wrong.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9009259 [details] [diff] [review]
1491406-mailbox-URI_NORELATIVE.patch (v2)

Sorry Bobby, I didn't see you previous comment. Thanks for looking!

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1491406-mailbox-URI_NORELATIVE.patch (v3)

Needed some test adjustments.

Comment 11

[:aceman]

Comment on attachment 9009265 [details] [diff] [review]
1491406-mailbox-URI_NORELATIVE.patch (v3)

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

Seems to work, thanks for finding out!

::: mailnews/imap/src/nsImapService.cpp
@@ +2445,5 @@
>  
>  NS_IMETHODIMP nsImapService::GetProtocolFlags(uint32_t *result)
>  {
> +  *result = URI_NORELATIVE |
> +    URI_STD | URI_FORBIDS_AUTOMATIC_DOCUMENT_REPLACEMENT |

Maybe URI_STD should be kept as the first one, if at all needed. We would need advice from m-c whether it is proper style to have URI_STD in the flags or not (when URI_RELATIVE is too). It's value is 0 right now, but could change.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 9009265 [details] [diff] [review]
1491406-mailbox-URI_NORELATIVE.patch (v3)

I'll ask Boris.

Hi Boris, can you please comment on this patch. Since message display didn't work, I had to land it. I'm happy to follow up. We don't quite understand why NNTP, SMTP and POP3 always had the flag, whereas mailbox: and IMAP didn't.

Also, should I remove URI_STD?

Thanks for your help!

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1491406-mailbox-URI_NORELATIVE.patch (v4)

OK, I took the URI_STD out since NNTP, SMTP and POP3 don't have relative and "std" together.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b5438a85767a
Add URI_NORELATIVE to mailbox: and imap: URLs to avoid TLD base lookup. r=aceman

Comment 15

[Jorg K (CEST = GMT+2)]

Looking at this a little further, other protocol handlers apart from SMTP, POP3 and NNTP already had URI_NORELATIVE:

calendar/lightning/components/calItipProtocolHandler.js
55 protocolFlags: Ci.nsIProtocolHandler.URI_NORELATIVE | ...

chat/components/src/smileProtocolHandler.js
16 protocolFlags: Ci.nsIProtocolHandler.URI_NORELATIVE

ldap/xpcom/src/nsLDAPProtocolHandler.js
17 protocolFlags: nsIProtocolHandler.URI_NORELATIVE 

The only one that's missing it now is the address book URL:
https://searchfox.org/comm-central/rev/eb9de130cbf328ff82f3939e2c2ffc6fb04a990a/mailnews/addrbook/src/nsAddbookProtocolHandler.cpp#53

I wonder whether that matters.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So there was definitely a semantic change in ContentPrincipal::GetSiteOrigin with bug 1487032.

Before that patch, a failure of thirdPartyUtil->GetBaseDomain() for the given URL led to fallback to GetOrigin().  After the patch, it causes GetSiteOrigin() to fail.  Was this a purposeful change?

That failure then gets propagated to ContentPrincipal::GetSiteIdentifier.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9009277 [details] [diff] [review]
1491406-mailbox-URI_NORELATIVE.patch (v4)

Remind me again what the structure of IMAP and mailbox URLs is?  How does that compare to SMTP and POP URLs?

Comment 18

[Bobby Holley (:bholley)]

->Jan

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)
> Remind me again what the structure of IMAP and mailbox URLs is?  How does
> that compare to SMTP and POP URLs?
OK, here goes:

mailbox://[e-mail]@server.de/Inbox?number=1
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/5gfhfw7f.POP%20tbtest/Mail/server.de/Inbox?number=1
and there's also
mailbox://nobody@Local%20Folders/xxx
mailbox:///C:/Users/jorgk/AppData/Local/Temp/nsmail-13.eml&number=0 for single messages.
We convert the first type (user@server) into the second one when needed.

imap://gene@localhost:130/fetch%3EUID%3E.INBOX.555%3E32?part=1.1.3
imap://[e-mail]:143/fetch%3EUID%3E.INBOX.No%20local%20storage%3E2|

We see mailbox and IMAP URLs all the time since they show up when we view messages.

SMTP and POP3 I haven't seen, I'd have to add some debug to see them. If DXR doesn't lie, they are something like smtp://user@localhost/, so not much different from the IMAP ones. Yes,
https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/compose/src/nsSmtpServer.cpp#569
confirms that.

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/local/src/nsPop3Service.cpp#127
shows:
  char * urlSpec = (downloadNewMail)
    ? PR_smprintf("pop3://%s@%s:%d", escapedUsername.get(), popHost.get(), popPort)
    : ...

So the same deal really. As I said, personally I don't understand why SMTP and POP had URI_NORELATIVE and mailbox and IMAP didn't since to me the former look a whole lot more like a standard URL.

If that doesn't answer the question, I can provide more details.

Comment 20

[Boris Zbarsky [:bzbarsky]]

That helps, thanks.  So those definitely look like things that should handle relative URLs... the question is whether they ever need to.  Are relative URLs ever used with those?

(Amusingly, nothing in necko really checks URI_NORELATIVE, so it looks like it's possible to set that _and_ still do relative URLs?)

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #20)
> Are relative URLs ever used with those?
I'm not aware of any relative URLs used for those protocols.

Is that related to Resolve()?
https://searchfox.org/comm-central/search?q=%3A%3AResolve&case=false&regexp=false&path=mailnews%2F
Mailnews URLs, the base class of all those others, provides that:
https://searchfox.org/comm-central/rev/c8bcd4d2b62e5c4361e23f17bb1fc17914d5fc49/mailnews/base/util/nsMsgMailNewsUrl.cpp#628
Maybe then #anchor URLs can be relative?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Is that related to Resolve()?

Or just passing a base URL to NS_NewURI and a URL string that doesn't start with "something:".

> Maybe then #anchor URLs can be relative?

Yes.  But that isn't affected by URI_NORELATIVE, I guess, which is good...

Comment 23

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #16)
> Before that patch, a failure of thirdPartyUtil->GetBaseDomain() for the
> given URL led to fallback to GetOrigin().  After the patch, it causes
> GetSiteOrigin() to fail.  Was this a purposeful change?

Yeah, I didn't feel good about sweeping all potential failures under the rug and just falling back to the origin no matter what failed. For instance imagine an OOM or something when we try to determine the base domain for foo.bar.facebook.com - just running with the full origin in that case instead of facebook.com felt like it could become a source of mysterious bugs.

That said, it backfired with this bug + bug 1491728, but I hope we can fix them without the unconditional fallback.