Closed Bug 1564094 Opened 5 years ago Closed 5 years ago

"View > Text Encoding" not working on when message is stored in IMAP folder (since TB 61)

Categories

(Thunderbird :: General, defect)

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

Tracking

(thunderbird68+ fixed, thunderbird69 fixed, thunderbird70 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 70.0
Tracking Flags:
Tracking Status
thunderbird68 + fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

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

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

1564094-OriginalURI.patch
1.34 KB, patch
Details | Diff | Splinter Review
debug-imap-source-load.patch
7.32 KB, patch
Details | Diff | Splinter Review
1564094-history.patch (v2)
828 bytes, patch
bzbarsky
: review+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review
1564094-button-follow-up.patch (v2)
1.47 KB, patch
aceman
: review+
Details | Diff | Splinter Review

STR:
View a message and press Ctrl+U to see the message source.
On the view menu, select Text Encoding and select any other text encoding.

Result: Doesn't work, and I see this is in the error console:
Security Error: Content at view-source:imap://jorgk%40jorgk%2Ecom@mail.your-server.de:143/fetch%3EUID%3E.INBOX.spambucket%3E14402 may not load or link to imap://jorgk%40jorgk%2Ecom@mail.your-server.de:143/fetch%3EUID%3E.INBOX.spambucket%3E14402.
NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.reload] browser-custom-element.js:953
reloadWithFlags chrome://global/content/elements/browser-custom-element.js:953
onSetCharacterSet chrome://messenger/content/viewSource.js:440
oncommand chrome://messenger/content/viewSource.xul:1

Also not working in TB 68 beta, but the line numbers are a bit different there:
Security Error: Content at view-source:imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E13 may not load or link to imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E13.
NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.reload] browser-custom-element.js:816
reloadWithFlags chrome://global/content/elements/browser-custom-element.js:816
onSetCharacterSet chrome://messenger/content/viewSource.js:440
oncommand chrome://messenger/content/viewSource.xul:1

The code that fails is:
this.browser.reloadWithFlags(Ci.nsIWebNavigation.LOAD_FLAGS_CHARSET_CHANGE);

Alice, can you please find the regression for us.

And please don't forget to use an IMAP folder.

Flags: needinfo?(alice0775)

#1 text encoding doesn't work and without error message
#2 Ctrl+U gets empty view source window
#3 text encoding doesn't work and with following error message
Security Error: Content at view-source:imap://alice0775%40gmail%2Ecom@imap.googlemail.com:993/fetch%3EUID%3E/INBOX%3E29169 may not load or link to imap://imap.googlemail.com:993/fetch%3EUID%3E/INBOX%3E29169.
NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.reload] browser.xml:89

#1 regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=278d1f336d04a8bc0a6cb2181a98bee40ce6fb9f&tochange=2edea7714121b2f25cc155b0592520ce1ebfa6fa
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7543a3e938d5ec408a7d0c93dc71b40607e7d5b&tochange=bccdc684210431c233622650a91454c09f6af9eb

#2 regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=89435f04afee556e91d8f17634cd377dff5ac8d2&tochange=019e34d5f6f50f4b1e670e7a5bc8b81bfc03e644
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6862624e24d005fb4f8fb07c6800d2acef1d287e&tochange=b99844d179cacf74a5d39ad23429be91e989c331

#3 regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=3213fa235aef00d065affbe563a05a4469f2e96d&tochange=4d7274e18df2de8216378e52a605e00135c14fd7
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8063b0c54b888fe1f98774b71e1870cc2267f33f&tochange=7b40283bf1c7a2a3e6a8a5d00156a2f506ff465b

Flags: needinfo?(alice0775)

BTW, archived builds are ONLY for just 1 year, available. see https://tools.taskcluster.net/index/comm.v2.comm-central.pushdate.2018

Thanks, Alice, yes, this broke a few times in 2018. I don't quite understand the ranges. Window #1 is in mozilla60, but changing the encoding on the source of an IMAP message works in TB 60.

Window #2 is during mozilla61. It must have been fixed afterwards since Ctrl+U doesn't produce an empty window now.

So I guess we're interested in window #3, also during mozilla61. Can you please post the links to the two binaries used for the last range.

Flags: needinfo?(alice0775)

OK, I found a version of 61.0a1 (2018-05-01) and that already shows the security error:
Security Error: Content at view-source:imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E1 may not load or link to imap://mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E1.
[Show/hide message details.] NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.reload] browser.xml:89

I'll see whether I can find something in the range.

Flags: needinfo?(alice0775)

OK, let's turn back to current trunk. Maybe Boris can give us a hint here. Why would this.webNavigation.reload(aFlags); at
https://searchfox.org/mozilla-central/rev/f711552789d6308796e34170ddaabead044adc1e/toolkit/content/widgets/browser-custom-element.js#953
fail?

We call it form here:
https://searchfox.org/comm-central/rev/30fb8479949b5246fda13398de98216dcb3ad346/common/src/viewSource.js#440

Looks like M-C do it differently these days:
https://searchfox.org/mozilla-central/rev/f711552789d6308796e34170ddaabead044adc1e/browser/base/content/browser.js#7584

The error is:
Security Error: Content at view-source:imap://jorgk%40jorgk%2Ecom@mail.your-server.de:143/fetch%3EUID%3E.INBOX.spambucket%3E14402 may not load or link to imap://jorgk%40jorgk%2Ecom@mail.your-server.de:143/fetch%3EUID%3E.INBOX.spambucket%3E14402.
NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.reload] browser-custom-element.js:953

EDIT: It's puzzling since it loaded the page in the first place.

Flags: needinfo?(bzbarsky)

BTW, just trying to reload (View > Reload) gives the same error.

Summary: "View > Text Encoding" not working on TB 69 trunk when message is stored in IMAP folder → "View > Text Encoding" not working on when message is stored in IMAP folder (since TB 61)

The error message cited in comment 4 claims that the load is being done with a triggering principal of "view-source:imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E1" but the URI being loaded is "imap://mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E1" (without the "view-source:") bit. I can't speak to whether the triggering principal is correct here, but if we're reloading a source view, why is the URI being loaded not "view-source:"?

The simplest approach may be to breakpoint in nsScriptSecurityManager::ReportError and then see what the stack looks like, which will tell you where the CheckLoadURI check is coming from and maybe where the URI passed to it is coming from. Then try to trace that back to see where the non-view-source URI is appearing from?

Flags: needinfo?(bzbarsky)

Thanks, Boris. Yes, those inconsistent URIs are puzzling and even more puzzling that it works for mailbox: and news: messages/URIs but not for IMAP. I'll see what I can find.

OK, I got a stack, but we can see it from the debug output already:

[14584, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.PrincipalInfo())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.cpp, line 174
[14584, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/mozilla-source/comm-central/dom/security/nsContentSecurityManager.cpp, line 1065
[14584, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/mozilla-source/comm-central/dom/security/nsContentSecurityManager.cpp, line 933
[14584, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/mozilla-source/comm-central/comm/mailnews/imap/src/nsImapProtocol.cpp, line 9291
[14584, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303F4: file c:/mozilla-source/comm-central/docshell/base/nsDocShell.cpp, line 10645
JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 953: NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWebNavigation.reload]

xul.dll!nsScriptSecurityManager::ReportError(const char * aMessageTag, nsIURI * aSource, nsIURI * aTarget, bool) Line 1044 C++
xul.dll!nsScriptSecurityManager::CheckLoadURIFlags(nsIURI * aSourceURI, nsIURI * aTargetURI, nsIURI * aSourceBaseURI, nsIURI * aTargetBaseURI, unsigned int aFlags, bool) Line 846 C++
xul.dll!nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal * aPrincipal, nsIURI * aTargetURI, unsigned int aFlags) Line 794 C++
xul.dll!DoCheckLoadURIChecks(nsIURI * aURI, nsILoadInfo * aLoadInfo) Line 351 C++
xul.dll!nsContentSecurityManager::CheckChannel(nsIChannel * aChannel) Line 1065 C++
xul.dll!nsContentSecurityManager::doContentSecurityCheck(nsIChannel * aChannel, nsCOMPtr<nsIStreamListener> & aInAndOutListener) Line 933 C++
xul.dll!nsImapMockChannel::AsyncOpen(nsIStreamListener * aListener) Line 9285 C++
xul.dll!nsViewSourceChannel::AsyncOpen(nsIStreamListener * aListener) Line 313 C++
xul.dll!nsURILoader::OpenURI(nsIChannel * channel, unsigned int aFlags, nsIInterfaceRequestor * aWindowContext) Line 847 C++
xul.dll!nsDocShell::OpenInitializedChannel(nsIChannel * aChannel, nsIURILoader * aURILoader, unsigned int aOpenFlags) Line 10645 C++
xul.dll!nsDocShell::DoChannelLoad(nsIChannel * aChannel, nsIURILoader * aURILoader, bool) Line 10603 C++
xul.dll!nsDocShell::DoURILoad(nsDocShellLoadState * aLoadState, bool aDocShell, nsIDocShell * * aRequest, nsIRequest * *) Line 10406 C++
xul.dll!nsDocShell::InternalLoad(nsDocShellLoadState * aLoadState, nsIDocShell * * aDocShell, nsIRequest * * aRequest) Line 9673 C++
xul.dll!nsDocShell::Reload(unsigned int aReloadFlags) Line 4729 C++

And the problem is, that nsImapMockChannel::AsyncOpen() calls nsContentSecurityManager::doContentSecurityCheck() on a channel that doesn't have load info. So adding this with the patch makes it work. Looks like the wrong approach since the channel should be created with the correct load info. Ben is more familiar with that. I tried switching to a system principal here or providing the load info here, but no improvement:
https://searchfox.org/comm-central/rev/30fb8479949b5246fda13398de98216dcb3ad346/mailnews/imap/src/nsImapProtocol.cpp#797

I can't actually see where the channel is created. At that line 797? Does NS_NewChannel() not set load info on the channel?

Boris, maybe you can enlighten me on the mechanics of all that. I'll attach a working patch in the next comment.

Attachment #9076647 - Flags: review?(benc)
Attachment #9076647 - Flags: feedback?(bzbarsky)

OK, so "channel that doesn't have load info" is not what's going on, since that would crash.

As I see it, there are two fundamental bugs here:

  1. The triggering principal in the loadinfo is apparently a content principal for "view-source:imap://whatever". I'd have expected it to be a system principal for both the original opening of view-source and the reload. It's worth figuring out whether it is for one or the other and why it's not for the case(s) it's not system for.

  2. The URI being checked is the wrong URI: it's "imap://whatever", not "view-source:imap://whatever". This URI comes from the NS_GetFinalChannelURI call that nsContentSecurityManager::CheckChannel makes. That tries to get the result principal URI from the loadinfo, then tries to GetOriginalURI on the channel. Normally the original URI for the nested channel of a view-source load would be the "view-source:" URI involved, but nsImapMockChannel has broken original-uri handling (see https://searchfox.org/comm-central/rev/c32ec2d31d161a34416c315e0eeeb4b3f2c3d162/mailnews/imap/src/nsImapProtocol.cpp#8619-8631 where it ignores the SetOriginalURI call from the view-source code and then returns the "imap://whatever" URI from GetOriginalURI). Ideally, that would be fixed. It's pretty likely that fixing this will fix this specific problem without sorting out issue #1 about the triggering principal.

Note that nsMsgProtocol handles SetOriginalURI correctly (and IMAP overrides with the broken behavior), which is why view-source on mailbox:// things presumably works. On the other hand, NNTP seems to have the same behavior as IMAP, so this would be a problem there too?

While fixing #2, watch out for reintroducing the problem bug 193317 was trying to fix. :(

It might be safer to investigate what's going on with #1 than to mess with the original URI bits on these channels, though it's possible that since 2003 things have improved in terms of how mailnews uses necko such things don't fall down badly in the situation of bug 193317.

Comment on attachment 9076647 [details] [diff] [review]
1564094-mock-channel-loadinfo.patch

This seems like it would allow web content to link to imap:// URLs, which is _not_ desirable.
Attachment #9076647 - Flags: feedback?(bzbarsky) → feedback-

I didn't have time to remove the patch and cancel the requests :-(

I've added this debug in nsImapMockChannel::AsyncOpen():

  nsCOMPtr<nsIPrincipal> p;
  m_loadInfo->GetTriggeringPrincipal(getter_AddRefs(p));
  printf("=== %d\n", p->IsSystemPrincipal()?1:0);

That prints 1 for the first load and 0 for a reload or change of text encoding.

This works as predicted, I'll do some more testing tomorrow not to run into bug 193317 for IMAP. For now, I created an IMAP message with a background image and saw no problem.

Attachment #9076647 - Attachment is obsolete: true
Attachment #9076647 - Flags: review?(benc)
Attachment #9076670 - Flags: review?(bzbarsky)

That prints 1 for the first load and 0 for a reload or change of text encoding.

OK. That first part is good, but the second part is very wrong...

Could we try tracing why that's happening? The triggering principal should be getting saved in the session history entry and the reload should then use the same one as the original load. Is that nor working for some reason? Does the relevant browser have session history disabled, say?

Comment on attachment 9076670 [details] [diff] [review]
1564094-OriginalURI.patch

Can you just remove these impls and let the nsMsgProtocol impls do the work?

That said, this seems _really_ risky per that old bug I linked and probably needs an actual test plan.

"History" rings a bell somehow, but may that's from some other context. viewSource.js was forked from M-C when they removed it, see:
https://hg.mozilla.org/comm-central/log/tip/common/src/viewSource.js

As for the change being risky: As far as I can see, nsNntpMockChannel does it and the underlying protocol message protocol does it:
https://searchfox.org/comm-central/search?q=SetOriginalURI&case=false&regexp=false&path=mailnews
which I what news nsNntpMockChannel, if I looked at that correctly:
https://searchfox.org/comm-central/rev/c32ec2d31d161a34416c315e0eeeb4b3f2c3d162/mailnews/news/src/nsNntpMockChannel.cpp#127
with FORWARD_CALL being here:
https://searchfox.org/comm-central/rev/c32ec2d31d161a34416c315e0eeeb4b3f2c3d162/mailnews/news/src/nsNntpMockChannel.cpp#40
But there was also bug 1312314.

As for the "Can you just remove these impls and let the nsMsgProtocol impls do the work?". I don't think that's possible since the IMAP mock stuff doesn't inherit directly from nsMsgProtocol:
https://searchfox.org/comm-central/search?q=public+nsMsgProtocol&case=false&regexp=false&path=mailnews

Looks like a messy area. I wonder what would happen if we also "fixed" the NNTP mock the same way I suggested here for IMAP and nsMsgProtocol already does.

OK, here is the thing with some debugging:

IMAP view source, the "original URI" calls are on the mock channel, so is the async open:

=== nsImapMockChannel::SetOriginalURI view-source:imap://info%40hostalpancheta%2
Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E6
[12884, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.Principal
Info())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.c
pp, line 174
=== nsImapMockChannel::AsyncOpen, IsSystemPrincipal=1
=== nsImapMockChannel::GetOriginalURI imap://info%40hostalpancheta%2Ees@mail.hos
talpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E6
=== nsImapMockChannel::GetOriginalURI imap://info%40hostalpancheta%2Ees@mail.hos
talpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E6
=== nsImapMockChannel::GetOriginalURI imap://info%40hostalpancheta%2Ees@mail.hos
talpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E6

Reload:

=== nsImapMockChannel::SetOriginalURI view-source:imap://info%40hostalpancheta%2
Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E6
[12884, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.Principal
Info())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.c
pp, line 174
=== nsImapMockChannel::AsyncOpen, IsSystemPrincipal=0
=== nsImapMockChannel::GetOriginalURI imap://info%40hostalpancheta%2Ees@mail.hos
talpancheta.es:993/fetch%3EUID%3E.INBOX.Bel%26AOk-n%3E6
[12884, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805
303F4: file c:/mozilla-source/comm-central/dom/security/nsContentSecurityManager
.cpp, line 1065
[12884, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805
303F4: file c:/mozilla-source/comm-central/dom/security/nsContentSecurityManager
.cpp, line 933
[12884, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805
303F4: file c:/mozilla-source/comm-central/comm/mailnews/imap/src/nsImapProtocol
.cpp, line 9294
[12884, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805
303F4: file c:/mozilla-source/comm-central/docshell/base/nsDocShell.cpp, line 10
645
JavaScript error: chrome://global/content/elements/browser-custom-element.js, li
ne 953: NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIWe
bNavigation.reload]

News article view source, both calls are on the protocol:

=== nsNNTPProtocol::SetOriginalURI view-source:news://news.mozilla.org/ePqdnd6BL
6iX0b7AnZ2dnUU7-e_NnZ2d%40mozilla.org?group=mozilla.dev.apps.thunderbird&key=161
80
[12884, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.Principal
Info())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.c
pp, line 174
=== nsNNTPProtocol::AsyncOpen, IsSystemPrincipal=1
=== nsNNTPProtocol::GetOriginalURI news://news.mozilla.org:119/ePqdnd6BL6iX0b7An
Z2dnUU7-e_NnZ2d%40mozilla.org?group=mozilla.dev.apps.thunderbird&key=16180
=== nsNNTPProtocol::GetOriginalURI news://news.mozilla.org:119/ePqdnd6BL6iX0b7An
Z2dnUU7-e_NnZ2d%40mozilla.org?group=mozilla.dev.apps.thunderbird&key=16180
=== nsNNTPProtocol::GetOriginalURI news://news.mozilla.org:119/ePqdnd6BL6iX0b7An
Z2dnUU7-e_NnZ2d%40mozilla.org?group=mozilla.dev.apps.thunderbird&key=16180

Reload:

=== nsNNTPProtocol::SetOriginalURI view-source:news://news.mozilla.org/ePqdnd6BL
6iX0b7AnZ2dnUU7-e_NnZ2d%40mozilla.org?group=mozilla.dev.apps.thunderbird&key=161
80
[12884, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.Principal
Info())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.c
pp, line 174
=== nsNNTPProtocol::AsyncOpen, IsSystemPrincipal=0
=== nsNNTPProtocol::GetOriginalURI news://news.mozilla.org:119/ePqdnd6BL6iX0b7An
Z2dnUU7-e_NnZ2d%40mozilla.org?group=mozilla.dev.apps.thunderbird&key=16180
=== nsNNTPProtocol::GetOriginalURI news://news.mozilla.org:119/ePqdnd6BL6iX0b7An
Z2dnUU7-e_NnZ2d%40mozilla.org?group=mozilla.dev.apps.thunderbird&key=16180
=== nsNNTPProtocol::GetOriginalURI news://news.mozilla.org:119/ePqdnd6BL6iX0b7An
Z2dnUU7-e_NnZ2d%40mozilla.org?group=mozilla.dev.apps.thunderbird&key=16180

We note that although the principals differ and Get/SetOriginalURI are equally dummied out in nsNNTPProtocol as in nsImapMockChannel, the NNTP reload works and the IMAP doesn't. Strange, what's the difference?

For a mailbox message we get:

=== nsMsgProtocol::SetOriginalURI view-source:mailbox:///C:/Users/jorgk/AppData/
Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20M
IME%20stuff%20plain%20text?number=14
[12884, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.Principal
Info())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.c
pp, line 174
=== nsMsgProtocol::AsyncOpen, IsSystemPrincipal=1
=== nsMsgProtocol::GetOriginalURI view-source:mailbox:///C:/Users/jorgk/AppData/
Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20M
IME%20stuff%20plain%20text?number=14
=== nsMsgProtocol::GetOriginalURI view-source:mailbox:///C:/Users/jorgk/AppData/
Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20M
IME%20stuff%20plain%20text?number=14
=== nsMsgProtocol::GetOriginalURI view-source:mailbox:///C:/Users/jorgk/AppData/
Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20M
IME%20stuff%20plain%20text?number=14

Reload

=== nsMsgProtocol::SetOriginalURI view-source:mailbox:///C:/Users/jorgk/AppData/
Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20M
IME%20stuff%20plain%20text?number=14
[12884, Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.Principal
Info())', file c:/mozilla-source/comm-central/dom/clients/manager/ClientSource.c
pp, line 174
=== nsMsgProtocol::AsyncOpen, IsSystemPrincipal=0
=== nsMsgProtocol::GetOriginalURI view-source:mailbox:///C:/Users/jorgk/AppData/
Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20M
IME%20stuff%20plain%20text?number=14
=== nsMsgProtocol::GetOriginalURI view-source:mailbox:///C:/Users/jorgk/AppData/
Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20M
IME%20stuff%20plain%20text?number=14
=== nsMsgProtocol::GetOriginalURI view-source:mailbox:///C:/Users/jorgk/AppData/
Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Test%20M
IME%20stuff%20plain%20text?number=14

So this leaves us with a few choices:

  1. Figure out and fix why the principal changes in the reload call.
  2. Figure out why NNTP works, but IMAP doesn't when their code seems to be identical as far as original URI handling is concerned.
  3. Use the "risky" patch I proposed.

In case you want to see how the debug is produced.

As far as I can see, nsNntpMockChannel does it and the underlying protocol message protocol does it:

Well, modulo the (broken, imo) changes to nsNntpMockChannel made in bug 1312314, yes?

And nsNNTPProtocol does NOT do it (because of the fix for bug 193317). So the forwarding to m_protocol in nsNntpMockChannel is completely pointless: it's always a no-op on the setter and returns the URI on the getter. I guess that means bug 1312314 made the Nntp mock at least consistent in terms of how it behaved no matter whether it has m_protocol or not.

Can you please at least look up the blame for why IMAP mock is doing the no-op thing? Changing this sort of security-related invariant just because we feel like it without understanding the reasons for the code being the way it's written is generally a bad idea.

Looks like a messy area.

Well, yes, because a bunch of people spent years going "Oh, the security check is failing? Instead of figuring out why, let's just add some hacky invariant-violations to make it work in this one case", and now it's coming back to bite this code. What should have happened instead, in both bug 1312314 and bug 193317 is figuring out reasonable root causes and solving the problem correctly...

OK, here is the thing with some debugging:

None of that answers the question of why the triggering principal is non-system on reload, right?

the NNTP reload works and the IMAP doesn't. Strange, what's the difference?

You'd likely need to step through the CheckLoadURIWithPrincipal code in both cases to tell.

For a mailbox message we get:

OK, so it handles the originalURI correctly, and gets saved by that, but still has the weirdness around the triggering principal.

So this leaves us with a few choices:

Fwiw, I think we should definitely do #1, probably do #2, and ideally consider doing #3, after figuring out what problems the current broken code was trying to fix and testing the heck out of it in those cases and other, related, ones.

How exactly does Thunderbird load its source stuff? Does it go through the code at https://searchfox.org/mozilla-central/rev/da3f3eaaacb6fb344fd21ac29ace2da0e33f12d3/toolkit/components/viewsource/content/viewSource-content.js#277-289? Does it go through nsDocShell::LoadPage but not that code?

Thanks for the long answer. I'm a bit time-strapped at the moment, and it doesn't look like a quick fix anyway. #1 doesn't look so easy to do, you said it might depend on history. Any hints where to look? I can do #2 and I can make a patch for #3 fixing the news/NNTP stuff at the same time. Then we need to decide how to test this so that bug 193317 and bug 1312314 don't come back.

One question I can answer right now, the IMAP blame:
https://searchfox.org/comm-central/rev/30fb8479949b5246fda13398de98216dcb3ad346/mailnews/imap/src/nsImapProtocol.cpp#8620

bug 437643 - Build Thunderbird and SeaMonkey from comm-central, initial import of code from CVS tag HG_COMM_INITIAL_IMPORT at 2008-07-22 05:18:47 PST, imported and tagged cvs.mozilla.org modules: mozilla/directory/xpcom/ mozilla/mailnews/ mozilla/mail/ mozilla/suite/ mozilla/other-licenses/branding/thunderbird/
hg@mozilla.org <hg@mozilla.org>, Tue, 22 Jul 2008 14:21:15 +0200

:-(

Any hints where to look?

See last paragraph of comment 21.

But in general, you just want to step through the reload() call and see where it's getting its triggering principal from, no? This is not rocket science.

One question I can answer right now, the IMAP blame:

Yes, I'm aware of that. I looked it up before saying that someone needs to look up the blame. The question is where that code came from before that and whether those bits are in comm-central. There's other blame from before 2008 that is in comm-central, so seems like this information should be findable...

OK, mystery #2 is solved: news and IMAP both run through CheckLoadURIFlags, it returns OK for the former, and an error for the latter. We fail on DenyAccessIfURIHasFlags checking for URI_DANGEROUS_TO_LOAD since IMAP is dangerous and news is not:
https://searchfox.org/comm-central/search?q=URI_DANGEROUS_TO_LOAD&case=false&regexp=false&path=mailnews

Now onto mystery #1.

Mystery #1:
We don't go through https://searchfox.org/mozilla-central/rev/da3f3eaaacb6fb344fd21ac29ace2da0e33f12d3/toolkit/components/viewsource/content/viewSource-content.js#277-289, so no history is set as we already suspected.

As I said in comment #18, https://hg.mozilla.org/comm-central/log/tip/common/src/viewSource.js was forked. I'll try to work out where the loading happens.

OK, talking https://searchfox.org/comm-central/rev/6e6533927f58e5bce51970872bb9973a480460eb/common/src/viewSource.xul#307-309 out fixes this bug here.

You want to fix it this way? What about item #3. Move that to a different bug we don't have to ship in TB 68?

Attached patch 1564094-history.patch (obsolete) — — Splinter Review
Assignee: nobody → jorgk
Attachment #9076908 - Flags: review?(bzbarsky)
Comment on attachment 9076908 [details] [diff] [review]
1564094-history.patch

Ah, we mid-air-collided on this!  Yes, I think this is the right fix direction, though it's worth checking whether that regresses bug 463176, which is why that disablehistory bit got added.  If it does regress that, we can think about options for dealing with that.
Flags: needinfo?(bzbarsky)

What about item #3. Move that to a different bug we don't have to ship in TB 68?

Yes.

Bug 463176? I'm not sure what to look for. NS_ERROR_FACTORY_NOT_REGISTERED from nsIDocShellHistory.useGlobalHistory opening view source? I don't see that. There is also no back/forward anywhere in the UI I can see. And there is no history like in FF we could go back to.

NS_ERROR_FACTORY_NOT_REGISTERED from nsIDocShellHistory.useGlobalHistory opening view source? I don't see that.

I'd expect it to be the exception from https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/browser-custom-element.js#1297-1300 if it happens. Which is not NS_ERROR_FACTORY_NOT_REGISTERED but a different error now.

That said, looks like setting disableglobalhistory="true" while not messing with sessionhistory might be the way to go here.

This works, too. Take a pick ;-)

Attachment #9076949 - Flags: review?(bzbarsky)
Comment on attachment 9076949 [details] [diff] [review]
1564094-history.patch (v2)

Yes, this should be nice and safe!
Attachment #9076949 - Flags: review?(bzbarsky) → review+
Attachment #9076670 - Flags: review?(bzbarsky)
Attachment #9076908 - Flags: review?(bzbarsky) → review-
Attachment #9076908 - Attachment is obsolete: true
Attachment #9076949 - Flags: approval-comm-release+
Attachment #9076949 - Flags: approval-comm-beta+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9a902bfb1b62
Enable history for view source so reload works with the system principal. r=bz

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
Version: 63 → 61
Attached patch 1564094-button-follow-up.patch (obsolete) — — Splinter Review
Attachment #9077962 - Flags: review?(acelists)

Oops, that was totally wrong :-(

Attachment #9077962 - Attachment is obsolete: true
Attachment #9077962 - Flags: review?(acelists)
Attachment #9077964 - Flags: review?(acelists)
Comment on attachment 9077964 [details] [diff] [review]
1564094-button-follow-up.patch (v2)

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

Thanks, works for me (no more error about undefined backCommand as the elements do not exist in TB).
Attachment #9077964 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/390fba7934a0
Follow-up: With history enabled, check that back/forward buttons are there. r=aceman
Attachment #9076949 - Flags: approval-comm-release+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: