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 Inspector | Assignee: | 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
zehexin6
2022-01-11 03:19:58 PST
Created attachment 455289 [details]
Patch v1.0
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 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. Created attachment 455392 [details]
Patch v1.1
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]. |