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

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Hartmut Figge]

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 <[e-mail]>
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)

Comment 1

[Jorg K (CEST = GMT+2)]

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.

Comment 2

[Jorg K (CEST = GMT+2)]

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.

Comment 3

[Jorg K (CEST = GMT+2)]

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

[alta88]

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 ;)

Comment 5

[Hartmut Figge]

(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.

Comment 6

[Hartmut Figge]

(In reply to Hartmut Figge from comment #5)

> I can only speak for SM-Trunk x86_64.

There is a missing Linux. ;)

Comment 7

[Jorg K (CEST = GMT+2)]

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().

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 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.

Comment 9

[Hartmut Figge]

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

Comment 10

[alta88]

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?

Comment 11

[Jorg K (CEST = GMT+2)]

(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.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1312314.patch

How about this?

Interesting reading: Bug 193317 comment #8 and bug 193317 comment #9 where this was already suggested.

Comment 13

[Jorg K (CEST = GMT+2)]

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

Could you kindly post a message with a background image to m.t.multimedia.

Comment 14

[alta88]

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.

Comment 15

[alta88]

JoeS, could you do comment 13, you're the multimedia expert. Remember Bug 193317?

Comment 16

[aleth [:aleth]]

(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 ;)

Comment 17

[Jorg K (CEST = GMT+2)]

I sent a message with a background image to [e-mail] but now it's waiting for approval.

Comment 18

[Hartmut Figge]

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.

Comment 19

[Jorg K (CEST = GMT+2)]

Finally the newsgroup message got through, the patch has no adverse effect.

Comment 20

[aleth [:aleth]]

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

Needs a peer to rs+ the review for checkin.

Comment 21

[Jorg K (CEST = GMT+2)]

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

[Kent James (:rkent)]

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.

Comment 23

[Jorg K (CEST = GMT+2)]

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;
}

Comment 24

[alta88]

(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.

Comment 25

[Jorg K (CEST = GMT+2)]

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.

Comment 26

[alta88]

(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.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1312314.patch

After much deliberation ;-) - r=alta88, rs=rkent

Comment 28

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/8f555bd5679372e78f7529ad8189690f49152c8b
Landed on closed/busted tree, will follow-up with try run.

Comment 29

[Jorg K (CEST = GMT+2)]

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

Comment 30

[Jorg K (CEST = GMT+2)]

Aurora (TB 51):
https://hg.mozilla.org/releases/comm-aurora/rev/3a2c0f8c8896daf9e9a7ab9fd23eea1c9583ec13

Comment 31

[Jorg K (CEST = GMT+2)]

Damn, pasted the wrong changeset. Here we go again:
Aurora (TB 51):
https://hg.mozilla.org/releases/comm-aurora/rev/d3dc37a155de205e8221477cf29d0065f5831429

Comment 32

[Boris Zbarsky [:bzbarsky]]

This patch is ... very questionable. The right behavior for channels is to have an originalURI member that is set to the same thing as the URI at channel creation time (which NTTP mock channels were not doing) and then can be changed via SetOriginalURI. The invariant is that it can never be null (and NTTP mock was violating this invariant). See bug 1564094 for the sort of problems messing up SetOriginalURI can cause.

Comment 33

[Jorg K (CEST = GMT+2)]

Boris, thanks for digging thought this, but it's too late now. I was a newbie at TB 52 and Kent suggested this fix. Let's have the discussion in bug 1564094. I've already proposed to align the IMAP behaviour with nsMsgProtocol and we can clean-up NNTP at the same time.

Debugging in bug 1564094 comment #19 showed that at least wrt to "View Source" news messages don't appear to have a problem.