We found out that there are developers out there who have external apps that act as protocol handlers (i.e. non-necko protocols), and the uriloader dispatches them just fine, but they still end up having to install a necko protocol handler just so that anchor tags (href=...) work. This is because we don't highlight the anchor unless NS_NewURI succeeds, and NS_NewURI only succeeds if necko knows of a registered protocol handler for that protocol. I think the right thing is that all anchor tag hrefs should just get highlighted, and we should decide at runtime whether clicking on it should succeed or fail. In the case that the uriloader dispatches it externally, then they won't need to register a dummy necko protocol handler

Reassigning to attinasi - Triaging Troy's bug list

I am not sure I understand exactly what this is about... I created a test page with a link that specifies an 'unsupported' (well, actually non-existent) protocol and the link is styled correctly, as a link - I'll attach the testcase. If I am missing the boat on this, please educate me. There are about 42 occurrances of NS_NewURI in layout and I don't have any idea which ones this bug is about... Maybe it was already fixed?

I'm still waiting for somebody to tell me what the problem is here, to see if we still have a problem. As I mentioned, I am not sure this is even a problem as my testcase ssems to indicate we handle unknown protocols just fine... Please advise, or I may have to mark this invaild...

I described the problem already. What else do you need to know?

This is no longer a problem since Chris backed out his changes that were using NS_NewURI to get the canonical form of the URI for history lookup. Marking FIXED (Thanks for the clarifcation, Warren)

Marking verified per last comments.

this is still a problem. to test it, create a html doc w/ a bogus href (<a href="foobar://blah">foobar://blah</a>), and set a breakpoint in mozilla/docshell/base/nsDSURIContentListener.cpp > nsDSURIContentListener::OnStartURIOpen, click on that bogus href and you assert to death because the URI is never created and never hit the breakpoint.

OK, I see the assertions. Updating the testcase too since the strict DTD landing has made if not work (it was malformed).

related to 48270?

Marking Future because it does not seem critical to Beta3/RTM. Please advise if you disagree.

actually this is critical for embedding. embeddors need the ability to push their own protocol schemes into content. with the way things are currently, if the do this they can never capture that scheme because NewURI fails and we stop processing. We need to let necko decide if the processing should stop (via the uriloader) which will allow embeddors to capture their scheme loads via OnStartURILoad().

Approving for beta3 since Judson says it is critical.

It's not as easy as having Layout avoid NS_NewURI. Even if we get past layout (which I believe is limited to nsGenericElement::TriggerLink) there is code in nsWebShell::HandleLinkClickEvent that uses NS_NewURI to create a URI to pass to nsDocShell::InternalLoad, and that code requires an nsIURL instance be passed in. Is there another way to create an object that implements the nsIURI interface so we can pass it to the nsDocShell::InternalLoad method? If so, then this should be easy to fix. If not, then I cannot see how to address the problem, which really is not a layout problem but a DocShell/WebShell problem. Adding the helpwanted keyword and CC'ing waterson because this is not an area I am familiar with at all. Note also that in nsGenericElement::TriggerLink we actually ignore the failure of NS_NewURI and go ahead and proceed with calling the linkHandler->OnLinkClick method, which may be a security violation since the security manager is bypassed (should this be fixed or is it correct to bypass the security manager when NS_NewURI fails?)

Also, in nsDocShell::DoURILoad there is a check against mUseExternalProtocolHandler before external protocol handlers will be used, and thati seems to always be false - what sets that to true? Just the pref?

Hey Marc, you should ignore that code. It's only used by shrimp (which doesn't have a "browser") where we want all link clicks to go to the default protocol handler. No one in mozilla or the seamonkey browser sets that pref to true.

Thanks Scott. The reason I'm looking at this at all is because the embedding folks feel this needs to be fixed for embedding clients, not for SeaMonkey. Now I'm curious how this is used for Shrimp: does it have an external protocol handler that it uses, and if so how does it create the URI for the non-standard protocol?

Another plea for assistance: how do you create an object that implements nsIURI without calling NS_NewURI??? nsDocShell::InternalLoad needs and nsIURI and it has to support non-standard protocols, so how does one create such a beast?

What is the specific code in question? (Looks like maybe the code that's trying to get the anchor's state?) I'm not sure that calling NS_NewURI() is bad if we deal with the case that it might not give us back something...

Mark: The issue here is not that you're using the success or failure of NS_NewURI to determine whether to highlight the link or not. That's what's wrong -- it's not that you can't be calling NS_NewURI. The reason it's wrong to not highlight links when NS_NewURI fails is because NS_NewURI only works for installed necko protocols, but the doc loader can in some cases delegate the url load to an external app. Right now, you can't click on those links because we're not highlighting them, and making them clickable. If you made all href links clickable, then the doc loader would do the right thing.

From what I see there is no layout problem with links that have non-standard protocols in their href. A link like <a href='soap://suds.soap'>Link</a> is both styled like a link and can be clicked like a link - from layout's perspective it is a link. Where I start seeing problems is after the nsGenericElement::TriggerLink method calls out to the link handler's OnLinkClick method. This is actually the WebShell::OnLinkClick method, and that ends up calling (indirectly) to WebShell::HandleLinkClickEvent, which uses NS_NewURI to create an nsIURI to pass to InternalLoad, however the nsIURI is null (never checked in WebShell::HandleLinkClickEvent) and that causes loads of grief down the link handler call chain. If you put a breakpoint in nsGenericElement::TriggerLink (in the aClick case) and follow it through the link handler you will see what I am writing about. The problem looks like it starts in the WebShell, so I am assigning this over to the webshell owner to look at. If this is the wrong resource to deal with issues in WebShell::HandleLinkClickEvent then please send it back or on to the correct resource. Also, I'm updating the misleading summary to be more reflective of the problem.

->adam

Isn't this an mscott bug?

Would it be acceptable to modify the implementation of nsIOService::NewURI() which NS_NewURI() calls to create a URI object directly when there is no protocol handler associated with the scheme? Presumably the only reason the protocol handler is given the chance to create the URI is in case it wants to fix it up.

the whole idea of the necko created uri is that the protocol handlers drive object creation.

If there *must* be a handler to create a URI then surely anyone who wants to introduce their own protocols will have to write their own handler as well? Once they do that their problem is solved. Is this bug trying to suggest that embedders shouldn't have to do that? I do think the current protocol handling scheme is too inflexible. Read what you can do with IE: http://msdn.microsoft.com/workshop/networking/pluggable/overview/overview.asp

no. There's a difference between capturing a click on something that should be "clickable," versus providing a protocol handler implementation. layout should figure out what is "clickable" while necko should figure out what protocol handlers are.

So when the docshell can't turn the spec into a URI, the embedding app should be notified to figure out what to do with it?

exactly. nsIURIContentListener::OnStartURI should get fired (w/ a homegrown nsSimpleURI object) when it gets that far into notification.

2 points: - you can always instantiate a "standard URL" directly via the component mgr and NS_STANDARDURL_CID (if you want "standard" URL syntax parsing). That's what protocol handlers to that want that syntax. - I think nsIURIContentListener::OnStartURI should take a string, not a simple URI object.

Adding Jussi in case we want to consider a change in the 4.x codebase in this area.

Huh? 4.x already does this right.

r=valeski. the bug is already approved for checkin (via the nsbeta3+ moniker)

Fix checked in

Not exactly sure what I need to verify here. Warren could you please verify since you are the reporter ?

Verification is to load http://bugzilla.mozilla.org/query.cgi and click the "AND" button at the bottom. Verify that the page is reloaded with the extra conditions. Verify that other pages containing normal anchors, e.g. http://www.iol.ie/~locka/mozilla/mozilla.htm still scroll correctly without doing an uneccessary reload.

adam commented in the wrong bug, disregard the above comment.

Marking verified in the Sept 14th build.