86486 - [FIX]URL: Invalid protocols in HREF links fail silently

Status: VERIFIED FIXED

(Core :: Networking, defect, P2)

Description

[Gregory McLean]

Attached html presents a link when clicked on does nothing. No error's no
response on the browser's part at all.

View-Source and paste the url in the url bar will generate a popup with 'invaild
protocol httpf'.

Clicking should maybe produce the same?

Linux build 2001061712 from the blizzard rpms.

Comment 1

[Gregory McLean]

Attachment: Simple test case

Comment 2

[Gregory McLean]

Possibly a dup of 46537, if not its in the same family ;)

Comment 3

[benc]

Greg:
changed summary. This is a really good catch, b/c I'm not where I wanted to be
in testing this stuff... I updated bug 46537 too, but I think we should keep
them separate

Gagan: 
since the errors come back correctly in some entry points, isn't this someone
else's code talking to Necko that needs to trap the error?

Comment 4

[Bradley Baetz (:bbaetz)]

If I do open link in new window, I get a security manager assertion:

###!!! ASSERTION: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: '(!((rv) &
0x80000000))', file nsScriptSecurityManager.cpp, line 808
###!!! Break: at file nsScriptSecurityManager.cpp, line 808
JavaScript error: 
 line 0: uncaught exception: Load of httpf://www.cnn.com/ denied.

This is because the call to NS_NewURI failed, and something is probably turning
a CAPS failure into a security exception. mstoltz, does that sound right?

I don't know if thats the same cause when just clicking on a link, though. I
don't get that assertion, and don't hit the one at the bottom of CheckLoadUri
either.

Comment 5

[neeti]

In nsGenericElement::TriggerLink(..) we return an error from NS_NewURI(..). We 
then do a CheckLoadURI(..) only if rv succeeds. 

proceed = securityManager->CheckLoadURI(..)

Now, since "proceed" is already initialized to NS_OK,  we call OnLinkClick(..)
// Only pass off the click event if the script security manager
// says it's ok.
    if (NS_SUCCEEDED(proceed))
      handler->OnLinkClick(this, aVerb, absURLSpec.GetUnicode(),
                           aTargetSpec.GetUnicode());

Comment 6

[Hixie (not reading bugmail)]

Well, clicking on a link which is insecure should _also_ give an error.

e.g., if in a web page there is a link to about:cache, then we should report an
error if the user clicks on it.

See also bug 28586, which  is complaining about the fact that error messages should
be inline (as a web page) instead of dialogs.

Comment 7

[Jesse Ruderman]

See also bug 47617, Connection to https needs to tell user to install PSM if 
w/o.

See also bug 84128, Need user-visible message when CheckLoadURI fails (eg, for 
links to file: urls).

Comment 8

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 9

[benc]

+mozilla1.0 - please bring this back to 1.0

Comment 10

[Johnny Stenback (:jst)]

Why would this need to be fixed for mozilla1.0? It doesn't break anything as
long as people do the right thing, it only breaks in cases where we'd break anyway.

Comment 11

[Andrew Hagen]

This should be fixed ASAP. It's a user or web manaager problem, but Mozilla
should at least say that "httpf" or "zzvkjdfr" is an invalid protocol. This kind
of error handling would tell the user that Mozilla is still working, but the
link itself is broken.

Comment 12

[Andrew Hagen]

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

Comment 13

[Andrew Hagen]

Is this bug a dupe of bug 62744?

Comment 14

[Adam Hauner]

Plat: PC --> All, OS: Linux --> All 
Removing URL http://tweetie.comstar.net/test.html, because it is 404.

Comment 15

[Johnny Stenback (:jst)]

Andrew, this is not a dup of bug 62744.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Darin, was this fixed by your checkin to bug 47617 (Connection to https needs to
tell user to install PSM if w/o) ?  Seems that you left the error message as
generic for this purpose.

Comment 17

[Darin Fisher]

the fix for bug 47617 only handles toplevel document loads that fail with
NS_ERROR_UNKNOWN_SOCKET_TYPE.  that doesn't apply here.

Comment 18

[Andrew Hagen]

It's fixed. I don't know when, but as of 0.9.7, at the latest, worksforme on
Windows 98. 

Comment 19

[Andrew Hagen]

Now using 0.9.7 Build ID: 2002011003, it's broken again.

Comment 20

[Aaron Kaluszka]

I don't know if this is covered elsewhere, but data:foo doesn't work either.
(should be data:text/html,foo)

Comment 21

[benc]

data: has it's own set of bugs. please search Browser:Networking. Properly
written bugs should have "data:" as start of the summary.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

mine

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Move the error reporting down into DoURILoad

Comment 25

[Darin Fisher]

Comment on attachment 106899 [details] [diff] [review]
Move the error reporting down into DoURILoad

so, you're moving the error "trap" into DoURILoad because we only care about
alerting the user about errors to create a channel?1?  i'm not sure i follow.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Right now we have the error trap in a few places:

nsWebShell::EndPageLoad
nsDocShell::IsPrintingOrPP (this one doesn't have much to do with pageload)
nsDocShell::LoadURI (the version that takes a PRUnichar* uri)

I'm taking this last check and moving it down so that callers of both versions
of nsDocShell::LoadURI (the PRUnichar* version and the nsIURI version) as well
as direct callers of nsDocShell::InternalLoad will benefit from it.  I'm leaving
the check for NS_ERROR_MALFORMED_URI in nsDocShell::LoadURI (PRUnichar* version)
because that's the only place it's relevant.  So now we report the following errors:

1)  If we start with a string, we will report NS_ERROR_MALFORMED_URI as needed
2)  Once we have a URI, we will report any error in attempting to create a
    channel (that's a recognized error as far as DisplayLoadError is concerned)
3)  Once we have a channel, I suppose we could have errors in trying to open it.
    In fact, we have some code there to deal with port blocking (see
    nsDocShell::DoChannelLoad).  Perhaps it would be better to put the error
    report around the entire call to DoURILoad in nsDocShell::InternalLoad
4)  Once the channel is open, we will get state change notifications via our
    nsIWebProgressListener interface so we will call EndPageLoad() and its error 
    handling.

So we only need to handle errors up to the point when we have an open channel...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: So more like this.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Actually, I should check the return value from the OnStartURIOpen call. 
Consider that fixed; it is in my local tree (using a different nsresult variable
than rv so that I don't clobber rv), like so:

            PRBool abort = PR_FALSE;
            nsresult rv2 = mContentListener->OnStartURIOpen(aURI, &abort);
            if (NS_SUCCEEDED(rv2) && abort) {
                // Hey, they're handling the load for us!  How convenient!
                return NS_OK;
            }

Comment 29

[Darin Fisher]

Comment on attachment 106921 [details] [diff] [review]
So more like this.

yeah, that makes a lot more sense to me.  thanks!  assuming adam is okay with
this patch, sr=darin.

Comment 30

[Adam Lock]

Comment on attachment 106899 [details] [diff] [review]
Move the error reporting down into DoURILoad

Shouldn't that DisplayLoadError go inside the NS_ERROR_UNKNOWN_PROTOCOL block?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Adam, did you read the last patch or the previous one?  Did you read Darin's
comments?  The idea here is that we want to catch all errors from DoURILoad and
let ReportLoadError decide whether it wants to report them.

Comment 32

[Adam Lock]

Comment on attachment 106921 [details] [diff] [review]
So more like this.

Sorry, was looking at a stale copy of the bug. r=adamlock

Comment 33

[Boris Zbarsky [:bzbarsky]]

All good.  ;)  Patch checked in; thanks for the reviews.

Comment 34

[benc]

VERIFIED: Mozilla 1.3a, Win 98.

If anyone wants to verify for Linux and Mac before I do, click on the URL field
label, and then remove the comment for the plaform you tested on...

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

Comment 36

[benc]

VERIFIED:
Mac OS X, Linux - Mozilla 1.3a.

Comment 37

[Andrew Hagen]

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