36862 - Clicking on link with unknown protocol causes crash

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Biswapesh Chatterjee]

Info: Checked on builds 2000022215 and 2000041915 on WinNT 4 SP 6 (PII)
Description: Clickiing on a link of type "<protocol>://<Location>" where 
<protocol> is unknown or not implemented (say ldap, https or foo) causes crash 
in necko.dll (necko!nsProxyObjectCallInfo::operator= )
Test Case: Load attached test file in mozilla. Click on a unregistered protocol.
Expected result: "<foo> is not a registered protocol" should be the message in a 
popup box.
Actual Result: Crash.
Other info: Regression. Was working fine a week or two back if I remember 
correctly. probably not mem-cache or proxy related since disabling them has no 
effect.

Comment 1

[Biswapesh Chatterjee]

Attachment: Test Case

Comment 2

[Biswapesh Chatterjee]

Attachment: Stack trace

Comment 3

[David Hallowell]

Tested this bug under M15 (2000041811) under Linux (Slackware 7.0) and the
expected behaviour was observed (a dialogue box saying foo is not a registered
protocol).

I then tried to reproduce this bug with an M16 nightly build (2000042113) and
clicking on a link for an unregistered protocol e.g. foo://127.0.0.1/ causes a
crash, however typing the same URL into the url bar brings up the expected
dialog box rather than crashing the browser. Using drag-n-drop on one of these
invalid links does not crash the browser, only clicking on the link produces the
crash.

Confirming bug and changing platforms to all.

Comment 4

[Sammy Ford]

cc'ing  warren cause he was the last to touch the area with the api change

#0  0x40c6df6f in nsIOService::NewChannelFromURI (this=0x8162658, aURI=0x0,
result=0xbf
ff2dc)
    at nsIOService.cpp:224
#1  0x4004be03 in NS_OpenURI (result=0xbffff3b0, uri=0x0, ioService=0x0,
loadGroup=0x86
3b58,                             

Comment 5

[leger]

Adding crash keyword.

Comment 6

[Andreas Otte]

This is from nsWebShell.cpp:

void
nsWebShell::HandleLinkClickEvent(nsIContent *aContent,
                                 nsLinkVerb aVerb,
                                 const PRUnichar* aURLSpec,
                                 const PRUnichar* aTargetSpec,
                                 nsIInputStream* aPostDataStream)
{
  nsAutoString target(aTargetSpec);

  switch(aVerb) {
    case eLinkVerb_New:
      target.AssignWithConversion("_blank");
      // Fall into replace case
    case eLinkVerb_Undefined:
      // Fall through, this seems like the most reasonable action
    case eLinkVerb_Replace:
      {
        // for now, just hack the verb to be view-link-clicked
        // and down in the load document code we'll detect this and
        // set the correct uri loader command
        nsCOMPtr<nsIURI> uri;
        NS_NewURI(getter_AddRefs(uri), aURLSpec, nsnull);
        nsCAutoString tempTargetSpecCString;
        tempTargetSpecCString.AssignWithConversion(aTargetSpec);
        InternalLoad(uri, mCurrentURI, tempTargetSpecCString, aPostDataStream,
loadLink); 
      }
      break;
    case eLinkVerb_Embed:
      // XXX TODO Should be similar to the HTML IMG ALT attribute handling
      //          in NS 4.x
    default:
      NS_ABORT_IF_FALSE(0,"unexpected link verb");
  }
}

Why is this a void function????? 

We never check the result of NS_NewURI which will fail in case of foo:// ...
For that reason we pass a null uri into InternalLoad. Somewhere down the road we
don't expect a null URI ===> boooommmm!!!

ccing travis as nsWebShell/nsDocShell expert.

Comment 7

[Andreas Otte]

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

Comment 8

[Gagan]

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

Comment 9

[Gagan]

I've added parameter checks to avoid the crashes now, but someone still needs to
look into that function above to clean up and check for error states! cc'ing
rpotts. 

Comment 10

[Tom Everingham]

verified:
NT 2000051808