Bug 235061

Summary: Web Inspector: `console.log` format strings containing invalid specifiers results in `[Object object]` replacing the specifier instead of ignoring the invalid specifier
Product: WebKit Reporter: zehexin6
Component: Web InspectorAssignee: Patrick Angle <pangle>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, hi, inspector-bugzilla-changes, pangle, webkit-bug-importer, zehexin6
Priority: P2 Keywords: InRadar
Version: Safari 15   
Hardware: Mac (Intel)   
OS: macOS 12   
Attachments:
Description Flags
Patch v1.0
none
Patch v1.1 ews-feeder: commit-queue-

Description zehexin6 2022-01-11 03:19:58 PST
# console.log print strings contains '%_' result is Object

## Reproduce code

console.log('__url__%__', 'url');

## Actual output

__url__[object Object]_

## Expected output

__url__%__ url

## Reference link

- https://console.spec.whatwg.org/#formatting-specifiers
- https://console.spec.whatwg.org/#formatter
Comment 1 Radar WebKit Bug Importer 2022-01-18 03:20:16 PST
<rdar://problem/87704591>
Comment 2 Patrick Angle 2022-03-21 16:00:27 PDT
Created attachment 455289 [details]
Patch v1.0
Comment 3 Devin Rousso 2022-03-21 16:06:45 PDT
Comment on attachment 455289 [details]
Patch v1.0

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

r=me, interesting 🤔

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1105
> +    value(format, substitutions, formatters, initialValue, append, {ignoreUnknownSpecifiers} = {})

I wonder is there a situation where we want to include an unknown specifier?  My gut tells me that this should be the default (and non-configurable) behavior.

> LayoutTests/inspector/unit-tests/string-utilities-expected.txt:23
> +WARN: String.format("%_ %s", "first", "second"): unsupported format character â_â. Treating as a string.

ewww we have smart quotes in error messages?  We should probably remove that (and switch to `WI.reportInternalError`).  Can be a followup if you'd prefer :)
Comment 4 Patrick Angle 2022-03-22 09:47:10 PDT
Comment on attachment 455289 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1105
>> +    value(format, substitutions, formatters, initialValue, append, {ignoreUnknownSpecifiers} = {})
> 
> I wonder is there a situation where we want to include an unknown specifier?  My gut tells me that this should be the default (and non-configurable) behavior.

A quick survey of call sites seems to indicate that other usage either 1. is applied to localized strings, which should never contain an unknown specifier or 2. is acting on a string from the backend (IssueMessage) that should be a valid format string as well. ConsoleMessageView is kinda the exception in that we don't control the format string since it is author-provided.

>> LayoutTests/inspector/unit-tests/string-utilities-expected.txt:23
>> +WARN: String.format("%_ %s", "first", "second"): unsupported format character â_â. Treating as a string.
> 
> ewww we have smart quotes in error messages?  We should probably remove that (and switch to `WI.reportInternalError`).  Can be a followup if you'd prefer :)

If we make `ignoreUnknownSpecifiers` the default and non-configurable case, the first part of this will be irrelevant anyways. I'm not sure we want to reportInternalError, at least for the specific call-site of String.format we are dealing with here, since malformed user input shouldn't cause us to throw up the error handler in engineering builds. Every other (or almost every other?) call site to String.format probably should though... I'll do that as a followup so we can discuss that solution separately from this bug.
Comment 5 Patrick Angle 2022-03-22 10:45:43 PDT
Created attachment 455392 [details]
Patch v1.1
Comment 6 EWS 2022-03-22 15:23:24 PDT
Committed r291712 (248750@main): <https://commits.webkit.org/248750@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455392 [details].