dom: Avoid crashing on empty favicon href attribute#41056
dom: Avoid crashing on empty favicon href attribute#41056mrobinson merged 1 commit intoservo:mainfrom
href attribute#41056Conversation
servo-highfive
added
the
S-awaiting-review
| let Ok(href) = self.Href().parse() else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
shouldn't this return here catch an empty string? Or does the empty string somehow parse to a valid url?
And if it doesn't, please use href instead of self.Href() here.
There was a problem hiding this comment.
self.Href() does the extra work of resolving to an absolute URL, so when href is empty it returns the document url and parses successfully. So I think this part should not change, since we want the absolute url that self.Href() produces.
There was a problem hiding this comment.
Makes sense, thanks!
Can you add a regression test?
There was a problem hiding this comment.
I added a crash test in tests/wpt/mozilla/tests/mozilla/link_empty_href_crash.html . Not sure if this is the right location!
webbeef
force-pushed
the
empty-favicon-href
branch
2 times, most recently
from
ed9eada to
505bd07
Compare
| <!doctype html> | ||
| <meta charset="utf-8"> | ||
| <title>Test for #41047: Panic when the href attribute of a link is empty.</title> | ||
| <script src="/resources/testharness.js"></script> | ||
| <script src="/resources/testharnessreport.js"></script> | ||
| <script> | ||
| test(function() { | ||
| var link = document.createElement("link"); | ||
| link.setAttribute("rel", "icon"); | ||
| link.setAttribute("href", ""); | ||
| document.head.appendChild(link); | ||
| }, "Doesn't crash when setting an empty href link"); | ||
| </script> |
There was a problem hiding this comment.
It looks like this test could be upstreamed into the WPT repository.
There was a problem hiding this comment.
Ok - is that fine to move it under tests/wpt/tests/dom/link_empty_href_crash.html instead?
There was a problem hiding this comment.
<link> elements are a HTML concept, so html is the better directory for WPT. How about tests/wpt/tests/html/links/icon/empty-href-crash.html. I think the filename has to end with -crash to make the harness recognize it as a crashtest.
There was a problem hiding this comment.
There was a problem hiding this comment.
done in latest commit
webbeef
force-pushed
the
empty-favicon-href
branch
from
505bd07 to
1ec1010
Compare
|
🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#56521) with upstreamable changes. |
Add a check to handle_favicon_url() similar to the ones done in similar functions: handle_stylesheet_url() and fetch_and_process_prefetch_link() Signed-off-by: webbeef <[email protected]>
webbeef
force-pushed
the
empty-favicon-href
branch
from
1ec1010 to
74b7309
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#56521). |
mrobinson
changed the title
href attribute
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
|
✍ Updated existing upstream WPT pull request (web-platform-tests/wpt#56521) title and body. |
servo-highfive
removed
the
S-awaiting-merge
6 participants
Add a check to handle_favicon_url() similar to the ones done in similar functions: handle_stylesheet_url() and fetch_and_process_prefetch_link()
Testing: Check that loading http://chiptune.com/ doesn't crash anymore.
Fixes: #41047