Relative URLs in Live Bookmark link elements not resolved

VERIFIED FIXED in Firefox 2

Status

()

defect
VERIFIED FIXED
15 years ago
13 years ago

People

(Reporter: philor, Assigned: sayrer)

Tracking

({fixed1.8.1, testcase, verified1.8.0.1})

Trunk
Firefox 2
Points:
---
Bug Flags:
blocking1.8.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

15 years ago
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
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20040930
Firefox/0.10

Confirming

->NEW
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

15 years ago
*** Bug 261206 has been marked as a duplicate of this bug. ***
Assignee: vladimir → vladimir+bm

Comment 3

14 years ago
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.

Updated

14 years ago
Attachment #190753 - Flags: review?(vladimir)
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.
Attachment #190753 - Flags: review?(vladimir) → review+

Comment 5

14 years ago
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

14 years ago
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.
Attachment #190753 - Flags: approval1.8b4?

Updated

14 years ago
Attachment #190753 - Flags: approval1.8b4? → approval1.8b4+
Assignee: vladimir+bm → rubys
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);
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.
I've backed out the patch.

Comment 10

14 years ago
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.
Attachment #190753 - Flags: superreview-
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

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

shot down in flames
Attachment #190753 - Attachment is obsolete: true

Comment 13

14 years ago
Posted patch streamlined patch (obsolete) — Splinter Review
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

14 years ago
Comment on attachment 191052 [details] [diff] [review]
streamlined patch

nudge this forward
Attachment #191052 - Flags: review?(darin)
Attachment #191052 - Flags: approval1.8b4?

Comment 15

14 years ago
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?
Attachment #191052 - Flags: review?(darin) → review-
Version: unspecified → Trunk

Comment 16

14 years ago
Attachment #191052 - Attachment is obsolete: true
Attachment #191769 - Flags: review?(darin)

Comment 17

14 years ago
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.
Attachment #191769 - Flags: review?(darin) → review-

Comment 18

14 years ago
(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?
hmm, if such a dependency is OK, would nsILink work? It has a convenient
GetHrefURI method.

Comment 20

14 years ago
(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.
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?)
(Reporter)

Comment 22

14 years ago
(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.

(Assignee)

Updated

14 years ago
Assignee: rubys → sayrer
(Assignee)

Comment 23

14 years ago
Attachment #191769 - Attachment is obsolete: true
Attachment #204853 - Flags: review?(darin)

Comment 24

14 years ago
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

14 years ago
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!
Attachment #204853 - Flags: review?(darin) → review+
(Assignee)

Updated

14 years ago
Attachment #204853 - Flags: review+ → review?(vladimir)
(Reporter)

Comment 26

14 years ago
*** Bug 320128 has been marked as a duplicate of this bug. ***
(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

14 years ago
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.
(Reporter)

Comment 29

14 years ago
(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.
(Assignee)

Updated

14 years ago
Attachment #204853 - Flags: review?(vladimir)
(Assignee)

Comment 30

14 years ago
Posted patch Use nsIDOM3Node (obsolete) — Splinter Review
OK, now we have both choices.
Attachment #205775 - Flags: review?(vladimir)

Comment 31

14 years ago
> 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 ;-)
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

14 years ago
> 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

14 years ago
> 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 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 :).
Attachment #205775 - Flags: review?(vladimir) → review+
(Assignee)

Comment 36

14 years ago
Attachment #204853 - Attachment is obsolete: true
Attachment #205775 - Attachment is obsolete: true

Comment 37

14 years ago
> >+                                                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

14 years ago
Comment on attachment 206186 [details] [diff] [review]
fix formatting

sr=darin w/ nsString for absLinkStr.
Attachment #206186 - Flags: superreview+

Comment 40

14 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Flags: blocking-aviary2?
(Assignee)

Updated

14 years ago
Flags: testcase+
(Assignee)

Updated

14 years ago
Flags: testcase+
Keywords: testcase
(Assignee)

Comment 42

14 years ago
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

14 years ago
nominating per some comments by Robert.
Flags: blocking1.8.1?
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
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1?
Flags: blocking-aviary2?
(Assignee)

Comment 45

14 years ago
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).
Flags: blocking1.8.0.1?
Comment on attachment 206190 [details] [diff] [review]
patch that was checked in

a=dveditz for drivers
Attachment #206190 - Flags: approval1.8.1+
Attachment #206190 - Flags: approval1.8.0.1+
Flags: blocking1.8.1+
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1+
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;
Target Milestone: --- → Firefox 2
verified on mac, build 2006010904
verified on mac, build 2006011103 (a 1.8.0.1 build)
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.