Bug 189554 - URL::string should return a null String if parsing failed
Summary: URL::string should return a null String if parsing failed
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-12 11:21 PDT by Alex Christensen
Modified: 2018-09-13 09:47 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.83 KB, patch)
2018-09-12 11:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (4.96 KB, patch)
2018-09-12 11:24 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (4.96 KB, patch)
2018-09-12 11:25 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (655.56 KB, application/zip)
2018-09-12 12:35 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (629.45 KB, application/zip)
2018-09-12 12:37 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (11.29 MB, application/zip)
2018-09-12 13:09 PDT, EWS Watchlist
no flags Details
Patch (6.31 KB, patch)
2018-09-12 13:47 PDT, Alex Christensen
achristensen: review-
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (1.52 MB, application/zip)
2018-09-12 15:01 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews102 for mac-sierra (484.68 KB, application/zip)
2018-09-12 15:04 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2018-09-12 11:21:56 PDT
URL::string should return a null String if parsing failed
Comment 1 Alex Christensen 2018-09-12 11:23:27 PDT
Created attachment 349563 [details]
Patch
Comment 2 Alex Christensen 2018-09-12 11:24:36 PDT
Created attachment 349564 [details]
Patch
Comment 3 Alex Christensen 2018-09-12 11:25:23 PDT
Created attachment 349565 [details]
Patch
Comment 4 Alexey Proskuryakov 2018-09-12 12:28:06 PDT
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 5 EWS Watchlist 2018-09-12 12:35:11 PDT
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.
Comment 6 EWS Watchlist 2018-09-12 12:35:12 PDT
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 7 EWS Watchlist 2018-09-12 12:37:55 PDT
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.
Comment 8 EWS Watchlist 2018-09-12 12:37:57 PDT
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 9 EWS Watchlist 2018-09-12 13:09:13 PDT
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.
Comment 10 EWS Watchlist 2018-09-12 13:09:15 PDT
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
Comment 11 Alex Christensen 2018-09-12 13:47:27 PDT
Created attachment 349578 [details]
Patch
Comment 12 Daniel Bates 2018-09-12 14:57:30 PDT
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 13 EWS Watchlist 2018-09-12 15:01:42 PDT
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.
Comment 14 EWS Watchlist 2018-09-12 15:01:44 PDT
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 15 EWS Watchlist 2018-09-12 15:04:46 PDT
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.
Comment 16 EWS Watchlist 2018-09-12 15:04:48 PDT
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
Comment 17 Chris Dumez 2018-09-12 15:06:14 PDT
(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
Comment 18 Alex Christensen 2018-09-12 15:34:31 PDT
(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 19 Alex Christensen 2018-09-12 15:35:12 PDT
Comment on attachment 349578 [details]
Patch

There are a few more places we need stringMaybeInvalid, and they're tied up in URLUtils.
Comment 20 Alex Christensen 2018-09-13 09:25:09 PDT
(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.
Comment 21 Alexey Proskuryakov 2018-09-13 09:47:12 PDT
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.