213701 - page info no longer needs to double check that urls are absolute

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Daniel Brooks [:db48x]]

Thank you BZ :)

Comment 1

[Daniel Brooks [:db48x]]

Attachment: fix

Comment 2

[Daniel Brooks [:db48x]]

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

Comment 3

[Daniel Brooks [:db48x]]

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

Comment 4

[Daniel Brooks [:db48x]]

Attachment: incomplete

gotta run, is this even sane?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 6

[Boris Zbarsky [:bzbarsky]]

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....

Comment 7

[Daniel Brooks [:db48x]]

> (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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 9

[Daniel Brooks [:db48x]]

Attachment: fix

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.

Comment 10

[Daniel Brooks [:db48x]]

Attachment: corrected

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

Comment 11

[Daniel Brooks [:db48x]]

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

Comment 12

[Daniel Brooks [:db48x]]

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

Comment 13

[Daniel Brooks [:db48x]]

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

Comment 14

[Daniel Brooks [:db48x]]

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

Comment 15

[Justin Kerk]

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

Comment 16

[Daniel Brooks [:db48x]]

Attachment: 213701-5.diff

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)

Comment 17

[Daniel Brooks [:db48x]]

updated the test url now that my server is back online

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 177310 [details] [diff] [review]
213701-5.diff

This really needs ok from jst, since it's changing what the DOM does...

Comment 19

[Johnny Stenback (:jst)]

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?

Comment 20

[Daniel Brooks [:db48x]]

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

Comment 21

[Daniel Brooks [:db48x]]

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

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase

For the embed element, the patch works fine.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot

Comment 24

[Johnny Stenback (:jst)]

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 25

[Johnny Stenback (:jst)]

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 26

[Boris Zbarsky [:bzbarsky]]

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...

Comment 27

[Daniel Brooks [:db48x]]

(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).

Comment 28

[Boris Zbarsky [:bzbarsky]]

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).

Comment 29

[Daniel Brooks [:db48x]]

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()

Comment 30

[Daniel Brooks [:db48x]]

Attachment: 213701-6.diff

better patch. carrying forward r+

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Daniel Brooks:

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

Comment 33

[Hideo Oshima]

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

Comment 34

[Florian Quèze [:florian]]

Attachment: 213701-7.diff

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

Comment 35

[Johnny Stenback (:jst)]

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.

Comment 36

[Florian Quèze [:florian]]

Attachment: 213701-8.diff (trunk)

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

Comment 37

[Boris Zbarsky [:bzbarsky]]

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?

Comment 38

[Florian Quèze [:florian]]

Attachment: 213701-9.diff (trunk)

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

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 40

[Florian Quèze [:florian]]

Attachment: 213701-10.diff (trunk)

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 230002 [details] [diff] [review]
213701-10.diff (trunk)

r+sr=bzbarsky.  I'll try to get this checked in tonight.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Checked that patch in on trunk.

Comment 43

[Stefan [:stefanh]]

(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?

Comment 44

[Florian Quèze [:florian]]

(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.

Comment 45

[[e-mail]]

(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...