262222 - Relative URLs in Live Bookmark link elements not resolved

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Phil Ringnalda (:philor)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040928 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040928 Firefox/0.10

If a <link> element in an Atom or RSS feed contains a relative URL, Live
Bookmarks doesn't attempt to resolve the absolute URL, it just tries to open the
relative URL as-is.

In Atom, the situation is clear-cut: xml:base processing is required for
relative URLs, so you just walk up the tree, gathering path parts, until you hit
an xml:base attribute with a scheme and host, or until you get to the top
without, when you use the feed URL as a base.

In RSS, it's fuzzier: the specs all predate xml:base, and fail to mention
anything that should be used as a base, so RFC 2396 says you should use the feed
URL, but because most people using <img src="images/me-on-pony.jpg"> mean
/blog/images/ rather than /xml/images/, aggregators traditionally use the
channel/link (the feed "home page") as a base URL, rather than using the feed
URL, for resolving all relative URLs, not just in content. Correct, or work
right? Dunno.

Reproducible: Always
Steps to Reproduce:
1. Add the above URL as a Live Bookmark.
2. Select any item.
3. Pout.

Actual Results:  
Browser attempted to load a page from the site www.1852.html

Expected Results:  
Load www.intertwingly.net/blog/1852.html

Comment 1

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20040930
Firefox/0.10

Confirming

->NEW

Comment 2

[Phil Ringnalda (:philor)]

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

Comment 3

[Sam Ruby]

Attachment: Resolve relative URIs in Atom feeds

While I know both C++ and DOM, and MS-COM is familiar to me, these string
classes are definitely new to me, so a review is clearly needed.  I've run
various tests, and it appears to work, but I have no idea of I'm leaking memory
or the like.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 190753 [details] [diff] [review]
Resolve relative URIs in Atom feeds

r=vladimir, looks fine to me.. I don't like having all the
UCS2toUTF8/UTF8toUCS2 conversions, but there's no way around them (unless a
string guru has any pointers).. this code should be fine though.

Comment 5

[Sam Ruby]

What's the next step?  I am not a committer (nor do I feel confident enough in
my understanding of the Mozilla code base to desire to be at this time).

I do feel that relative URIs will be common in Atom 1.0 feeds.

Once this issue is resolved, I plan to look into providing a patch for bug 302133.

Comment 6

[Sam Ruby]

Comment on attachment 190753 [details] [diff] [review]
Resolve relative URIs in Atom feeds

Asking for 1.8b4 approval since this gets us most of the way to Atom 1.0
support, and Atom should be getting IETF approval any day now.

Comment 7

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

Comment on attachment 190753 [details] [diff] [review]
Resolve relative URIs in Atom feeds

+static NS_DEFINE_CID(kStdURLCID,		   NS_STANDARDURL_CID);

eh. not only should you not create nsIURI implementations via createInstance
(with very few exceptions), you should also prefer contract ids.

especially:
+					 base =
nsCOMPtr<nsIURI>(do_CreateInstance(kStdURLCID, &rv));
+					
base->SetSpec(NS_ConvertUCS2toUTF8(baseURI));

this should use the IO service, not everything is a standardurl.

+					     linkStr =
NS_ConvertUTF8toUCS2(result);

CopyUTF8toUTF16(result, linkStr);

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This should probably be backed out.  Mistakes like that (the first 2 parts of
comment 7) really shouldn't be in the tree, and if they're getting past review,
then this part of the code needs a superreview requirement.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

I've backed out the patch.

Comment 10

[Darin Fisher]

Comment on attachment 190753 [details] [diff] [review]
Resolve relative URIs in Atom feeds

>Index: nsBookmarksFeedHandler.cpp

>+ base = nsCOMPtr<nsIURI>(do_CreateInstance(kStdURLCID, &rv));

NO!  Never allocate a StandardURL unless you are implementing
nsIProtocolHandler :-)


>+ rv = base->Resolve(NS_ConvertUCS2toUTF8(linkStr), result);

If linkStr is of a different URL type than base, then
you should not be passing it to Resolve.  You should
really use nsIIOService::NewURI instead.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Bah, now I know why this code felt longer than it needed to be.  2 calls to
NewURI and a GetSpec should do it...  Sorry for the crappy review =/

Comment 12

[Sam Ruby]

Comment on attachment 190753 [details] [diff] [review]
Resolve relative URIs in Atom feeds

shot down in flames

Comment 13

[Sam Ruby]

Attachment: streamlined patch

No do_CreateInstance, No NS_ConvertUCS2toUTF8, No NS_ConvertUTF8toUCS2.

In fact, it looks like I can do better than two calls to NewURI and a GetSpec.

How about one NS_NewURI, and one NS_MakeAbsoluteURI.

Who wants to review it?

Comment 14

[Sam Ruby]

Comment on attachment 191052 [details] [diff] [review]
streamlined patch

nudge this forward

Comment 15

[Darin Fisher]

Comment on attachment 191052 [details] [diff] [review]
streamlined patch

>+                                        nsAutoString baseURI;
>+                                        linkElem3->GetBaseURI(baseURI);
>+                                        nsCOMPtr<nsIURI> base;
>+                                        NS_NewURI(getter_AddRefs(base), nsDependentString(baseURI));

There's no reason to wrap a nsAutoString with a nsDependentString.
You can just pass the baseURI to NS_NewURI without wrapping it.

Also, I noticed that you are not handling errors anywhere.  Are
you sure this code works properly if say |base| could not be
allocated?  Or, what if something else couldn't be allocated?

Comment 16

[Sam Ruby]

Attachment: nsDependentString gone; rv checked everywhere

Comment 17

[Darin Fisher]

Comment on attachment 191769 [details] [diff] [review]
nsDependentString gone; rv checked everywhere

>+                                    nsCOMPtr<nsIDOM3Node> linkElem3 = do_QueryInterface(childNode);
>+                                    if (linkElem3) {
>+                                        // get the BaseURI (as string)
>+                                        nsAutoString base;
>+                                        rv = linkElem3->GetBaseURI(base);
>+                                        if (NS_FAILED(rv)) break; // out of while(childNode) loop
>+
>+                                        // convert a baseURI (string) to a nsIURI
>+                                        nsCOMPtr<nsIURI> baseURI;
>+                                        rv = NS_NewURI(getter_AddRefs(baseURI), base);
>+                                        if (NS_FAILED(rv)) break; // out of while(childNode) loop
>+
>+                                        // use baseURI to resolve linkStr to an absolute URI
>+                                        rv = NS_MakeAbsoluteURI(linkStr, linkStr, baseURI);
>+                                        if (NS_FAILED(rv)) break; // out of while(childNode) loop
>+                                    }

Sorry for not noticing this earlier, but it occured to me that you
should use nsIContent directly instead of using nsIDOM3Node, like so:

  nsCOMPtr<nsIContent> linkContent = do_QueryInterface(childNode);
  if (linkContent) {
    nsCOMPtr<nsIURI> baseURI = linkContent->GetBaseURI();
    if (baseURI) {
      nsString absLinkStr;
      if (NS_SUCCEEDED(NS_MakeAbsoluteURI(absLinkStr, linkStr, baseURI)))
	linkStr = absLinkStr;
    }
  }

Doing this avoids a bunch of string conversions and it also avoids the
need to access the nsIDOM3Node interface, which is itself less efficient
to access.

Because you already failover to just using linkStr as the value when
the childNode does not QI to nsIDOM3Node, I decided to just make all
errors non-fatal in a similar way.  This seems like a reasonable 
solution to me.

I also chose not to pass linkStr as both the "in" and "out" params to
NS_MakeAbsoluteURI so that I would still have linkStr if I could not
create an abs URI for some reason.

Comment 18

[Sam Ruby]

(In reply to comment #17)
> 
> Sorry for not noticing this earlier, but it occured to me that you
> should use nsIContent directly instead of using nsIDOM3Node

When I try to add '#include "nsIContent.h"', I get

  nsIContent.h: No such file or directory

I'm sure I can fix this, but it would likely involve additional dependencies
between components.  Is this the right way to go?

Comment 19

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

hmm, if such a dependency is OK, would nsILink work? It has a convenient
GetHrefURI method.

Comment 20

[Sam Ruby]

(In reply to comment #19)
> hmm, if such a dependency is OK, would nsILink work? It has a convenient
> GetHrefURI method.

By default nsILink.h is also not visible:

  nsILink.h: No such file or directory

Also, the fact that an interface exists does not imply that any given object
actually implements that interface.

Comment 21

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

needs REQUIRES=content (like nsIContent). Oh... this is probably not in the
XHTML namespace. so, nevermind. (why does that file not use namespace-aware
methods, for example for checking the type of nodes? does RSS not use namespaces?)

Comment 22

[Phil Ringnalda (:philor)]

(In reply to comment #21)
> does RSS not use namespaces?

An RSS item can be in no namespace or one of two different namespaces, all three
(or nine, or eleven, depending on how you count them) with subtly different
mostly unwritten semantics. Danger, rathole ahead. Run away.

Comment 23

[Robert Sayre]

Attachment: use nsIContent directly instead of using nsIDOM3Node, per Darin

Comment 24

[Darin Fisher]

FYI, I believe that the Livemarks feature is currently being ported to the new bookmarks infrastructure.  See bug 317837.  It is possible that this patch may conflict.

Comment 25

[Darin Fisher]

Comment on attachment 204853 [details] [diff] [review]
use nsIContent directly instead of using nsIDOM3Node, per Darin

r=me, but you ought to get a module owner or peer to review this as well.

also, please be sure to sync up with annie before landing this as it may cause conflicts.  thx!

Comment 26

[Phil Ringnalda (:philor)]

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

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #17)
> Sorry for not noticing this earlier, but it occured to me that you
> should use nsIContent directly instead of using nsIDOM3Node, like so:

I'm not sure if we really have a policy on this, but I sort of prefer it the other way around, especially since app-level code tends to be copied into extensions, etc.
I'm not sure if "being a XULRunner app" will require anything about not using internal Gecko interfaces.

Comment 28

[Benjamin Smedberg]

nsIContent will be unavailable to xulrunner apps for various linkage reasons (deCOMtamination means that nsIContent is no longer a real interface). nsIDOM3Node is the good way here.

Comment 29

[Phil Ringnalda (:philor)]

(In reply to comment #27)
> I'm not sure if we really have a policy on this

Given that we've already lost one potential contributor thanks to whiplash in this bug, having a policy would probably be a good thing.

Comment 30

[Robert Sayre]

Attachment: Use nsIDOM3Node

OK, now we have both choices.

Comment 31

[Darin Fisher]

> nsIContent will be unavailable to xulrunner apps for various linkage reasons
> (deCOMtamination means that nsIContent is no longer a real interface).
> nsIDOM3Node is the good way here.

What linkage problem are you referring to?  The methods on nsIContent are either virtual or inlined.  The inlined methods that call other methods call virtual methods (on mNodeInfo).  So, there is no linking to XUL.dll to be concerned with.  Moreover, I checked and there is another use of nsIContent under mozilla/browser, which is another reason why I suggested nsIContent (not only is it more efficient to use it, but there is already convention for its use).

The most compelling argument to avoid nsIContent here is probably what dbaron said about others copying code from mozilla/browser.  So, yeah... let's go with nsIDOMNode3 ;-)

Comment 32

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

I think we really need a freezable way to get an nsIURI for link targets, document URIs, and base URIs. but ok... needn't be done in this bug.

Comment 33

[Benjamin Smedberg]

> What linkage problem are you referring to?  The methods on nsIContent are

See the header references here:
http://lxr.mozilla.org/mozilla/source/content/base/public/nsIContent.h#51
and #84

Basically if you're using the frozen API you can't even include nsIAtom.h (referenced through nsINodeInfo.h) because many linkers resolve inline methods "early" even when they're not used.

Comment 34

[Darin Fisher]

> Basically if you're using the frozen API you can't even include nsIAtom.h
> (referenced through nsINodeInfo.h) because many linkers resolve inline methods
> "early" even when they're not used.

blech... ok ;(

Comment 35

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 205775 [details] [diff] [review]
Use nsIDOM3Node


Looks fine to me, with a few nits:

>+                                        if(NS_SUCCEEDED(rv) && !base.IsEmpty()){

space after if, and in between ) {

>+                                            // convert a baseURI (string) to a nsIURI
>+                                            nsCOMPtr<nsIURI> baseURI;
>+                                            rv = NS_NewURI(getter_AddRefs(baseURI), base);
>+                                            if(baseURI){

same thing

>+                                                nsString absLinkStr;

nsAutoString

>+                                                if (NS_SUCCEEDED(NS_MakeAbsoluteURI(absLinkStr, linkStr, baseURI)))
>+                                                    linkStr = absLinkStr;
>+                                            }
>+                                        }
>+                                    }

The code flow looks fine to me, but if possible get darin or someone who knows the DOM better than me to take a glance at it (because as we've seen, I'm not the best person for that type of wrangling :).

Comment 36

[Robert Sayre]

Attachment: fix formatting

Comment 37

[Darin Fisher]

> >+                                                nsString absLinkStr;
> 
> nsAutoString

vlad: actually, nsString is better because absLinkStr is assigned to linkStr.  nsString allocates a sharable heap allocated buffer, so it can be cheaply assigned into linkStr.  If absLinkStr were a nsAutoString, then its data may live on the stack, and it would then need to be copied into linkStr (two memcpys instead of one).

Comment 38

[Darin Fisher]

Comment on attachment 206186 [details] [diff] [review]
fix formatting

sr=darin w/ nsString for absLinkStr.

Comment 39

[Darin Fisher]

Attachment: patch that was checked in

Comment 40

[Darin Fisher]

fixed-on-trunk

Comment 41

[Robert Sayre]

Attachment: screenshot of 1.5 breakage

testcase: http://tbray.org/ongoing/ongoing.atom

Comment 42

[Robert Sayre]

Summary of the changes in the fix: 
Set the parser's baseURI, evaluate links relative to nsIDOM3Node's baseURI.

Risk assessment: low. Most links are unaffected, RSS is unaffected, and the diff is small.

A reproducible testcase: See #41.

l10n impact: None.

Comment 43

[Scott MacGregor]

nominating per some comments by Robert.

Comment 44

[Mike Connor [:mconnor]]

This has already been ported to Places (which will be used for bookmarks in Firefox 2) by gavin in Bug 320974.  See http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp#708

Comment 45

[Robert Sayre]

Nominating this for the branch, since the error message is misleading, the fix is low-risk, and a few other web content fixes are already on the branch (e.g. bug 109176).

Comment 46

[Daniel Veditz [:dveditz]]

Comment on attachment 206190 [details] [diff] [review]
patch that was checked in

a=dveditz for drivers

Comment 47

[:Gavin Sharp [email: [e-mail]]]

1.8:
mozilla/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp; 1.8.2.1;
1.8.0:
mozilla/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp; 1.8.10.1;

Comment 48

[Dave Liebreich [:davel]]

verified on mac, build 2006010904

Comment 49

[Dave Liebreich [:davel]]

verified on mac, build 2006011103 (a 1.8.0.1 build)

Comment 50

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change