210089 - crash if I try to read a posting in a newsgroup containing a "&" in its name [@ nsNntpService::GetFolderFromUri ]

Status: VERIFIED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[Michael Kunz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.4) Gecko/20030529
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.4) Gecko/20030529

If I try to read a posting in a newsgroup, which contains a "&" in its name,
Mozilla totally crashes.
This worked till Mozilla 1.4b. Since 1.4RC1 I can reproduce this bug everytime I
try to read a posting in a newsgroup with a "&" in it.
I have sent two Talkback reports concerning this bug.
1.4RC1: TB21205572X
1.4RC2: TB21205021Q

Reproducible: Always

Steps to Reproduce:
1. subscribe a newsgroup with "&" in its name (i know Son-of-RFC1036, but this
is no reason for a crash)
2. click on a posting in this newsgroup
3. crash

Actual Results:  
Mozilla totally crashes.

Expected Results:  
Mozilla should show me the selected posting.

Talkback crash IDs:
1.4RC1: TB21205572X
1.4RC2: TB21205021Q

Comment 1

[Jim Dunn]

posting for mcsmurf

TB21205572X (& for the most part TB21205021Q is the same)
---------------------------------------------------------
nsNntpService::GetFolderFromUri
[d:/builds/seamonkey/mozilla/mailnews/news/src/nsNntpService.cpp, line 647]
nsNntpService::DecomposeNewsMessageURI
[d:/builds/seamonkey/mozilla/mailnews/news/src/nsNntpService.cpp, line 602]
nsNntpService::DisplayMessage
[d:/builds/seamonkey/mozilla/mailnews/news/src/nsNntpService.cpp, line 245]
nsMessenger::OpenURL
[d:/builds/seamonkey/mozilla/mailnews/base/src/nsMessenger.cpp, line 575]
nsMsgDBView::LoadMessageByMsgKeyHelper
[d:/builds/seamonkey/mozilla/mailnews/base/src/nsMsgDBView.cpp, line 963]
nsMsgDBView::LoadMessageByMsgKey
[d:/builds/seamonkey/mozilla/mailnews/base/src/nsMsgDBView.cpp, line 946]
nsMsgDBView::SelectionChanged
[d:/builds/seamonkey/mozilla/mailnews/base/src/nsMsgDBView.cpp, line 1016]
XPTC_InvokeByIndex
[d:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2025]
XPC_WN_CallMethod
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1285]
js_Invoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 845]
js_Interpret [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2854]
js_Invoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 861]
js_InternalInvoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 936]
JS_CallFunctionValue [d:/builds/seamonkey/mozilla/js/src/jsapi.c, line 3529]
nsJSContext::CallEventHandler
[d:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1114]
nsJSEventListener::HandleEvent
[d:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp, line 183]
nsEventListenerManager::HandleEventSubType
[d:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, line
1192]
nsEventListenerManager::HandleEvent
[d:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, line
1804]
nsXULElement::HandleDOMEvent
[d:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp, line 3302]
nsTreeSelection::FireOnSelectHandler
[d:/builds/seamonkey/mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp,
line 764]
nsTreeSelection::Select
[d:/builds/seamonkey/mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp,
line 381]
XPTC_InvokeByIndex
[d:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2025]
XPC_WN_CallMethod
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1285]
js_Invoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 845]
js_Interpret [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2854]
js_Invoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 861]
js_InternalInvoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 936]
JS_CallFunctionValue [d:/builds/seamonkey/mozilla/js/src/jsapi.c, line 3529]
nsJSContext::CallEventHandler
[d:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1114]
nsJSEventListener::HandleEvent
[d:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp, line 183]
nsXBLPrototypeHandler::ExecuteHandler
[d:/builds/seamonkey/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp, line 449]
nsXBLEventHandler::DoMouse
[d:/builds/seamonkey/mozilla/content/xbl/src/nsXBLEventHandler.h, line 149]
nsXBLMouseHandler::MouseDown
[d:/builds/seamonkey/mozilla/content/xbl/src/nsXBLMouseHandler.cpp, line 81]
nsEventListenerManager::HandleEvent
[d:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, line
1282]
nsXULElement::HandleDOMEvent
[d:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp, line 3302]
PresShell::HandleEventInternal
[d:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6389]
PresShell::HandleEvent
[d:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6320]
nsViewManager::HandleEvent
[d:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2314]
nsView::HandleEvent [d:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 308]
nsViewManager::DispatchEvent
[d:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2050]
HandleEvent [d:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 82]
nsWindow::DispatchEvent
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1057]
nsWindow::DispatchWindowEvent
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1074]
nsWindow::DispatchMouseEvent
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5190]
ChildWindow::DispatchMouseEvent
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5445]
nsWindow::ProcessMessage
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 4029]
nsWindow::WindowProc
[d:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1337]
USER32.dll + 0x3a68 (0x77d13a68)
USER32.dll + 0x3b37 (0x77d13b37)
USER32.dll + 0x3d91 (0x77d13d91)
USER32.dll + 0x3df7 (0x77d13df7)
nsAppShellService::Run
[d:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 479]
main1 [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1284]
main [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1650]
WinMain [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1672]
WinMainCRTStartup()
kernel32.dll + 0x214c7 (0x77e614c7) 

Comment 2

[Frank Wein [:mcsmurf]]

Could be checkin from Bug 153052

Comment 3

[Karsten Düsterloh]

I agree. That patch and its successors do not take into account that in 

|  rv = rootFolder->GetChildNamed(NS_ConvertASCIItoUCS2(path.get() + 1).get()
|                                 /* skip the leading slash */, 
|                                 getter_AddRefs(subFolder));
|  NS_ENSURE_SUCCESS(rv,rv);
|  return CallQueryInterface(subFolder, aFolder);

the |subFolder| might be NULL, eg when "&" occur in |path|.
In a debug build, CallQueryInterface asserts, but RC1 and RC2 do crash.

Comment 4

[Michael Kunz]

Is there a chance to fix this before the next release? This bug makes Mozilla  
unusable for me.
This bug can't be so hard to fix, since it worked wonderful till the RCs for 1.4

Comment 5

[Asa Dotzler [:asa]]

unsetting requests for already released milestones.

Comment 6

[Frank Schönheit]

The "&" in the name is most probably irrelevant here. I suppose the deeper
reason for the crash (besides not testing |subFolder|, as pointed out by
Karsten) is that  the
  accountManager->FindServer( ... )
some lines above does *only* use the host name to find the server. In setups
where there are multiple accounts, working on the server, this will fail for all
but one account. And this fail will immediately lead to a crash.

I suppose that both bug 210887 and bug 210547 are duplicates of this one here.

(the problem, btw, was was introduced with the fix for bug 186725)

A first-level fix for the problem is checking subFolder for being NULL.
Unfortunately, this only leads to Mozilla not crashing anymore, but still not
displaying the postings in all accounts (working on the same server) except one.

As a real fix, I tried to also pass a reasonable value for the first argument of
findServer (userPass), which currently is left empty. This seemed to fix the
problem for me - I will attach a diff file later.

Comment 7

[Frank Schönheit]

> there are multiple accounts, working on the server, ...
                                             ^
insert " same" here -------------------------+

Comment 8

[Frank Schönheit]

yet another correction :(

> (the problem, btw, was was introduced with the fix for bug 186725)

should have been: bug 153052

Comment 9

[Frank Schönheit]

Attachment: suggested patch

this fixes the crash as well as the problem of finding the wrong server.

I don't know of any side effects, but don't know this code very well. In any
way, I suppose that what the patch does is necessary /at least/. The current
behaviour of caring for the host only when looking up a server definately fails
in scenarious with multiple accounts having the same host.

Comment 10

[Frank Schönheit]

Comment on attachment 129459 [details] [diff] [review]
suggested patch

bienvenu, could you have a look at this, please?

Comment 11

[David :Bienvenu]

Comment on attachment 129459 [details] [diff] [review]
suggested patch

this looks OK, except

+  PR_FREEIF(unescapedUserPass);

can just be PR_Free (PR_Free checks for null as well - the difference is that
PR_FREEIF nulls out the arg pointer, which we don't care about)

Comment 12

[Michael Kunz]

My three news accounts point to different servers. Anyway Mozilla crashes when I
try to read a posting in a group with "&" in its name.

Comment 13

[Frank Schönheit]

so my theory is dying then :)

I assume that attachment 129459 [details] [diff] [review] will also fix your crash (by fixing the
symptoms, but perhaps not the reasons), but speaking strictly, it's a patch for
bug 210547, not for this one here.

Comment 14

[Frank Schönheit]

Attachment: PR_Free instead of PR_FREEIF

Comment 15

[Frank Schönheit]

Comment on attachment 129605 [details] [diff] [review]
PR_Free instead of PR_FREEIF

Seth, could you please have a look at this?

Perhaps it's worth thinking about changing the semantics of
nsIFolder::getChildNamed so that it doesn't return NS_OK when there is no sub
folder with the given name. This would have prevented this bug, and perhaps is
more natural.

I found only 4 places (1 x nsMsgFolder.cpp, 3 x nsImportMail.cpp) which could,
but not even need to, be adjusted then.

Comment 16

[(not reading, please use [e-mail] instead)]

sorry for the delay.
  
I'm able to reproduce this.  I'll take a look.

Comment 17

[(not reading, please use [e-mail] instead)]

frank, thanks for doing the initial work here.

question:  does your fix allow you to read messages on newsgroups with "&" in
the newsgroup name, or does it just prevent the crash?

on disk (so the .newsrc file), we store the group name as unescaped. (foo&bar.test)

but for our URIs, and behind the scenes, in the subscribe UI code, we store it
as escaped, (foo%26bar.test)

when creating our rdf resources for the newsgroups, the uri would be:
news://host/foo%26bar.test (you can see why we'd want to escape here, imagine a
folder name with a ? or &, etc.)

but we set the pretty name to be the unescaped version (foo&bar.test), as that's
what the user understands.

see nsMsgNewsFolder::AddNewsgroup() for where this all happens.

so I think the fix for this crasher (and for reading messages in a group like
foo&bar.test) is to unescape the path, before we call GetChildNamed() in
nsNntpService::GetFolderFromUri()

the reason this is correct is that the subfolder for "foo&bar.test" has that as
isn't name [the uri is news://host/foo%26bar.test, but we set the name to be the
unescaped form in nsMsgNewsFolder::AddNewsgroup()]

in my tests, this will fix the crasher and allow me to read messages on a group
like foo&bar.test

I'll attach that patch.

then, we should decide how to fix the problem of GetChildNamed() returning
NS_OK, but no folder (which causes this crash).

In addition the news usage and the import usage, there is a usage in
nsMsgFolder::ContainsChildNamed().  we should audit all the occurrances, but my
guess is we should do what frank suggests, and not return NS_OK (but
NS_ERROR_FAILURE).

also, let's take the issue of multiple accounts (different user named) on the
same news server to another bug.

Comment 18

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 19

[(not reading, please use [e-mail] instead)]

I don't have access to a news server with a newsgroup like "foo&bar.test".

To test my patch, here's how I faked it:

1)  set up news://news.mozilla.org/netscape.test for offline use, go offline,
and download messages
2)  exit mozilla
3)  add foo&bar.test to my news.mozilla.org.rc file
4)  copy netscape.test to foo&bar.test (the downloaded messages)
5)  copy netscape.test.msf to foo&bar.test.msf (the summary files)
6)  start mozilla up, and go offline (you might have to cancel the "no such
group, do you want to unsubscribe?" prompt)
7)  try to read an offline message in foo&bar.test

Comment 20

[Frank Schönheit]

@Seth
thanks for the illustrations in comment 17 - indeed, my patch prevented the
crash only, but did not allow to read the messages in such groups.
I tried your patch with a faked NG as described, and it both didn't crash and
allowed to read the messages :)

going to move my patch to bug 210547, where it belongs (after your comments, I
think I may also come up with a alternative version which also changes
GetChildNamed)
Thanks.

Comment 21

[Frank Schönheit]

Comment on attachment 129605 [details] [diff] [review]
PR_Free instead of PR_FREEIF

obsoleting and removing sr request

Comment 22

[Frank Schönheit]

Comment on attachment 129459 [details] [diff] [review]
suggested patch

obsoleting

Comment 23

[(not reading, please use [e-mail] instead)]

fix checked into the trunk, for 1.4 beta.  

we'll want this on the 1.4.1 branch, too.

Comment 24

[(not reading, please use [e-mail] instead)]

> fix checked into the trunk, for 1.4 beta.  

I meant 1.5 beta.

Comment 25

[Matthias Versen [:Matti]]

*** Bug 217156 has been marked as a duplicate of this bug. ***

Comment 26

[Mike Kaply [:mkaply]]

What's the deal with this one? Is it fixed?

I see no sr or approval or requests, but it's marked blocking 1.4?

Does someone want this in?

Comment 27

[(not reading, please use [e-mail] instead)]

yes, the crasher was fixed.  we want this for 1.4.1, too.

Comment 28

[Frank Wein [:mcsmurf]]

checked in by mkaply, see also
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/mailnews/news/src&command=DIFF_FRAMESET&file=nsNntpService.cpp&rev1=1.233&rev2=1.233.4.1&root=/cvsroot

Comment 29

[Asa Dotzler [:asa]]

*** Bug 210887 has been marked as a duplicate of this bug. ***

Comment 30

[Stephen Donner [:stephend] Not actively reading bugmail]

0 crashes found where the Stack Signature contains
'nsnntpservice::getfolderfromuri'.