Bug 216115 – new URL("#") should throw an error

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Yusuke Suzuki]

https://github.com/whatwg/url/issues/539

[Radar WebKit Bug Importer]

<rdar://problem/68252256>

[Alex Christensen]

Created attachment 408251 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 408251 [details]
Patch

r=me

[Darin Adler]

Comment on attachment 408251 [details]
Patch

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

> Source/WebCore/html/DOMURL.h:-43
> -    static ExceptionOr<Ref<DOMURL>> create(const String& url, const DOMURL& base);

Glad we are getting rid of this. I guess the specification changed since we originally wrote the code.

> Source/WebCore/html/DOMURL.h:-44
> -    static ExceptionOr<Ref<DOMURL>> create(const String& url, const URL& base);

Does this "un-optimize" any call sites that were passing a URL before, or did those call sites just not exist?

[Alex Christensen]

Comment on attachment 408251 [details]
Patch

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

>> Source/WebCore/html/DOMURL.h:-44
>> -    static ExceptionOr<Ref<DOMURL>> create(const String& url, const URL& base);
> 
> Does this "un-optimize" any call sites that were passing a URL before, or did those call sites just not exist?

Yes, this un-optimizes passing a DOM URL as the second parameter.  It used to call the constructor with a const DOMURL& and after my first patch it turns it into a String and reparses it. I'll keep that optimization, which should be undetectable from JavaScript.

[Alex Christensen]

Created attachment 408257 [details]
Patch

[Darin Adler]

Comment on attachment 408257 [details]
Patch

I don’t understand how this fix works. If the baseURL is not valid, it seems like we’d get a TypeError exception, both before and after the patch. Where’s the fix?

[Darin Adler]

Comment on attachment 408257 [details]
Patch

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

> Source/WebCore/html/DOMURL.cpp:-51
> -    if (!base.isValid())
> -        return Exception { TypeError };

Wouldn’t a better fix be to add a special case to allow null here.

> Source/WebCore/html/DOMURL.cpp:-60
> -    return create(url, base.isNull() ? aboutBlankURL() : URL { URL { }, base });

And remove the special case for null here?

[Alex Christensen]

Comment on attachment 408257 [details]
Patch

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

>> Source/WebCore/html/DOMURL.cpp:-51
>> -        return Exception { TypeError };
> 
> Wouldn’t a better fix be to add a special case to allow null here.

This is called from DOMURL::create(const String&, const DOMURL&) where we know the base is non-null and valid.  That would be an unneeded check.  It would probably be close to equivalent and the difference would probably be undetectable from JS, but I like the logic like this better.

>> Source/WebCore/html/DOMURL.cpp:-60
>> -    return create(url, base.isNull() ? aboutBlankURL() : URL { URL { }, base });
> 
> And remove the special case for null here?

We need a check for null here because it is ok to not specify the base, in which case baseURL would be null which should not throw a type error.

[Alex Christensen]

http://trac.webkit.org/r266748

[Devin Rousso]

Comment on attachment 408257 [details]
Patch

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

> LayoutTests/inspector/unit-tests/url-utilities.html:576
> +            test(new URL("#hash", "about:blank"), "about:blank");

This may cause issues in Web Inspector, specifically usage of `WI.urlWithoutFragment` and `parseURL`.  I don't know whether it's actually possible for Web Inspector to be given a Url that's just a fragment/hash, but I can't say for sure that it will never happen.

[Alex Christensen]

Ok.  One can never be sure that a change won't cause any issues.  Let's keep our eyes out, and if we see any we can reconsider this.

[Sam Sneddon [:gsnedders]]

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