245110 - Crash [@ nsAboutRedirector::NewChannel]

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[timeless]

cvs build from this morning, build options should approximate
http://viper.haque.net/~timeless/mozconfig/cobra/firebird.mozconfig

note: the box is doppler, not cobra, i'll post mozconfigs for doppler at a later
date.

Date/Time:      2004-05-30 11:33:38 -0700
OS Version:     10.3.4 (Build 7H63)
Report Version: 2

Command: firefox-bin
Path:    ./firefox-bin
Version: 0.8.0+ (0.8.0+)
PID:     23526
Thread:  0

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_PROTECTION_FAILURE (0x0002) at 0x00000000

Thread 0 Crashed:
0   firefox-bin         	0x000a914c nsAboutRedirector::NewChannel(nsIURI*,
nsIChannel**) + 0x2cc (nsAboutRedirector.cpp:113)
1   firefox-bin         	0x000b4d34 nsAboutProtocolHandler::NewChannel(nsIURI*,
nsIChannel**) + 0x234 (nsCOMPtr.h:710)
2   firefox-bin         	0x00083060 nsIOService::NewChannelFromURI(nsIURI*,
nsIChannel**) + 0x1e8 (nsCOMPtr.h:710)
3   firefox-bin         	0x00847cd8 0x1000 + 0x846cd8
4   firefox-bin         	0x00523fb0 nsDocShell::DoURILoad(nsIURI*, nsIURI*,
nsISupports*, char const*, nsIInputStream*, nsIInputStream*, int, nsIDocShell**,
nsIRequest**) + 0x130 (nsDocShell.cpp:5422)
5   firefox-bin         	0x00523c04 nsDocShell::InternalLoad(nsIURI*, nsIURI*,
nsISupports*, int, unsigned short const*, char const*, nsIInputStream*,
nsIInputStream*, unsigned, nsISHEntry*, int, nsIDocShell**, nsIRequest**) +
0xdcc (nsDocShell.cpp:5339)
6   firefox-bin         	0x00518d6c nsDocShell::LoadURI(nsIURI*,
nsIDocShellLoadInfo*, unsigned, int) + 0x5ec (nsCOMPtr.h:704)
7   firefox-bin         	0x0051cea4 nsDocShell::LoadURI(unsigned short const*,
unsigned, nsIURI*, nsIInputStream*, nsIInputStream*) + 0x398 (nsCOMPtr.h:704)
8   libxpcom.dylib      	0x16c14184 _XPTC_InvokeByIndex + 0xd8
(xptcstubsdef.inc:256)
9   firefox-bin         	0x0002cbb0
XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) +
0x9ec (xpcwrappednative.cpp:2026)

note: firefox trunk is known to be braindead. but this crash is due to poorly
written necko code.

steps:
1. build firefox (trunk)
2. run firefox (trunk)
3. observe errors loading plugins (console spew)
4. predict firefox will crash when you load about:plugins
5. load about:plugins

Comment 1

[timeless]

Attachment: fix

Comment 2

[Darin Fisher]

Comment on attachment 149675 [details] [diff] [review]
fix

I can see why you chose to write this patch, but I think it would be better to
null check |result| up front, and return a failure like NS_ERROR_ILLEGAL_VALUE
when result is null.

then again, why are you passing null as the out param here?  that's completely
bogus.

i would be happy if you added two NS_ASSERTION lines to the top of the function
and removed the runtime null checks.

Comment 3

[timeless]

Comment on attachment 149675 [details] [diff] [review]
fix

The key to this patch is not that i'm playing with result, it's that i'm
playing with under what conditions we poke *result. unfortunately i chose to
post a -w patch, sorry.

>             nsCOMPtr<nsIChannel> tempChannel;
>              rv = ioService->NewChannel(nsDependentCString(kRedirMap[i].url),
>                                         nsnull, nsnull, getter_AddRefs(tempChannel));
>              // Keep the page from getting unnecessary privileges unless it needs them
>-             if (NS_SUCCEEDED(rv) && result && kRedirMap[i].dropChromePrivs)
>+             if (NS_SUCCEEDED(rv) && result)
>+             {
>+                  if (kRedirMap[i].dropChromePrivs)
>              {
		    /*ignore this*/
>              }
/*in the old code this stuff is executed regardless of the value of rv */
>              *result = tempChannel.get();
>              NS_ADDREF(*result);
/*the new code split the condition so that the two lines above which crashed
are now guarded by the half of the condition which matters before the block
ends at the new end block marker which follows:*/
>+             }
>              return rv;

if you have an opinion on null checking result, please deal with it elsewhere,
it seems like someone wanted that behavior and i just want to fix the crash
which is semi serious given that ff is triggering it and we're marketting ff. i
don't have time to do the code archeology required to figure out whether
passing a null result is of interest.

Comment 4

[Darin Fisher]

under what conditions does this crash occur?  is it only happening in your
build?  are there talkback reports indicating that this happens in the real world?

i don't understand your objection to moving the null check to the top of the
function where it belongs.

Comment 5

[timeless]

this occurs whenever someone decides not to package the redirected files served
by the about redirector as ben did.

note this was introduced in rev 1.3 by mstoltz, as part of his second patch
version (in response to waterson's request), it never was properly reviewed.

bbaetz owns review blame for the original change which null checked result.
i've now wasted time checking the callers and am satisfied no one actually
passes null, although some callers leak like a sieve.

Comment 6

[Darin Fisher]

Attachment: v2 patch

This patch cleans up nsAboutRedirector.cpp ... it really needed some loving!

Comment 7

[Darin Fisher]

I wrote ben to make sure that he is aware that this problem exists or existed? 
Timeless: do you know if the packaging issue was resolved?  

Comment 8

[Darin Fisher]

This is an XP problem even if it only has showed up on OSX builds.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 150280 [details] [diff] [review]
v2 patch

+	     rv = ioService->NewChannel(nsDependentCString(kRedirMap[i].url),
+					nsnull, nsnull,
getter_AddRefs(tempChannel));
+	     // Keep the page from getting unnecessary privileges unless it
needs them
+	     if (NS_SUCCEEDED(rv))

hm... why not return here if NS_FAILED(rv)? then you wouldn't need to indent as
much below :)

+    NS_WARNING("nsAboutRedirector called for unknown case");

as this should never happen (because nsAboutRedirector is only registered for
the urls it knows about), maybe NS_ERROR would be better?

nice patch!

Comment 10

[Darin Fisher]

biesi: thanks for the suggestions

Comment 11

[Darin Fisher]

fixed-on-trunk

Comment 12

[benc]

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/netwerk/protocol/about/src/nsAboutRedirector.cpp
1.18darin%meer.net2004-06-08 14:22 fixes bug 245110 "Crash [@
nsAboutRedirector::NewChannel]" r=biesi