SM/TB-Trunk: Only two of three attached pictures are shown in news group
Categories
(Thunderbird :: General, defect)
Tracking
(thunderbird50 wontfix, thunderbird51 fixed, thunderbird52 fixed)
People
(Reporter: h.figge, Assigned: jorgk-bmo)
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
3.45 KB,
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Assignee | ||
Comment 1•8 years 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.
Assignee | ||
Comment 2•8 years 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•8 years 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.
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 ;)
Reporter | ||
Comment 5•8 years 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•8 years 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•8 years 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•8 years ago
|
||
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.
Reporter | ||
Comment 9•8 years ago
|
||
WFM too. Without a SSD compiling takes some time. ;)
Comment 10•8 years 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•8 years 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•8 years ago
|
||
How about this? Interesting reading: Bug 193317 comment #8 and bug 193317 comment #9 where this was already suggested.
Assignee | ||
Comment 13•8 years ago
|
||
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•8 years 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.
Comment 15•8 years ago
|
||
JoeS, could you do comment 13, you're the multimedia expert. Remember Bug 193317?
Comment 16•8 years 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•8 years ago
|
||
I sent a message with a background image to test-multimedia@lists.mozilla.org but now it's waiting for approval.
Assignee | ||
Updated•8 years ago
|
Reporter | ||
Comment 18•8 years 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•8 years ago
|
||
Finally the newsgroup message got through, the patch has no adverse effect.
Assignee | ||
Updated•8 years ago
|
Comment 20•8 years ago
|
||
Comment on attachment 8803997 [details] [diff] [review] 1312314.patch Needs a peer to rs+ the review for checkin.
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 21•8 years 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•8 years 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.
Assignee | ||
Comment 23•8 years 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; }
Comment 24•8 years 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.
Assignee | ||
Comment 25•8 years 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.
Comment 26•8 years 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.
Assignee | ||
Comment 27•8 years ago
|
||
After much deliberation ;-) - r=alta88, rs=rkent
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 28•8 years ago
|
||
https://hg.mozilla.org/comm-central/rev/8f555bd5679372e78f7529ad8189690f49152c8b Landed on closed/busted tree, will follow-up with try run.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 29•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2fd71ad418ed4cc1b40b3033fccb756f81e2d92
Comment hidden (obsolete) |
Assignee | ||
Comment 31•8 years ago
|
||
Damn, pasted the wrong changeset. Here we go again: Aurora (TB 51): https://hg.mozilla.org/releases/comm-aurora/rev/d3dc37a155de205e8221477cf29d0065f5831429
Comment 32•5 years ago
|
||
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.
Assignee | ||
Comment 33•5 years ago
|
||
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.
Description
•