Note: There are a few cases of duplicates in user autocompletion which are being worked on.

SM/TB-Trunk: Only two of three attached pictures are shown in news group

RESOLVED FIXED in Thunderbird 52.0

Status

Thunderbird
General
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: Hartmut Figge, Assigned: Jorg K (GMT+2))

Tracking

({regression})

Trunk
Thunderbird 52.0
regression

Thunderbird Tracking Flags

(thunderbird50 wontfix, thunderbird51 fixed, thunderbird52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 months ago
The problem can be seen in the posting 'Testing multiple images' in mozilla.test.multimedia on news.mozilla.org. Only the first two pictures are shown.

The culprit is

The first bad revision is:
changeset:   295188:a6e3503205e8
user:        Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
date:        Wed Apr 27 19:40:56 2016 +0200
summary:     Bug 1206961 - Use channel->AsyncOpen2() in image/imgLoader.cpp; removing security checks from the callsite reveals that we have to pass the accura
te contentPolicyType to ValidateEntry (r=seth,bz)
(Reporter)

Updated

9 months ago
status-firefox52: affected → ---

Updated

9 months ago
Flags: needinfo?(jorgk)
Flags: needinfo?(alta88)

Updated

9 months ago
Component: General → General
Product: Core → Thunderbird

Updated

9 months ago
status-thunderbird52: --- → affected
tracking-thunderbird52: --- → ?
(Assignee)

Comment 1

9 months ago
I can't reproduce this the way it's reported. At times I see the first two pictures, but not the third, at times the first and the third but not the second, sometimes all three. Looks a little like bug 1085677.

I don't think the bug quoted in comment #0 has anything to do with it. Maybe it's a caching problem which was caused be switching to cache2. I'll run it with my caching debug patch to see.

We also know that reading news is broken, Aleth discovered some terrible things in bug 1299562.
Flags: needinfo?(jorgk)
(Assignee)

Comment 2

9 months ago
Hmm, maybe I was 100% wrong. Caching shows nothing strange, however I see:

[3176] WARNING: NS_ENSURE_TRUE(aTargetURI) failed: file c:/mozilla-source/comm-c
entral/mozilla/caps/nsScriptSecurityManager.cpp, line 676
[3176] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file c:
/mozilla-source/comm-central/mozilla/dom/security/nsContentSecurityManager.cpp,
line 587
[3176] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file c:
/mozilla-source/comm-central/mozilla/dom/security/nsContentSecurityManager.cpp,
line 452
[3176] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file c:
/mozilla-source/comm-central/mailnews/news/src/nsNntpMockChannel.cpp, line 297
[3176] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/ima
ge/imgLoader.cpp, line 1699

So by the looks of it, someone passes a bad URI argument aTargetURI to nsScriptSecurityManager::CheckLoadURIWithPrincipal():https://dxr.mozilla.org/mozilla-central/rev/215f9686117673a2c914ed207bc7da9bb8d741ad/caps/nsScriptSecurityManager.cpp#676

All we need to do is attach the debugger and see what's happening.
(Assignee)

Comment 3

9 months ago
OK, here's a stack:
xul.dll!nsScriptSecurityManager::CheckLoadURIWithPrincipal(nsIPrincipal * aPrincipal, nsIURI * aTargetURI, unsigned int aFlags) Line 676	C++
xul.dll!DoCheckLoadURIChecks(nsIURI * aURI, nsILoadInfo * aLoadInfo) Line 111	C++
xul.dll!nsContentSecurityManager::CheckChannel(nsIChannel * aChannel) Line 587	C++
xul.dll!nsContentSecurityManager::doContentSecurityCheck(nsIChannel * aChannel, nsCOMPtr<nsIStreamListener> & aInAndOutListener) Line 452	C++
xul.dll!nsNntpMockChannel::AsyncOpen2(nsIStreamListener * aListener) Line 296	C++

Basically CheckChannel() gets uri==null returned from NS_GetFinalChannelURI() and it's downhill from there. NS_GetFinalChannelURI() calls nsNNTPProtocol::GetOriginalURI():
https://dxr.mozilla.org/comm-central/rev/4cb52bea1cc624ae22871deaad240a6461b6b42a/mailnews/news/src/nsNNTPProtocol.cpp#637

I bet you a beer that that sometimes returns null.

Comment 4

9 months ago
It wfm in Tb45 but not Tb50.0b2 beta. Third is missing, view all parts changes to sometimes second missing sometimes third, and constant progressbar. The progressbar on not the first load of that message after restart but only subsequent ones smells (complete guess) like cache but it's not the real problem. Didn't that land only in 51 though?

It's possible/likely Bug 980451 is involved. I will collect a fast review instead ;)
Flags: needinfo?(alta88)
(Reporter)

Comment 5

9 months ago
(In reply to Jorg K (GMT+2) from comment #1)

> I don't think the bug quoted in comment #0 has anything to do with it.

I can only speak for SM-Trunk x86_64. For the current Trunk a backout of the assumed culprit is not possible, but a build with c-c:0e1299ac02cc and m-c:4292da9df16b shows the bug and is near enough. Backout of changeset:   295188:a6e3503205e8 solves the problem.
(Reporter)

Comment 6

9 months ago
(In reply to Hartmut Figge from comment #5)

> I can only speak for SM-Trunk x86_64.

There is a missing Linux. ;)
(Assignee)

Comment 7

9 months ago
Hartmut, I already withdrew my statement in comment #2.

Debugging this further:
NS_GetFinalChannelURI() gets returned uri==null here:
https://dxr.mozilla.org/comm-central/rev/60dd82380d43a2b681f50842238f829204486290/mozilla/dom/security/nsContentSecurityManager.cpp#529
This is returned from nsNntpMockChannel::GetOriginalURI().
(Assignee)

Comment 8

9 months ago
Created attachment 8803943 [details] [diff] [review]
1312314.patch

I have no idea, but this fixes it. It's similar to
https://dxr.mozilla.org/comm-central/rev/4cb52bea1cc624ae22871deaad240a6461b6b42a/mailnews/news/src/nsNNTPProtocol.cpp#641
from bug 193317.
Attachment #8803943 - Flags: review?(alta88)
(Reporter)

Comment 9

9 months ago
WFM too. Without a SSD compiling takes some time. ;)

Comment 10

9 months ago
That doesn't feel like the right way to do it. nsNntpMockChannel::SetOriginalURI should be setting m_url instead of m_originalUrl, if that indeed is the solution.

-protocol.SetOriginalURI(m_originalUrl); 
+protocol.SetOriginalURI(m_url);

If you make that change, does it ensure the problem in bug 193317 remains fixed?  If so, does running nntp xpcshell/mozmill tests locally look good?
(Assignee)

Comment 11

9 months ago
(In reply to alta88 from comment #10)
> nsNntpMockChannel::SetOriginalURI should be setting m_url instead of
> m_originalUrl, if that indeed is the solution.
Additionally to what I have in my patch?
What's the difference between m_url and m_originalUrl? Can one be eliminated?
Or how about returning |m_originalUrl ? m_originalUrl : m_url| im my patch. 

> -protocol.SetOriginalURI(m_originalUrl); 
> +protocol.SetOriginalURI(m_url);

That doesn't work. nsNntpMockChannel::AttachNNTPConnection() seems to exit early and not call protocol.SetOriginalURI(m_url);

As far as I can see, nsNntpMockChannel::SetOriginalURI() is never called, at least not when simply starting TB and viewing the message in question.

I don't even know why I'm looking at this. I guess Aleth NI'ed me. Alta88, would you like to take over here.
(Assignee)

Comment 12

9 months ago
Created attachment 8803997 [details] [diff] [review]
1312314.patch

How about this?

Interesting reading: Bug 193317 comment #8 and bug 193317 comment #9 where this was already suggested.
Attachment #8803943 - Attachment is obsolete: true
Attachment #8803943 - Flags: review?(alta88)
(Assignee)

Comment 13

9 months ago
Comment on attachment 8803997 [details] [diff] [review]
1312314.patch

Could you kindly post a message with a background image to m.t.multimedia.
Attachment #8803997 - Flags: review?(alta88)

Comment 14

9 months ago
Comment on attachment 8803997 [details] [diff] [review]
1312314.patch

That looks correct, various gecko parts require a member GetOriginalURI even if it repeats m_url.
Attachment #8803997 - Flags: review?(alta88) → review+

Comment 15

9 months ago
JoeS, could you do comment 13, you're the multimedia expert. Remember Bug 193317?
Flags: needinfo?(jsabash)

Comment 16

9 months ago
(In reply to Jorg K (GMT+2) from comment #11)
> I don't even know why I'm looking at this. I guess Aleth NI'ed me. Alta88,
> would you like to take over here.

I needinfo'd you both because one way or another, you were the last two people to touch nntp stuff ;)
(Assignee)

Comment 17

9 months ago
I sent a message with a background image to test-multimedia@lists.mozilla.org but now it's waiting for approval.
(Assignee)

Updated

9 months ago
status-thunderbird50: --- → wontfix
status-thunderbird51: --- → affected
tracking-thunderbird52: ? → +
(Reporter)

Comment 18

9 months ago
For testing I prefer albasani.mozilla. Not moderated. Ir requires an account on news.albasani.net, though, so it's use for Bugzilla could not be recommended.
(Assignee)

Comment 19

9 months ago
Finally the newsgroup message got through, the patch has no adverse effect.
(Assignee)

Updated

9 months ago
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(jsabash)
Keywords: checkin-needed

Comment 20

9 months ago
Comment on attachment 8803997 [details] [diff] [review]
1312314.patch

Needs a peer to rs+ the review for checkin.
Attachment #8803997 - Flags: review?(mkmelin+mozilla)

Updated

9 months ago
Keywords: checkin-needed
(Assignee)

Updated

9 months ago
Attachment #8803997 - Flags: review?(mkmelin+mozilla) → review?(rkent)
(Assignee)

Comment 21

9 months ago
Comment on attachment 8803997 [details] [diff] [review]
1312314.patch

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

Kent inspired to look into it some more, so one suggestion below.

::: mailnews/news/src/nsNntpMockChannel.cpp
@@ +145,5 @@
>  
>  NS_IMETHODIMP nsNntpMockChannel::SetOriginalURI(nsIURI *aURI)
>  {
>    FORWARD_CALL(SetOriginalURI, aURI)
> +  m_url = aURI;

Looking at nsNNTPProtocol::SetOriginalURI()
https://dxr.mozilla.org/comm-central/rev/7c2e6b4b08219dc2e2c2278d0fc645678166145e/mailnews/news/src/nsNNTPProtocol.cpp#646
maybe it would be better to not assign anything to m_url here.

Comment 22

9 months ago
Comment on attachment 8803997 [details] [diff] [review]
1312314.patch

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

"Looking at nsNNTPProtocol::SetOriginalURI()
https://dxr.mozilla.org/comm-central/rev/7c2e6b4b08219dc2e2c2278d0fc645678166145e/mailnews/news/src/nsNNTPProtocol.cpp#646
maybe it would be better to not assign anything to m_url here."

My guess is to set m_url as the patch says, as the assumption here is that originalURI and m_url are identical.

I can't really add any value here, so I'll just rs+ the existing work.
Attachment #8803997 - Flags: review?(rkent) → review+
(Assignee)

Comment 23

9 months ago
Thanks. Poor Kent has to deal with all these requests. Let me ask Alta88.
I will land with r=alta88,rs=rkent if the tree re-opens one day.

Alta88, looking at
NS_IMETHODIMP nsNNTPProtocol::SetOriginalURI(nsIURI* aURI)
{
    // News does not seem to have the notion of an original URI (See Bug #193317)
    return NS_OK;       // ignore
}

which version do you prefer and why?

NS_IMETHODIMP nsNntpMockChannel::SetOriginalURI(nsIURI *aURI)
{	
  FORWARD_CALL(SetOriginalURI, aURI)
  m_url = aURI; <<=== do this assignment or not?
  return NS_OK;
}
Flags: needinfo?(alta88)

Comment 24

9 months ago
(In reply to Jorg K (GMT+2) from comment #23)
> Thanks. Poor Kent has to deal with all these requests. Let me ask Alta88.
> I will land with r=alta88,rs=rkent if the tree re-opens one day.
> 
> Alta88, looking at
> NS_IMETHODIMP nsNNTPProtocol::SetOriginalURI(nsIURI* aURI)
> {
>     // News does not seem to have the notion of an original URI (See Bug
> #193317)
>     return NS_OK;       // ignore
> }

This is also the behavior in the imap versions of SetOriginalURI in their channel definitions.

> 
> which version do you prefer and why?
> 
> NS_IMETHODIMP nsNntpMockChannel::SetOriginalURI(nsIURI *aURI)
> {	
>   FORWARD_CALL(SetOriginalURI, aURI)
>   m_url = aURI; <<=== do this assignment or not?
>   return NS_OK;
> }

The question is whether SetOriginalURI is ever called, and who calls it. The patch removes the one call from nntp code, but is it possible SetOriginalURI is called with a null aURI in gecko land?

The better course would be to not potentially set m_url to null, but leave it. Meaning remove that line.  It will be easy enough to change if some regression pops up, so no need to over stress about it.
Flags: needinfo?(alta88)
(Assignee)

Comment 25

9 months ago
OK, thanks for the inspiration. I did some debugging dumping out m_url and aURI passed to nsNntpMockChannel::SetOriginalURI(). The result is this:

=== m_url = news://news.mozilla.org:119/mailman.0.1455993882.8167.test-multimedi
a%40lists.mozilla.org?group=mozilla.test.multimedia&key=3437
=== aURI = news://news.mozilla.org:119/mailman.0.1455993882.8167.test-multimedia
%40lists.mozilla.org?group=mozilla.test.multimedia&key=3437

So if the original URI is set, it's set to a value that m_url already holds. So whether or not we do the assignment doesn't really matter.

So does that help us in deciding whether we should leave or remove the |m_url = aURI;|?

Sorry to trouble you again with this.
Flags: needinfo?(alta88)

Comment 26

9 months ago
(In reply to Jorg K (GMT+2) from comment #25)
> OK, thanks for the inspiration. I did some debugging dumping out m_url and
> aURI passed to nsNntpMockChannel::SetOriginalURI(). The result is this:
> 
> === m_url =
> news://news.mozilla.org:119/mailman.0.1455993882.8167.test-multimedi
> a%40lists.mozilla.org?group=mozilla.test.multimedia&key=3437
> === aURI =
> news://news.mozilla.org:119/mailman.0.1455993882.8167.test-multimedia
> %40lists.mozilla.org?group=mozilla.test.multimedia&key=3437
> 
> So if the original URI is set, it's set to a value that m_url already holds.
> So whether or not we do the assignment doesn't really matter.
> 
> So does that help us in deciding whether we should leave or remove the
> |m_url = aURI;|?
> 

I think the safer, more prudent thing to do (and as is done in imap) is to remove the assignment and thus the risk that m_url is set to something like null.
Flags: needinfo?(alta88)
(Assignee)

Comment 27

9 months ago
Created attachment 8805554 [details] [diff] [review]
1312314.patch

After much deliberation ;-) - r=alta88, rs=rkent
Attachment #8803997 - Attachment is obsolete: true
Attachment #8805554 - Flags: review+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed
(Assignee)

Comment 28

9 months ago
https://hg.mozilla.org/comm-central/rev/8f555bd5679372e78f7529ad8189690f49152c8b
Landed on closed/busted tree, will follow-up with try run.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
(Assignee)

Updated

9 months ago
Attachment #8805554 - Flags: approval-comm-aurora+
(Assignee)

Comment 29

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2fd71ad418ed4cc1b40b3033fccb756f81e2d92
Comment hidden (obsolete)
(Assignee)

Comment 31

9 months ago
Damn, pasted the wrong changeset. Here we go again:
Aurora (TB 51):
https://hg.mozilla.org/releases/comm-aurora/rev/d3dc37a155de205e8221477cf29d0065f5831429
You need to log in before you can comment on or make changes to this bug.