Bug 189554 – URL::string should return a null String if parsing failed

• Component: New Bugs
• Status: NEW

[Alex Christensen]

URL::string should return a null String if parsing failed

[Alex Christensen]

Created attachment 349563 [details]
Patch

[Alex Christensen]

Created attachment 349564 [details]
Patch

[Alex Christensen]

Created attachment 349565 [details]
Patch

[Alexey Proskuryakov]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 349578 [details]
Patch

[Daniel Bates]

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?

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Chris Dumez]

(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

[Alex Christensen]

(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.

[Alex Christensen]

Comment on attachment 349578 [details]
Patch

There are a few more places we need stringMaybeInvalid, and they're tied up in URLUtils.

[Alex Christensen]

(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.

[Alexey Proskuryakov]

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.