Bug 98184 – HTMLBaseElement href attribute binding returns wrong URL

• Component: DOM
• Status: RESOLVED FIXED

[Erik Arvidsson]

We currently use [Reflect, URL] which resolves the reflected attribute relative to the base element of the document.

In the case of the base element we need to resolve the attribute relative to the URL of the document and not the base element itself.

[Erik Arvidsson]

Created attachment 166727 [details]
Patch

[Adam Barth]

Comment on attachment 166727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166727&action=review

> Source/WebCore/html/HTMLBaseElement.cpp:81
> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(getAttribute(hrefAttr)), document()->url());

fastGetAttribute

Does this correctly if the attribute is missing?

[Erik Arvidsson]

(In reply to comment #2)
> (From update of attachment 166727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166727&action=review
> 
> > Source/WebCore/html/HTMLBaseElement.cpp:81
> > +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(getAttribute(hrefAttr)), document()->url());
> 
> fastGetAttribute

fastGetAttribute should work but [Reflect, URL] uses getAttribute so I decided to use that... I'll change it.

> Does this correctly if the attribute is missing?

I tried this manually because I wasn't sure the existing tests would cover this.

[Erik Arvidsson]

Created attachment 166734 [details]
Change to use fastGetAttribute

[Adam Barth]

> fastGetAttribute should work but [Reflect, URL] uses getAttribute so I decided to use that... I'll change it.

That's because Reflect isn't smart enough to know whether this is one of the slow attributes.  :)

[Adam Barth]

Is there a spec we can reference here?  Have you tested the behavior in other browsers?

[Adam Barth]

Does this fix a particular web site?  I don't doubt that we should make this change, I'd just like to know if we're improving compat and/or interop.

[Erik Arvidsson]

http://code.google.com/p/chromium/issues/detail?id=140200

[Erik Arvidsson]

With this patch we match Firefox.

[Alexey Proskuryakov]

Look like a duplicate of bug 19998, and possibly bug 47348.

[Darin Adler]

Comment on attachment 166734 [details]
Change to use fastGetAttribute

View in context: https://bugs.webkit.org/attachment.cgi?id=166734&action=review

> Source/WebCore/html/HTMLBaseElement.cpp:82
> +String HTMLBaseElement::href() const
> +{
> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url());
> +}

How does this compile? completeURL returns a KURL but this function returns a String.

This needs a comment explaining how it differs from the getURLAttribute function. It’s not enough to have it in the change log.

> Source/WebCore/html/HTMLBaseElement.cpp:84
> +void HTMLBaseElement::setHref(const String& v)

Please use words rather than single letters for argument names.

> Source/WebCore/html/HTMLBaseElement.h:35
> +    void setHref(const String&);

Type here should be const AtomicString& rather than const String&, because the former is the type that setAttribute takes. The code generated by [Reflect] correctly uses AtomicString.

[Erik Arvidsson]

(In reply to comment #10)
> Look like a duplicate of bug 19998, and possibly bug 47348.

I did look at 19998 before filing this bug. It is not a duplicate bug.

It does look like a dupe of 47348, even though the comments in there have contradictions.

[Erik Arvidsson]

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

[Erik Arvidsson]

Created attachment 166903 [details]
Patch

[Darin Adler]

Comment on attachment 166903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166903&action=review

> Source/WebCore/ChangeLog:17
> +        (WebCore):

Please leave meaningless lines like this out of the change log even though the buggy script puts them in.

> Source/WebCore/ChangeLog:18
> +        (WebCore::HTMLBaseElement::setHref):

Worth commenting on this function here.

> Source/WebCore/ChangeLog:20
> +        (HTMLBaseElement):

Please leave meaningless lines like this out of the change log even though the buggy script puts them in.

> Source/WebCore/html/HTMLBaseElement.cpp:83
> +    // This does not use getURLAttribute because we need to resolve this relative to the document
> +    // and not relative to the base element itself.
> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url());

I think the wording here is not sufficiently clear. After all, getURLAttribute does resolve relative to the document. It’s just that it resolves relative to the document’s base URL, which can be influenced by base elements, and might or might not be affected by this particular base element.

You are aware of those connections as you are writing this, but the person reading the code may not be, so I suggest more precise wording. Here’s my cut:

    // This does not use the getURLAttribute function because that will resolve relative to the document’s base URL;
    // base elements like this one can be used to set that base URL. Thus we need to resolve relative to the document’s
    // URL and ignore the base URL.

After writing that comment, I realize that this code may be incorrect.

Passing the document’s URL for baseURLOverride prevents the Document::completeURL function from considering the parent document’s base URL. Is that OK? In what cases do the parent document base URLs come into play? The test case won’t cover this, because it’s only a single frame, so there’s a chance we have broken something here that is not covered by any test case.

> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase-expected.txt:11
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +Base case, rebase URL without attribute having been set
> +PASS a.href is "http://old_base/foo?query"
> +PASS a.href is "http://new_base/foo?query"

This looks silly. You should look at other tests that run at load event time, to see if there’s a technique that can prevent writing out the results in this backwards order.

> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:19
> +    base.href = 'http://old_base/';
> +    a.href = 'foo?query';
> +    shouldBeEqualToString('a.href', 'http://old_base/foo?query');
> +    base.href = 'http://new_base/';
> +    shouldBeEqualToString('a.href', 'http://new_base/foo?query');

Test cases like this can be written in a way that makes the test output much clearer.

    shouldBeEqualToString('base.href = "http://old_base/"; a.href = "foo?query"; a.href', 'http://old_base/foo?query');

If the test is written in this fashion then the test output makes more clear what’s being tested.

> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:23
> +<script src="../../js/resources/js-test-post.js"></script>

I don’t think this is needed.

> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-binding-expected.txt:1
> +PASS

It’s a little weak to have a test write out only PASS rather than indicating what it’s testing. I know it’s hard to write out URLs without being dependent on file system location, so that’s an excuse, but it’s even better if you can show what’s being tested and what the result is, somehow.

> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-binding.html:5
> +if (self.testRunner)

It’s curious to use self.testRunner here when all other tests idiomatically use window.testRunner. Any reason to be non-traditional here?

[Erik Arvidsson]

Created attachment 166939 [details]
Patch

[Erik Arvidsson]

Comment on attachment 166903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166903&action=review

>> Source/WebCore/html/HTMLBaseElement.cpp:83
>> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url());
> 
> I think the wording here is not sufficiently clear. After all, getURLAttribute does resolve relative to the document. It’s just that it resolves relative to the document’s base URL, which can be influenced by base elements, and might or might not be affected by this particular base element.
> 
> You are aware of those connections as you are writing this, but the person reading the code may not be, so I suggest more precise wording. Here’s my cut:
> 
>     // This does not use the getURLAttribute function because that will resolve relative to the document’s base URL;
>     // base elements like this one can be used to set that base URL. Thus we need to resolve relative to the document’s
>     // URL and ignore the base URL.
> 
> After writing that comment, I realize that this code may be incorrect.
> 
> Passing the document’s URL for baseURLOverride prevents the Document::completeURL function from considering the parent document’s base URL. Is that OK? In what cases do the parent document base URLs come into play? The test case won’t cover this, because it’s only a single frame, so there’s a chance we have broken something here that is not covered by any test case.

Yeah, this definitely does not do what I expected.

>> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:23
>> +<script src="../../js/resources/js-test-post.js"></script>
> 
> I don’t think this is needed.

I cleaned up the tests a bit.

[Ojan Vafai]

(In reply to comment #17)
> (From update of attachment 166903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166903&action=review
> 
> >> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:23
> >> +<script src="../../js/resources/js-test-post.js"></script>
> > 
> > I don’t think this is needed.

It's the bit that checks that the test was successfully parsed and then prints out TEST COMPLETED. We never got around to moving this into js-test-pre.js and Alexey had an objection that doing so made js-test-pre.js too complicated. I still think we should do it since people are just leaving it out now and we get worse tests.

[Erik Arvidsson]

Darin, do you want to take a second look?

[Ryosuke Niwa]

Comment on attachment 166939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166939&action=review

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

You need to add a description here.

> Source/WebCore/html/HTMLBaseElement.cpp:82
> +    // This does not use the getURLAttribute function because that will resolve relative to the documentâs base URL;

Nit: documentâs

> Source/WebCore/html/HTMLBaseElement.cpp:85
> +    // return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url());

Why do we need to include this commented out code?

> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-binding.html:16
> +shouldBeTrue("endsWith(base.href, 'foo/bar/')");
> +shouldBeFalse("endsWith(base.href, 'foo/bar/foo/bar/')");

I don't understand why this test is called href-attribute-binding since we didn't make any changes to the binding code.
How about something like href-attribute-resolves-with-respect-to-document?

[Ryosuke Niwa]

(In reply to comment #17)
> (From update of attachment 166903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166903&action=review
> 
> >> Source/WebCore/html/HTMLBaseElement.cpp:83
> >> +    return document()->completeURL(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)), document()->url());
> > 
> > I think the wording here is not sufficiently clear. After all, getURLAttribute does resolve relative to the document. It’s just that it resolves relative to the document’s base URL, which can be influenced by base elements, and might or might not be affected by this particular base element.
> > 
> > You are aware of those connections as you are writing this, but the person reading the code may not be, so I suggest more precise wording. Here’s my cut:
> > 
> >     // This does not use the getURLAttribute function because that will resolve relative to the document’s base URL;
> >     // base elements like this one can be used to set that base URL. Thus we need to resolve relative to the document’s
> >     // URL and ignore the base URL.
> > 
> > After writing that comment, I realize that this code may be incorrect.

Can we add a test to ensure we don't make the same mistake in the future?

[Erik Arvidsson]

Comment on attachment 166939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166939&action=review

>> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-binding.html:16
>> +shouldBeFalse("endsWith(base.href, 'foo/bar/foo/bar/')");
> 
> I don't understand why this test is called href-attribute-binding since we didn't make any changes to the binding code.
> How about something like href-attribute-resolves-with-respect-to-document?

The buggy behavior was due to us using the [Reflect, URL] in the bindings when using the default behavior does not work for this.

Anyway, I'm not attached to the name. I'll rename the file.

[Erik Arvidsson]

Created attachment 168429 [details]
Patch

[Build Bot]

Comment on attachment 168429 [details]
Patch

Attachment 168429 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14265358

New failing tests:
fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html
fast/dom/element-attribute-js-null.html

[Erik Arvidsson]

Created attachment 168443 [details]
Patch

[Build Bot]

Comment on attachment 168443 [details]
Patch

Attachment 168443 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14300001

New failing tests:
fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html
fast/js/random-array-gc-stress.html

[Erik Arvidsson]

(In reply to comment #26)
> New failing tests:
> fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html

I'm looking into this now.

It turns out that the Apple port of DRT crashes with testRunner.setCanOpenWindows(). I filed https://bugs.webkit.org/show_bug.cgi?id=99465

Also, KURL and GURL behaves different. No surprises there :'( I'll have to handle this slightly differently

[Erik Arvidsson]

Created attachment 168956 [details]
Patch

[Erik Arvidsson]

PTAL

I removed the window.open test (it was not really needed to cover the issue in the first patch).

I had to manually handle invalid and about:blank URL since KURL and GURL resolves URLs differently in those cases.

[Erik Arvidsson]

Created attachment 169183 [details]
Patch

[Ojan Vafai]

Comment on attachment 169183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169183&action=review

> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:16
> +base.href = 'http://old_base/';
> +a.href = 'foo?query';
> +shouldBeEqualToString('a.href', 'http://old_base/foo?query');
> +base.href = 'http://new_base/';
> +shouldBeEqualToString('a.href', 'http://new_base/foo?query');

Did you not want to do Darin's comment about making this test output cleaner?

> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:8
> +    // HACK: setCanOpenWindows needs the test to be async or the Apple port crashes.

s/HACK/FIXME

> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:49
> +// Make sure that we don't use the creator document as the base.
> +debug('createHTMLDocument');
> +var doc = document.implementation.createHTMLDocument('');
> +base = doc.head.appendChild(doc.createElement('base'));
> +base.setAttribute('href', 'foo/bar/');
> +shouldBeEqualToString('base.href', '');
> +
> +// Make sure that we use the parent document as the base.
> +debug('iframe');
> +var iframe = document.body.appendChild(document.createElement('iframe'));
> +base = iframe.contentDocument.head.appendChild(doc.createElement('base'));
> +base.setAttribute('href', 'foo/bar/');
> +shouldBeEqualToString('base.href', '');
> +
> +// Make sure that we don't use the opener document as the base.
> +debug('window.open');
> +var win = window.open('about:blank');
> +base = win.document.head.appendChild(win.document.createElement('base'));
> +base.setAttribute('href', 'foo/bar/');
> +shouldBeEqualToString('base.href', '');

Sorry, I don't understand these test cases. Also, the comments don't seem to match the test cases. Maybe I don't just don't understand the patch?

> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:50
> +win.close();

Is this part of the async hack?

[Erik Arvidsson]

Comment on attachment 169183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169183&action=review

I'll upload a new patch

>> LayoutTests/fast/dom/HTMLAnchorElement/set-href-attribute-rebase.html:16
>> +shouldBeEqualToString('a.href', 'http://new_base/foo?query');
> 
> Did you not want to do Darin's comment about making this test output cleaner?

I'm not sure I think 

document.querySelector('a').href

is cleaner than 

a.href

But I'll make the change anyway.

>> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:49
>> +shouldBeEqualToString('base.href', '');
> 
> Sorry, I don't understand these test cases. Also, the comments don't seem to match the test cases. Maybe I don't just don't understand the patch?

The first patch was using the wrong method to resolve the URL. Ryosuke wanted me to test the cases that would be broken if the wrong method was used. That is why iframe, createHTMLDocument and window.open are tested.

When the document does not have a URL (because it does not have a creation context) the base URL cannot be resolved and we should use "" as the value of the href. This is no change in behavior, just more tests to ensure this did not break it (like it did in the first patch).

>> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:50
>> +win.close();
> 
> Is this part of the async hack?

No. Just closing it because it is annoying to keep opening new windows when testing in other browsers.

[Erik Arvidsson]

Created attachment 169416 [details]
Patch

[Ojan Vafai]

Comment on attachment 169416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169416&action=review

> LayoutTests/fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html:29
> +// Make sure that we don't use the creator document as the base.

Can you add a comment that these three cases should not resolved because nested document lack a url?

[Erik Arvidsson]

Comment on attachment 169416 [details]
Patch

This does not do the right thing when base.href is set to a URL that has a scheme.

[Erik Arvidsson]

Created attachment 169456 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169456 [details]
Patch

Clearing flags on attachment: 169456

Committed r132071: <http://trac.webkit.org/changeset/132071>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Steve Block]

> It does look like a dupe of 47348
Are you sure this is a dupe? Bug 47348 is about changing the behavior of HTMLAnchorElement when the base URI is updated. This bug seems to be about the behavior of HTMLBaseElement.