Thank you BZ :)

here's a testcase, though I don't know if it has everything page info can detect at the moment

alas, bz's doesn't seem to have changed one or two things, so embed and applet don't return absolute urls.

gotta run, is this even sane?

Comment on attachment 128427 [details] [diff] [review] incomplete No, this is not sane... if you are trying to make codebase an absolute url or something, use NS_IMPL_URI_ATTR. If you're trying to make properties take codebase into account, that's a separate issue that needs discussing. And this patch would fail to compile miserably anyway on several counts.

Comment on attachment 128415 [details] [diff] [review] fix >Index: xpfe/browser/resources/content/pageInfo.js >- linkView.addRow([elem.value || gStrings.linkSubmit, getAbsoluteURL(elem.form.getAttribute("action"), elem), gStrings.linkSubmission, elem.form.getAttribute("target")]); // use getAttribute() due to bug 122128 >+ linkView.addRow([elem.value || gStrings.linkSubmit, elem.form.getAttribute("action"), gStrings.linkSubmission, elem.form.getAttribute("target")]); // use getAttribute() due to bug 122128 This is no good. Using getAttribute means that the URL will not be absolutified -- getAttribute just returns the raw attr value. You should really move to the security-safe way of getting the property getter, and then you can just use the property... >- formView.addRow([elem.name, elem.method, getAbsoluteURL(elem.getAttribute("action"), elem), elem]); // use getAttribute() because of bug 122128 >+ formView.addRow([elem.name, elem.method, elem.getAttribute("action"), elem]); // use getAttribute() because of bug 122128 Same. >- imageView.addRow([getAbsoluteURL(elem.code || elem.object, elem), gStrings.mediaApplet, "", elem, false]); >+ imageView.addRow([elem.code || elem.object, gStrings.mediaApplet, "", elem, false]); As you noted, the code property is not absolutified. > if (elem.hasAttributeNS(XLinkNS, "href")) > { > linktext = getValueText(elem); >- linkView.addRow([linktext, getAbsoluteURL(elem.href, elem), gStrings.linkX, ""]); >+ linkView.addRow([linktext, elem.href, gStrings.linkX, ""]); > } Does this code even work? Why would elem.href be defined? >- var absoluteURL = getAbsoluteURL(url, item); >- var isProtocolAllowed = regex.test(absoluteURL); >+ var isProtocolAllowed = regex.test(url); What function is this in? Please use the -p argument to diff to make things a little clearer....

> (From update of attachment 128427 [details] [diff] [review]) > No, this is not sane... if you are trying to make codebase an absolute url or > something, use NS_IMPL_URI_ATTR. If you're trying to make properties take > codebase into account, that's a separate issue that needs discussing. Properties that are urls on this object (nsHTMLAppletElement) need to be made absolute wrt codebase. I figured the best way to do this was override AttrToURI() to take codebase into account, so that NS_IMPL_URI_ATTR would not have to be changed. (I just forgot to change it from NS_IMPL_STRING_ATTR to URI_ATTR.) I just figured that would be easier than making a third macro callled NS_IMPL_URI_WITH_A_DIFFERENT_WAY_OF_FINDING_THE_BASE_ATTR, which wouldn't actually be too hard to do. Perhaps I missed something (besides a better name for a third macro)? > And this patch would fail to compile miserably anyway on several counts. Yes, I figured that. This is what I dashed off while the idea was in my head, and before I rushed off to work. My question is really just if this is the right way to go about it. >>+ linkView.addRow([elem.value || gStrings.linkSubmit, elem.form.getAttribute("action"), gStrings.linkSubmission, elem.form.getAttribute("target")]); // use getAttribute() due to bug 122128 >This is no good. Using getAttribute means that the URL will not be >absolutified -- getAttribute just returns the raw attr value. You should >really move to the security-safe way of getting the property getter, and then >you can just use the property... arrg. I guess there's nothing I can do about that though. >>- imageView.addRow([getAbsoluteURL(elem.code || elem.object, elem), gStrings.mediaApplet, "", elem, false]); >>+ imageView.addRow([elem.code || elem.object, gStrings.mediaApplet, "", elem, false]); >As you noted, the code property is not absolutified. Same with object. >> if (elem.hasAttributeNS(XLinkNS, "href")) >> { >> linktext = getValueText(elem); >>- linkView.addRow([linktext, getAbsoluteURL(elem.href, elem), gStrings.linkX, ""]); >>+ linkView.addRow([linktext, elem.href, gStrings.linkX, ""]); >> } >Does this code even work? Why would elem.href be defined? Yes, this does work. I even have/had a testcase. elem.href is defined because I checked for it (hasAttributeNS()). >>- var absoluteURL = getAbsoluteURL(url, item); >>- var isProtocolAllowed = regex.test(absoluteURL); >>+ var isProtocolAllowed = regex.test(url); >What function is this in? Please use the -p argument to diff to make things a >little clearer.... makePreview(), it tests to make sure the image isn't in chrome://, etc.

> Properties that are urls on this object (nsHTMLAppletElement) need to be made > absolute wrt codebase. Sure. > I figured the best way to do this was override AttrToURI() Yeah, that may be a decent way to do it... Then again, if we need this for both object and applet, I would prefer another macro. Perhaps: NS_IMPL_URI_ATTR_WITH_BASE(_class, _propName, _attrName, _baseName) ? AttrToURI could then be modified slightly to take a base atom arg, which would be allowed to be null.... Keep in mind that the codebase URI may be relative itself, btw. ;) > I even have/had a testcase. I'd love to see that testcase, if you have it.

should be complete, unless I'm forgetting an element that needs the same treatment. also untested, but that'll have to wait till I get home.

I knew I forgot something. The object attribute on applets is relative to codebase.

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

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

I seem to have let this slip. I've got a new patch I'm testing. (bug 232016 rotted the old one, naturally)

working testcase, adapted from the one in bug 284779 as for the patch, well, I seem to have introduced a crash, so I'm waiting for a debug build

Will this fix apply to Firefox too? If not, bug 284714 is not a dupe.

Yes, this fixes both Mozilla and Firefox, because it alters the way the DOM stores the attribute (as opposed to altering the way Page Info processes the attribute)

updated the test url now that my server is back online

Comment on attachment 177310 [details] [diff] [review] 213701-5.diff This really needs ok from jst, since it's changing what the DOM does...

So with this change, how do we compare to IE re exposing these URIs, do we now match what it does more than before this change?

I don't know, I can't run IE.

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

For the embed element, the patch works fine.

Comment on attachment 177310 [details] [diff] [review] 213701-5.diff From some simple testing with the testcase in this bug, IE exposes the src and data attributes as they appear in the HTML, not as absolute URLs. I'd rather change our implementation to match that (it does for embed tags, but not for object tags) than to go the other way. r-

Comment on attachment 177310 [details] [diff] [review] 213701-5.diff On second thought after Neil reminded me about all the other differences we've had between IE and Mozilla with related attributes, we might as well do what makes sense here. r=jst

Comment on attachment 177310 [details] [diff] [review] 213701-5.diff Three requests: 1) Use more context (-8, say). 2) Use the -p option to diff. 3) Don't copy/paste the existing GetURIAttr function. Add an arg to it that's allowed to be null, like I suggested earlier in the bug...

(In reply to comment #26) > (From update of attachment 177310 [details] [diff] [review] [edit]) > 3) Don't copy/paste the existing GetURIAttr function. Add an arg to it that's > allowed to be null, like I suggested earlier in the bug... I did it this way because when I combined them, the resulting function was either unreadable or twice as long (IE most of the body in a giant if/else).

er... the only part that should be if/else should be the getting of the baseURI (and even then, it should be pretty straightforward, I would think).

looking back at the patch, however, makes me wonder what would happen if someone called the second GetURIAttr() with a null aBaseAttr pointer… would it work correctly, skipping the bits where baseAttrValue is used? /me goes to reread GetAttr()

better patch. carrying forward r+

Comment on attachment 180550 [details] [diff] [review] 213701-6.diff >Index: content/html/content/src/nsGenericHTMLElement.cpp >+nsGenericHTMLElement::GetURIAttr(nsIAtom* aAttr, nsIAtom* aBaseAttr, nsAString& aResult) >+ nsAutoString attrValue, baseAttrValue; Why are you declaring baseAttrValue up here if you only use it in the |if (aBaseAttr)| block? > nsresult rv = GetAttr(kNameSpaceID_None, aAttr, attrValue); >- if (rv != NS_CONTENT_ATTR_HAS_VALUE) { >+ if (NS_FAILED(rv)) { This change looks wrong. It'll make an <img> (note no URI attr set) claim to have an href equal to the document URI, which is very much undesirable. I'm not sure why you decided to change this, but please don't do that. >+ nsCOMPtr<nsIURI> attrURI, baseAttrURI; Again, why do you need baseAttrURI out here? >+ if (aBaseAttr) { >+ rv = GetAttr(kNameSpaceID_None, aBaseAttr, baseAttrValue); >+ >+ if (NS_SUCCEEDED(rv)) { GetAttr basically _always_ succeeds. I think what you really want to check for here is the NS_CONTENT_ATTR_HAS_VALUE rv, as above. >+ rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(baseAttrURI), >+ baseAttrValue, GetOwnerDoc(), >+ baseURI); So I'd put the declaration of baseAttrURI right before this call. And right after this call do: baseURI.swap(baseAttrURI); Then instead of needing three more NewURIWithDocument calls you just need one. Make sure not to do that one in the case when NewURIWithDocumentCharset failed, though.

Daniel Brooks: Don't you work on this, now? I regret that this bug still occur in Firefox1.5.

This is core bug because seamoneky and firefox have this problem, isn't it?

Unbitrotted patch taking account of comment 31. This is an update of the patch from Daniel Brooks (attachment 180550 [details] [diff] [review])

Comment on attachment 228726 [details] [diff] [review] 213701-7.diff Defering to bz for r+sr on this one as he's more familiar with the details around how this should work.

The archive attribute isn't a URI. http://www.w3.org/TR/html4/struct/objects.html#adef-archive-OBJECT

I'll try to get to this in the next week or so, but aren't you declaring two different GetURIAttr methods and only implementing one?

(In reply to comment #37) > aren't you declaring two different GetURIAttr methods > and only implementing one? > yes, I was.

Comment on attachment 229568 [details] [diff] [review] 213701-9.diff (trunk) >Index: nsGenericHTMLElement.cpp >+ rv = nsContentUtils::NewURIWithDocumentCharset(getter_AddRefs(baseAttrURI), >+ baseAttrValue, GetOwnerDoc(), If this fails we should fall back on using attrValue, not fall back on using the baseURI of the node, imo. >Index: nsHTMLImageElement.cpp >-NS_IMPL_STRING_ATTR(nsHTMLImageElement, Lowsrc, lowsrc) >+NS_IMPL_URI_ATTR(nsHTMLImageElement, Lowsrc, lowsrc) Please undo this change? There's no spec that says we should do it, and I'd rather not implement inconsistencies from IE given that. The rest looks good.

Comment on attachment 230002 [details] [diff] [review] 213701-10.diff (trunk) r+sr=bzbarsky. I'll try to get this checked in tonight.

Checked that patch in on trunk.

(In reply to comment #42) > Checked that patch in on trunk. > Does this mean that this is fixed now? Or is there anything else that should go in?

(In reply to comment #43) > > Does this mean that this is fixed now? Or is there anything else that should go > in? > Yes, this is fixed in trunk only. I didn't mark this bug as FIXED because I hoped I could fix it for the 1.8branch too, but I guess now it's too late.

(In reply to comment #44) >Yes, this is fixed in trunk only. I didn't mark this bug as FIXED because I >hoped I could fix it for the 1.8branch too, but I guess now it's too late. Actually we haven't even released SeaMonkey 1.1 beta yet...