Bug 216138 – Web Inspector: InvalidCharacterError: The string contains invalid characters.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

# STEPS TO REPRODUCE
1. inspect <https://www.nhl.com/video/>
2. Develop > Start Element Selection
3. click any of the video "cards" in the Most Popular section
4. go to the Node panel in the details sidebar in the Elements Tab
5. change the grouping of the Event Listeners section to be Group by Target

[Error] Assertion Failed: An internal exception was thrown. – InvalidCharacterError: The string contains invalid characters.
InvalidCharacterError: The string contains invalid characters.
	(anonymous function) (Main.js:3311)
	_dispatchResponseToCallback (Connection.js:154)
	_dispatchResponse (Connection.js:122)
	dispatch (Connection.js:77)
	dispatchMessageFromTarget (TargetManager.js:176)
	dispatchMessageFromTarget (TargetObserver.js:47)
	_dispatchEvent (Connection.js:210)
	dispatch (Connection.js:79)
	dispatch (InspectorBackend.js:232)
	(anonymous function) (MessageDispatcher.js:42)

[Radar WebKit Bug Importer]

<rdar://problem/68296440>

[Patrick Angle]

Created attachment 408274 [details]
Patch

[Devin Rousso]

Comment on attachment 408274 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:9
> +        `DetailsSection`, which could end up with illegal characters due to the escaping done by

I'm confused.  What logic actually changed to not display an illegal character?  It seems like all you're doing now is not calling `CSS.escape` for the shown text, which I think would reintroduce Bug 151378.

Perhaps we could just fix this by adding some extra character to the beginning of the string if it starts with a number right before we `CSS.escape` and then remove it after?
```
    let startsWithNumber = /\d/.test(id[0]);
    if (startsWithNumber)
        id = "n" + id;
    id = CSS.escape(id);
    if (startsWithNumber)
        it = it.substring(1);
```

[Patrick Angle]

Comment on attachment 408274 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:9
>> +        `DetailsSection`, which could end up with illegal characters due to the escaping done by
> 
> I'm confused.  What logic actually changed to not display an illegal character?  It seems like all you're doing now is not calling `CSS.escape` for the shown text, which I think would reintroduce Bug 151378.
> 
> Perhaps we could just fix this by adding some extra character to the beginning of the string if it starts with a number right before we `CSS.escape` and then remove it after?
> ```
>     let startsWithNumber = /\d/.test(id[0]);
>     if (startsWithNumber)
>         id = "n" + id;
>     id = CSS.escape(id);
>     if (startsWithNumber)
>         it = it.substring(1);
> ```

The difference between Bug 151378 and this is that in that bug the text was being used in the interface. We are looking for a way to identify the pieces of Web Inspectors DOM by adding the string as a class in `WI.DetailsSection`. This is why I added the option to not `CSS.escape` the text instead of just removing the escape completely. All existing code paths continue to escape the id and classes, but we can pull the new `identifier` property for the situations in which we don't want the escaped form.

Your fix would work as well, but for it we would still need to make sure we don't apply it when we actually want to use the result in the UI, otherwise we end up with the same problem you had in Bug 151378 where the selector displayed in the UI does not properly match because it is missing the escape characters.

[Devin Rousso]

Comment on attachment 408274 [details]
Patch

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

>>> Source/WebInspectorUI/ChangeLog:9
>>> +        `DetailsSection`, which could end up with illegal characters due to the escaping done by
>> 
>> I'm confused.  What logic actually changed to not display an illegal character?  It seems like all you're doing now is not calling `CSS.escape` for the shown text, which I think would reintroduce Bug 151378.
>> 
>> Perhaps we could just fix this by adding some extra character to the beginning of the string if it starts with a number right before we `CSS.escape` and then remove it after?
>> ```
>>     let startsWithNumber = /\d/.test(id[0]);
>>     if (startsWithNumber)
>>         id = "n" + id;
>>     id = CSS.escape(id);
>>     if (startsWithNumber)
>>         it = it.substring(1);
>> ```
> 
> The difference between Bug 151378 and this is that in that bug the text was being used in the interface. We are looking for a way to identify the pieces of Web Inspectors DOM by adding the string as a class in `WI.DetailsSection`. This is why I added the option to not `CSS.escape` the text instead of just removing the escape completely. All existing code paths continue to escape the id and classes, but we can pull the new `identifier` property for the situations in which we don't want the escaped form.
> 
> Your fix would work as well, but for it we would still need to make sure we don't apply it when we actually want to use the result in the UI, otherwise we end up with the same problem you had in Bug 151378 where the selector displayed in the UI does not properly match because it is missing the escape characters.

If we don't call `CSS.escape` though, I don't think we'd display a properly escaped selector for something like `id="#foo"` (which should be displayed as "#\\#foo"), which was the original issue in Bug 151378.

Is there a way that we could maybe instead find and un-escape any displayable code points after `CSS.escape`?  `\31 ` is displayable as "a", so it's not like we actually need to show it as an escape character.

[Patrick Angle]

Comment on attachment 408274 [details]
Patch

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

>>>> Source/WebInspectorUI/ChangeLog:9
>>>> +        `DetailsSection`, which could end up with illegal characters due to the escaping done by
>>> 
>>> I'm confused.  What logic actually changed to not display an illegal character?  It seems like all you're doing now is not calling `CSS.escape` for the shown text, which I think would reintroduce Bug 151378.
>>> 
>>> Perhaps we could just fix this by adding some extra character to the beginning of the string if it starts with a number right before we `CSS.escape` and then remove it after?
>>> ```
>>>     let startsWithNumber = /\d/.test(id[0]);
>>>     if (startsWithNumber)
>>>         id = "n" + id;
>>>     id = CSS.escape(id);
>>>     if (startsWithNumber)
>>>         it = it.substring(1);
>>> ```
>> 
>> The difference between Bug 151378 and this is that in that bug the text was being used in the interface. We are looking for a way to identify the pieces of Web Inspectors DOM by adding the string as a class in `WI.DetailsSection`. This is why I added the option to not `CSS.escape` the text instead of just removing the escape completely. All existing code paths continue to escape the id and classes, but we can pull the new `identifier` property for the situations in which we don't want the escaped form.
>> 
>> Your fix would work as well, but for it we would still need to make sure we don't apply it when we actually want to use the result in the UI, otherwise we end up with the same problem you had in Bug 151378 where the selector displayed in the UI does not properly match because it is missing the escape characters.
> 
> If we don't call `CSS.escape` though, I don't think we'd display a properly escaped selector for something like `id="#foo"` (which should be displayed as "#\\#foo"), which was the original issue in Bug 151378.
> 
> Is there a way that we could maybe instead find and un-escape any displayable code points after `CSS.escape`?  `\31 ` is displayable as "a", so it's not like we actually need to show it as an escape character.

Theoretically `String.prototype.normalize()` should be able to unescape the string for us. However, I don't think it's necessary. In this patch the only time we don't call `CSS.escape` is when we are getting the properties for use as a class on our DOM when building the sidebar. Previous we passed the `displayName` from `DOMNode` to both the title and identifier parameters of the constructor for `WI.DetailsSection`. Now we only pass the `CSS.escape` version for the title, which is what is actually displayed in the inspector. The identifier is used internally for us to keep track of the Inspector's DOM. For that case we could use a random number if we didn't want the elements to have more meaningful classes when inspecting the web inspector.

I'm attaching a screenshot showing after the patch that we still display the `CSS.escape` for of the ID as to not regress Bug 151378.

[Patrick Angle]

Created attachment 408285 [details]
Sidebar > Event Listeners – After patch.

We still correctly display the escaped selectors here.

[Devin Rousso]

Comment on attachment 408274 [details]
Patch

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

r=me with some comments :)

>>>>> Source/WebInspectorUI/ChangeLog:9
>>>>> +        `DetailsSection`, which could end up with illegal characters due to the escaping done by
>>>> 
>>>> I'm confused.  What logic actually changed to not display an illegal character?  It seems like all you're doing now is not calling `CSS.escape` for the shown text, which I think would reintroduce Bug 151378.
>>>> 
>>>> Perhaps we could just fix this by adding some extra character to the beginning of the string if it starts with a number right before we `CSS.escape` and then remove it after?
>>>> ```
>>>>     let startsWithNumber = /\d/.test(id[0]);
>>>>     if (startsWithNumber)
>>>>         id = "n" + id;
>>>>     id = CSS.escape(id);
>>>>     if (startsWithNumber)
>>>>         it = it.substring(1);
>>>> ```
>>> 
>>> The difference between Bug 151378 and this is that in that bug the text was being used in the interface. We are looking for a way to identify the pieces of Web Inspectors DOM by adding the string as a class in `WI.DetailsSection`. This is why I added the option to not `CSS.escape` the text instead of just removing the escape completely. All existing code paths continue to escape the id and classes, but we can pull the new `identifier` property for the situations in which we don't want the escaped form.
>>> 
>>> Your fix would work as well, but for it we would still need to make sure we don't apply it when we actually want to use the result in the UI, otherwise we end up with the same problem you had in Bug 151378 where the selector displayed in the UI does not properly match because it is missing the escape characters.
>> 
>> If we don't call `CSS.escape` though, I don't think we'd display a properly escaped selector for something like `id="#foo"` (which should be displayed as "#\\#foo"), which was the original issue in Bug 151378.
>> 
>> Is there a way that we could maybe instead find and un-escape any displayable code points after `CSS.escape`?  `\31 ` is displayable as "a", so it's not like we actually need to show it as an escape character.
> 
> Theoretically `String.prototype.normalize()` should be able to unescape the string for us. However, I don't think it's necessary. In this patch the only time we don't call `CSS.escape` is when we are getting the properties for use as a class on our DOM when building the sidebar. Previous we passed the `displayName` from `DOMNode` to both the title and identifier parameters of the constructor for `WI.DetailsSection`. Now we only pass the `CSS.escape` version for the title, which is what is actually displayed in the inspector. The identifier is used internally for us to keep track of the Inspector's DOM. For that case we could use a random number if we didn't want the elements to have more meaningful classes when inspecting the web inspector.
> 
> I'm attaching a screenshot showing after the patch that we still display the `CSS.escape` for of the ID as to not regress Bug 151378.

Oh!  I see.  This only affects the `identifier`, not the `title`.  Gotcha.  I'll do an actual review then :)

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:775
> +    get identifier()

This is possibly confusing in that there's already an `id` property on `WI.DOMNode`.  Perhaps we could call this `get unescapedSelector`?

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:780
> +        const escaped = false;

Style: it's usually fine to not have `const` variables if the function being called is in the same file, but I'm also a fan of this so no problem :)

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1052
> +    _idSelector(escaped = true)

NIT: this name is a bit ambiguous, perhaps `shouldEscape`?

NIT: I'd also not have a default parameter for this (at least not one that's truthy) so that someone reading `this._idSelector()` can know at the callsite whether or not the selector is escaped.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1070
> +    _classSelector(escaped = true) {

ditto (:1052)

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:357
> +            const identifier = `${options.identifier ?? title}-event-listener-section`

Style: we only use `const` for values that don't change between invocations of Web Inspector, so this should be `let` as both `options.identifier` and/or `title` can change

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:412
> +                let section = createEventListenerSection(title, eventListenersForTarget, {hideTarget: true, identifier: identifier});

Style: you can just do `{hideTarget: true, identifier}` when the key and value are the same :)

[Patrick Angle]

Created attachment 408292 [details]
Patch

[Blaze Burg]

Comment on attachment 408292 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1070
> +    _classSelector(shouldEscape) {

Please add a few test cases which demonstrates what this does for the easy and hard cases.

[Patrick Angle]

Created attachment 408352 [details]
Patch

[Blaze Burg]

Comment on attachment 408352 [details]
Patch

r=me

[Blaze Burg]

Comment on attachment 408352 [details]
Patch

Setting cq+ so this can land. I looked at the api-gtk EWS failures and while they are related to Web Inspector, it seems highly unlikely that a frontend-only change caused an API test to fail.

GTK port maintainers, please investigate whether this patch could have caused the test failures. We can revert if needed, but I have no way to further investigate the API failures using EWS.

[EWS]

Committed r266815: <https://trac.webkit.org/changeset/266815>

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

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #13)
> Comment on attachment 408352 [details]
> Patch
> 
> Setting cq+ so this can land. I looked at the api-gtk EWS failures and while
> they are related to Web Inspector, it seems highly unlikely that a
> frontend-only change caused an API test to fail.
> 
> GTK port maintainers, please investigate whether this patch could have
> caused the test failures. We can revert if needed, but I have no way to
> further investigate the API failures using EWS.

I'll investigate, but the bots started to fail after this commit, so I'm pretty sure this was the cause, see: 

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/15753

No need to revert it for now, I'll check them tomorrow.

[Carlos Garcia Campos]

Comment on attachment 408352 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:357
> +            let identifier = `${options.identifier ?? title}-event-listener-section`

This line is the problem, because of the missing ';'

resource:///org/webkit/inspector/UserInterface/Main.js:16767: CONSOLE JS ERROR SyntaxError: Unexpected keyword 'let'. Expected ';' after variable declaration.

I wonder why it doesn't fail in Mac. I'll submit a patch to bug #216361