Summary: | URL::string should return a null String if parsing failed | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||||
Status: | NEW --- | ||||||||||||||||||||||
Severity: | Normal | CC: | ap, benjamin, cdumez, cmarcelo, dbates, esprehn+autocc, ews-watchlist, gyuyoung.kim, rniwa | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=132224 | ||||||||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2018-09-12 11:21:56 PDT
Created attachment 349563 [details]
Patch
Created attachment 349564 [details]
Patch
Created attachment 349565 [details]
Patch
Comment on attachment 349565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349565&action=review > Source/WTF/ChangeLog:3 > + URL::string should return a null String if parsing failed The URL class is routinely used to represent invalid URLs, so this would require a large refactoring (if even desirable at all). It is of course true that clients that assume the string to always be valid are a problem. Comment on attachment 349565 [details] Patch Attachment 349565 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9194040 Number of test failures exceeded the failure limit. Created attachment 349567 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 349565 [details] Patch Attachment 349565 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9193956 Number of test failures exceeded the failure limit. Created attachment 349568 [details]
Archive of layout-test-results from ews115 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 349565 [details] Patch Attachment 349565 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9194042 Number of test failures exceeded the failure limit. Created attachment 349572 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 349578 [details]
Patch
Comment on attachment 349578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349578&action=review > Source/WTF/wtf/text/WTFString.cpp:1220 > +const String& nullString() > +{ > + static NeverDestroyed<String> nullString; > + return nullString; > +} > + Why? Comment on attachment 349578 [details] Patch Attachment 349578 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9195254 Number of test failures exceeded the failure limit. Created attachment 349584 [details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 349578 [details] Patch Attachment 349578 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9195504 Number of test failures exceeded the failure limit. Created attachment 349585 [details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
(In reply to Daniel Bates from comment #12) > Comment on attachment 349578 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349578&action=review > > > Source/WTF/wtf/text/WTFString.cpp:1220 > > +const String& nullString() > > +{ > > + static NeverDestroyed<String> nullString; > > + return nullString; > > +} > > + > > Why? +1 (In reply to Daniel Bates from comment #12) > Comment on attachment 349578 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349578&action=review > > > Source/WTF/wtf/text/WTFString.cpp:1220 > > +const String& nullString() > > +{ > > + static NeverDestroyed<String> nullString; > > + return nullString; > > +} > > + > > Why? So we can return a String& to a null string. Same reason emptyString() exists. Comment on attachment 349578 [details]
Patch
There are a few more places we need stringMaybeInvalid, and they're tied up in URLUtils.
(In reply to Alexey Proskuryakov from comment #4) > Comment on attachment 349565 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349565&action=review > > > Source/WTF/ChangeLog:3 > > + URL::string should return a null String if parsing failed > > The URL class is routinely used to represent invalid URLs, so this would > require a large refactoring (if even desirable at all). I believe the anchor element is the only valid use of a URL representing an invalid URL because of its href definition being the canonicalized URL if it parses and whatever string it had if it doesn't parse. If there are more, please let me know. > It is of course true that clients that assume the string to always be valid > are a problem. The way the class is currently designed, this is done frequently, unintentionally, and unknowingly. Various URLs using custom schemes sometimes happen to be invalid per WebCore. We now have quite a few custom scheme workflows. Bug 136010 was one tangentially related example, but I think that we had something where it was even more clear that strings supplied by clients needed to be preserved verbatim. |