176904 - URL: non-ascii absolute URIs with unknown scheme can fail

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

I was looking at bug 166996 and found that the code in
nsWebShell::OnLinkClickSync absolutely depends on being given a _string_ rather
than a URI because embeddors may want to handle clicks on <a href="foopie:bar">
where "foopie" is not a protocol we recognize...

Now this works fine for ascii hrefs, but non-ascii ones currently go through a
URI to become properly absolute-ified.  And this breaks things.  Testcase coming up.

So the upshot is, either we need a way to create uris even when we have no idea
what the protocol is (this seems fishy, for one thing how does one make an
absolute URI if the base is an unknown protocol?  Just append?) or we need to
rethink the way we do those encoding conversions and whatnot....

Comment 1

[Boris Zbarsky [:bzbarsky]]

to darin...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase #1

In addition to the click behavior, watch the status bar.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Should we really be ignoring the base url like that?

Comment 4

[Christopher Hoess (gone)]

It seems to me that the rules in RFC 2396 should let us work with unknown URI
schemes in <base>. Any URI in the <base> element is implicitly required to be a
hierarchical URI and can be decomposed accordingly.

Comment 5

[Darin Fisher]

shouldn't the nsExternalProtocolHandler's NewURI be able to handle this? 
remember: there is always a registered protocol handler.  perhaps we just need
to refine the rules for nsExternalProtocolHandler.

so, i my mind, this is a uriloader/exthandler bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Well, now... This is what the code in the ExternalHelperAppService does:

334     uri->SetSpec(aSpec);
335     PRBool haveHandler = HaveProtocolHandler(uri);
336 
337     if (haveHandler)

So if HaveProtocolHandler() is false (which happens whenever
ExternalProtocolHandlerExists() for that protocol is false), this function
errors out....

Perhaps it should be succeeding unconditionally?  I'm not sure what the contract
is here, but there is no way for embeddors to affect what
ExternalProtocolHandlerExists() returns at the moment; and some of our internal
uses (eg display in the status bar) currently rely on strings because the href
could be completely unhandled by anything....

Maybe we should make the NewURI always succeed in this case and make sure no one
 interprets that as being synonymous with having an external protocol handler
for that protocol?

Comment 7

[Darin Fisher]

right, i agree.  nsExternalProtocolHandler::NewURI should always succeed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  Lemme see here...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: bare minimum

Just wanted to attach this so I wouldn't forget nsTreeBodyFrame later...  not
for review or anything

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: More or less complete

This converts the api and the callers; the only outstanding issues are testing
and this pesky NS_NewURI failure in the nsGenericElement code....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: ready for review

I've left the issue of whether TriggerLink should be taking a uri or not for
bug 166996 for now; I'll need to do some perf testing on those changes.

This patch preserves our behavior for the "unknown protocol" alert, though we
may want to decide what the deal is there -- if an embeddor handles the load
off of OnStartURIOpen, should we put up the alert?

Comment 12

[Darin Fisher]

Comment on attachment 105419 [details] [diff] [review]
ready for review

>Index: content/base/src/nsGenericElement.cpp

>+  if (mDocument &&
>+      NS_SUCCEEDED(mDocument->GetDocumentCharacterSet(docCharset)) && 0) {

"&& 0" ??  is that just for testing?


>Index: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

>+    if (!imageRequest)
>+      return NS_ERROR_FAILURE;
...
>     imageRequest->GetImage(aResult);

nice catch!


>Index: layout/html/forms/src/nsIsIndexFrame.cpp

>+    nsCOMPtr<nsIURI> uri;
>+    result = NS_NewURI(getter_AddRefs(uri), href, nsnull, docURL);

hmm... should we pass in the document charset here?  have you uncovered
an intl bug?


>Index: docshell/base/nsWebShell.cpp

> struct OnLinkClickEvent : public PLEvent {
...
>   nsWebShell*     mHandler;
>+  nsCOMPtr<nsIURI> mURI;
>   nsString       mTargetSpec;
>   nsCOMPtr<nsIInputStream> mPostDataStream;
>   nsCOMPtr<nsIInputStream> mHeadersDataStream;
>   nsCOMPtr<nsIContent>     mContent;
>   nsLinkVerb      mVerb;

nit: how about lining up the variable names for readability while
you're at it.

nice cleanup in this file btw :-)


>Index: docshell/base/nsDocShell.cpp

>+    if (NS_ERROR_MALFORMED_URI == rv) {
>         DisplayLoadError(rv, uri, aURI);
>+    } // end malformed uri protocol

>+    if (NS_ERROR_UNKNOWN_PROTOCOL == rv) {
>+        DisplayLoadError(rv, uri, aURI);
>+    } // end unknown protocol

nit: the comments seem a bit verbose here.  also, neighboring code
appears to leave off braces when possible.


>Index: uriloader/exthandler/nsExternalProtocolHandler.cpp

>   nsresult rv = NS_ERROR_UNKNOWN_PROTOCOL;
>   nsCOMPtr<nsIURI> uri = do_CreateInstance(kSimpleURICID, &rv);
>   if (uri)
>   {
>     uri->SetSpec(aSpec);
>+    NS_ADDREF(*_retval = uri);
>+    return NS_OK;
>   }
> 
>+  return rv;
> }

nit: how about this instead:

{
  nsresult rv;
  nsCOMPtr<nsIURI> uri = do_CreateInstance(kSimpleURICID, &rv);
  if (NS_FAILED(rv)) return rv;

  uri->SetSpec(aSpec);
  NS_ADDREF(*_retval = uri);
  return NS_OK;
}

Comment 13

[Boris Zbarsky [:bzbarsky]]

> "&& 0" ??  is that just for testing?

er, yes.  :)  Thank you for catching that...

The isindex thing probably is an intl bug yes.  There may be similar ones
elsewhere in the code I touched; I'll check.  Thanks for the comments; I'll try
to post a new patch soonish.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: address the comments

Comment 15

[Darin Fisher]

Comment on attachment 105561 [details] [diff] [review]
address the comments

r/sr=darin

Comment 16

[Boris Zbarsky [:bzbarsky]]

One thought... perhaps that alert for unknown protocol should be put up if and
only if the OnStartURIOpen says that we should keep handling it?  That is, if
the bool it returns is "true", and the protocol is unknown, we put up the alert.
 If it's "false", we just pretend the load succeeded and return NS_OK (it seems
like we should do that anyway, in that case).

Thoughts?  That would fix "no error when clicking on link with unknown protocol"
bug...

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 105561 [details] [diff] [review]
address the comments

- In nsGenericElement::LeaveLink()

+  nsresult rv = aPresContext->GetLinkHandler(getter_AddRefs(handler));
+  if (NS_FAILED(rv) || (nsnull == handler)) return rv;

How about this in stead?

+  if (NS_FAILED(rv) || !handler)
+    return rv;


- In nsGenericElement::TriggerLink():

 nsGenericElement::TriggerLink(nsIPresContext* aPresContext,
			       nsLinkVerb aVerb,
			       nsIURI* aBaseURL,
-			       const nsString& aURLSpec,
-			       const nsString& aTargetSpec,
+			       const nsAString& aURLSpec,
+			       const nsAString& aTargetSpec,

Wouldn't nsAFlatString fit better here? Then you wouldn't need all those
PromiseFlatString()'s.

- In nsDocShell.cpp:

-    if (NS_ERROR_UNKNOWN_PROTOCOL == rv ||
-	 NS_ERROR_MALFORMED_URI == rv) {
+    if (NS_ERROR_MALFORMED_URI == rv)
	 DisplayLoadError(rv, uri, aURI);
-    } // end unknown protocol

I'm not the owner of this code, but I really don't like seeing braces taken out
around one-line if's, it's one thing if you don't want to write them yourself,
but please don't take them out. We had a smoketest blocker a month ago or so
(remember?) due to someone making a mistake with an if that didn't have braces.
They do IMO serve a good purpose, that's why I always put them in, even if the
language doesn't require them.

...
+	 if (rv == NS_ERROR_UNKNOWN_PROTOCOL) {
+	     // This is a uri with a protocol scheme we don't know how
+	     // to handle.  Embedders might still be interested in
+	     // handling the load, though, so we fire a notification
+	     // before throwing the load away.
+	     PRBool abort;
+	     mContentListener->OnStartURIOpen(aURI, &abort);
+
+	 }

Empty line before closing brace?

Other than that, r/sr=jst

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: patch that addresses jst's comments

Comment 19

[Boris Zbarsky [:bzbarsky]]

Filed bug 181008 on the remaining issue with the unknown protocol alert. 
Checked in.

Comment 20

[Jean-Francois Ducarroz]

The change in nsExternalProtocolHandler.cpp for this fix has caused bug 181127. 

Comment 21

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

for completeness, bz said this exposed (not really caused) #191248, too.

see http://bugzilla.mozilla.org/show_bug.cgi?id=191248#c5

Comment 22

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

caused bug #183111, too.

Comment 23

[Boris Zbarsky [:bzbarsky]]

And bug 185169

Comment 24

[benc]

I'm working on verifying this. Can I just check the two testcases provided
against a current build?

also, is the depends on 181127 supposed to be that way?

Comment 25

[Boris Zbarsky [:bzbarsky]]

First, the summary as now written is incorrect.  The problem was that the URL 
string was not treated as a URL at all (silent failure), not that it was not 
treated as an absolute URL.  Kindly restore the original summary, which 
actually described the problem...

Yes, just testing the testcases in this bug in a current build and seeing 
whether the behavior makes sense would work.

Yes, the dependency is correct -- that bug was a regression caused by this 
patch; hence this bug cannot be considered fixed until that one is.

Comment 26

[benc]

a compromise summary that mostly makes sense b/c bz is a patient guy.