282644 - [FIX]Crash when using Command-Enter for a URL beginning with an unregistered protocol.

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Wevah (Nate Weaver)]

Type something like

foo:bar

in the location bar, and then press Command-Enter. Witness crash.

Comment 1

[Simon Fraser [no longer active]]

Stack.

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

Thread 0 Crashed:
0   org.mozilla.navigator 	0x002071fc nsDocShell::DoURILoad(nsIURI*, nsIURI*,
int, nsISupports*, char const*, nsIInputStream*, nsIInputStream*, int,
nsIDocShell**, nsIRequest**) + 0x10c
1   org.mozilla.navigator 	0x00206e5c nsDocShell::InternalLoad(nsIURI*, nsIURI*,
nsISupports*, unsigned int, unsigned short const*, char const*, nsIInputStream*,
nsIInputStream*, unsigned int, nsISHEntry*, int, nsIDocShell**, nsIRequest**) +
0xc34
2   org.mozilla.navigator 	0x001fbd00 nsDocShell::LoadURI(nsIURI*,
nsIDocShellLoadInfo*, unsigned int, int) + 0x60c
3   org.mozilla.navigator 	0x00200658 nsDocShell::LoadURI(unsigned short const*,
unsigned int, nsIURI*, nsIInputStream*, nsIInputStream*) + 0x360
4   org.mozilla.navigator 	0x00020624 -[CHBrowserView
loadURI:referrer:flags:allowPopups:] + 0x238
5   org.mozilla.navigator 	0x0000b940 -[BrowserWrapper
loadURI:referrer:flags:activate:allowPopups:] + 0xe0
6   org.mozilla.navigator 	0x00016994 -[BrowserWindowController
goToLocationFromToolbarURLField:inView:inBackground:] + 0xec
7   org.mozilla.navigator 	0x0001c23c -[BrowserWindowController
handleCommandReturn] + 0xfc

Comment 2

[Simon Fraser [no longer active]]

It's crashing in nsDocShell::DoURILoad() because mContentListener is null. I
believe this happens for the Command-Enter case because we haven't already
loaded about:config (as we do for new tabs), so EnsureContentListener() hasn't
been called.

So the fix is either a null check, or an earlier call to EnsureContentListener().

Comment 3

[Boris Zbarsky [:bzbarsky]]

How about we remove EnsureContentListener altogether and just create the
listener in nsDocShell::Init()?  I see no reason we'd ever want a docshell
around that doesn't have a DSURIContentListener; if nothing else, we need one of
those suckers to load anything into the docshell.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so, say

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 174642 [details] [diff] [review]
Like so, say

sr=jst

Comment 6

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

can this be a non-pointer member instead? (would probably need to AddRef it in
the constructor, so nobody calls delete on it)

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's a refcounted XPCOM object, so it needs to be able to outlive the docshell.
  I could make it a non-pointer member if it shares a refcount with the
docshell, but that's dangerous if the parent listener holds a ref to the
docshell (this last is the reason I left the "set mListener to null in destroy"
code too, though come to think of it, I think it's better to null out the parent
listener in destroy and leave mListener around....)

Comment 8

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

Comment on attachment 174642 [details] [diff] [review]
Like so, say

the destructor calls Destroy... if Destroy is also called explicitly, or if new
DSURIContentListener failed, you will now crash

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Address biesi's issues (from IRC too)

Comment 10

[Simon Fraser [no longer active]]

-> Boris

Comment 11

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

Comment on attachment 174697 [details] [diff] [review]
Address biesi's issues (from IRC too)

good catch on the loadcookie thing

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 13

[[e-mail]]

Comment on attachment 174697 [details] [diff] [review]
Address biesi's issues (from IRC too)

>-    NS_ADDREF(*aLoadCookie = NS_STATIC_CAST(nsIDocumentLoader*, mDocShell));
>+    NS_IF_ADDREF(*aLoadCookie = NS_STATIC_CAST(nsIDocumentLoader*, mDocShell));
NS_IF_ADDREF(*aLoadCookie = nsDocShell::GetAsSupports(mDocShell)) ?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, good idea.  Did that.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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