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

RESOLVED FIXED in Thunderbird 64.0

Status

defect
--
blocker
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 64.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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
In my local compile the message pane is empty and opening a messages gives an empty tab :-( - Not so good for a mail client.
Severity: normal → blocker
Summary: Total MozMill failure on 2018-09-14 → Total MozMill failure on 2018-09-14 - No messages displayed, neither in preview pane nor when opened in tab
I suspect it's this:
416aff73b2ee Jan de Mooij - Bug 1487032 - Store origin/site info in CompartmentPrivate. r=bholley
Will confirm via local backout.
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?
Blocks: 1487032
Flags: needinfo?(jdemooij)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(jdemooij)
Flags: needinfo?(bobbyholley)
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.
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #9009252 - Flags: review?(bobbyholley)
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.
Attachment #9009252 - Flags: review?(bobbyholley)
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.
Attachment #9009252 - Attachment is obsolete: true
Attachment #9009259 - Flags: review?(acelists)
Attachment #9009259 - Flags: feedback?(bobbyholley)
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!
Attachment #9009259 - Flags: feedback?(bobbyholley)
Needed some test adjustments.
Attachment #9009259 - Attachment is obsolete: true
Attachment #9009259 - Flags: review?(acelists)
Attachment #9009265 - Flags: review?(acelists)
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.
Attachment #9009265 - Flags: review?(acelists) → review+
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!
Attachment #9009265 - Flags: feedback?(bzbarsky)
OK, I took the URI_STD out since NNTP, SMTP and POP3 don't have relative and "std" together.
Attachment #9009265 - Attachment is obsolete: true
Attachment #9009265 - Flags: feedback?(bzbarsky)
Attachment #9009277 - Flags: review+
Attachment #9009277 - Flags: feedback?(bzbarsky)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b5438a85767a
Add URI_NORELATIVE to mailbox: and imap: URLs to avoid TLD base lookup. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Version: 63 → Trunk
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.
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.
Flags: needinfo?(jdemooij)
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(jorgk)
Attachment #9009277 - Flags: feedback?(bzbarsky)
->Jan
Flags: needinfo?(bobbyholley)
(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://tbtest@example.com@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://jorgk%40jorgk%2Ecom@mail.your-server.de: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.
Flags: needinfo?(jorgk)
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?)
(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?
> 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...
(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.
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.