Closed Bug 471131 Opened 15 years ago Closed 15 years ago

NNTP: Incomplete loading of multiple inline images/content

Categories

(MailNews Core :: Networking: NNTP, defect)

x86
All
defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: JoeS1, Assigned: jcranmer)

References

Details

(Keywords: regression)

Attachments

(2 files)

Steps:
Open a Newgroups message with more than 2 inline images.
Note that only the first 2 image display

You may have to open the same message more than once to see the problem.

Regression Range:
OK
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081218 Lightning/1.0pre Shredder/3.0b2pre ID:20081218031250

Bad
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081219 Lightning/1.0pre Shredder/3.0b2pre ID:20081219032723

The same identical message will display fine when opened from a local folder.

BTW: Changing Max_Cached_connections pref to a higher number does not seem to help.
If bug 470685 (a Linux bug) is a dupe, then this bug is not only seen on Windows.
OS: Windows XP → All
Attached file nntp log
The log refers to the testcase post.
The post is multipart/related with 5 parts.
After the 2nd part rendered, TB spins with "loading nessage in the status bar.
I closed the window at that point.
This makes current nightlies pretty much unusable for viewing newsgroup posts of this type.
(In reply to comment #4)

> This makes current nightlies pretty much unusable for viewing newsgroup posts
> of this type.

Backing out as mentioned in Bug 470685 helps. I am doing so for my private builds until 471131 is fixed.
Flags: blocking-thunderbird3?
(In reply to comment #0)

> BTW: Changing Max_Cached_connections pref to a higher number does not seem to
> help.

Well, it does help if you discover the correct pref in configedit
mail.server.server5.max_cached_connections (In my case)

IMAP default is set to 10
Why should we default NNTP to anything less.
imap default is 5.

For news, building up the state for a particular folder/group is a lot cheaper than it is for imap, so there's much less incentive to cache connections.
(In reply to comment #6)
> IMAP default is set to 10
> Why should we default NNTP to anything less.

NNTP servers (especially ISP servers) very frequently throttle the number of connections; 2 is a common maximum, although premium services may have more.

And the issue here is orthogonal: NNTP is not IMAP, so it's far better to ask why one should use the same settings for IMAP and NNTP.
But why should the Max# connections preclude closing the connection, and re-opening to get the remaining parts of the post.
Maybe we are not closing the connections at all ??
netstat shows that a connection is opened for each part of a multi-part message.
The connections are not closed at the completion of each part, nor at the end
of the "body load"

I don't remember how to pipe netstat results to a file to document this.

If mail.server.serverX.max_cached_connections remains at a default of 2
that means that you can never view a multipart post of more than 2 parts.

If this is going to hold, then I think we must add UI to make this more
discoverable.
doesn't block tb3.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
(In reply to comment #11)
> doesn't block tb3.

Well, it _is_ a recent regression caused by the bug 66150 "fix"
Certainly would impact newsgroup users coming over from tb2 that will be
quite surprised by the change.

The work-around pref (mail.server.serverX.max_cached_connections)
is not very discoverable at all.
I've been running through the code with a debugger and debug statements at various points and I've found the problem.

It turns out that, because of some ancient bug, the NNTP protocol object fails to work properly when it's loading an article or part from the cache. Since bug 66150, we've tried to reuse these unusable protocol objects.

I should be able to get this working within the next week.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attached patch Patch, version 1Splinter Review
Note to self: do not run make check when a patch known to break make check is applied.

As noted in the previous comment, the cause of the problem is that AsyncOpen fails to run the next URL in some cases, namely when reading offline or from the memory cache. This patch ensures that the next URL runs in these cases.

I've also cleaned up the comments in AsyncOpen (no one's really touched that since mscott wrote that 8 years ago), and added a log note when the file is being read from offline.
Attachment #356342 - Flags: superreview?(bienvenu)
Attachment #356342 - Flags: review?(bienvenu)
(In reply to comment #14)

> Created an attachment (id=356342) [details]

FWIW, this patch works for me.
Attachment #356342 - Flags: superreview?(bienvenu)
Attachment #356342 - Flags: superreview+
Attachment #356342 - Flags: review?(bienvenu)
Attachment #356342 - Flags: review+
Pushed as changeset 57470c8e8c12.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Checked with hourly:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090117 Lightning/1.0pre Shredder/3.0b2pre ID:20090117195944

Another ancient problem has been fixed:
Saved images are now retrieved from the memory cache, and not re-downloaded.
(although the memory cache default might have to be bumped up)

Nice to see newsgroups users needs not regarded as "edge cases"

Thanks to Joshua and David
Status: RESOLVED → VERIFIED
(In reply to comment #16)
> Pushed as changeset 57470c8e8c12.

What I am going to test is if this also resolves the constant refetching of attachments to news when each attachment is opened sequentialy.
I did not find enough test massages to be satisfied with the quality of my testing.  Yet I found this fix seems to have fixed the refetch from server for each attachment opened by a viewer/handler.

Now we need to find the related bugs to process for closure.
More testing and I am rescinding comment #19. This fix has no effect on the case where an posting has several binary attachments. Selecting each to open in it's associated viewer (tested with Windows Fontviewer and *.TTF files) resulted in the old behavior of reloading from the server for each attachment viewed. Checking memory cache with about:cache(memory) showed it was not being used.
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.