| Summary: | Web Inspector: Unhandled exception when moving cursor mid-token after receiving CSS property name completions | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Razvan Caliman <rcaliman> | ||||||||||||||
| Component: | Web Inspector | Assignee: | Razvan Caliman <rcaliman> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | ews-watchlist, hi, inspector-bugzilla-changes, pangle, webkit-bug-importer | ||||||||||||||
| Priority: | P1 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=227157 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 447362 [details]
Patch 1.0
Comment on attachment 447362 [details] Patch 1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=447362&action=review > Source/WebInspectorUI/ChangeLog:11 > + If the cursor is moved too far back within a token that is a prefix for the previous completion results, > + adjusting the cursor position can return a negative index. Clamping the adjusted cursor position solves the crash. > + The cursor position is used to visually align the completion suggestions list. I think the actual bug here was the one you discuss below, and this just happened to be where that bug lead to a crash. I'd flip this around, and discuss this clamping as failsafe of sorts to catch these sorts of problems, but emphasis the below solution more since it was the root cause of the issue as I understand it. > Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:36 > + console.assert(caretPosition === text.length, text, caretPosition, "Completions mid-token are not yet supported."); Is this actually a safe assumption? It seems like SpreadsheetTextField will still provide an actual caret position, even if the caret isn't at the end of the text. I think the fixed `if (caretPosition !== text.length)` solves the problem, and asserting here will only be noise, since this is still a state that I believe it is possible to get into. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:484 > + // Handle the case where the caret is not placed at the end of the prefix and ajustment leads to a negative index. NIT: s/ajustment/adjustment > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:487 > + // For example: > + // > + // b|order NIT: I'd just put this on a single line. e.g. ``` For example: `b|order` ``` > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:489 > + if (adjustedCaretPosition < 0) > + adjustedCaretPosition = caretPosition; I'd consider this to be an exceptional case now that you've fixed the cause in `WI.CSSKeywordCompletions.forPartialPropertyName`. I'd suggest this be an assertion followed by the failsafe `if (adjustedCaretPosition < 0)`, since it shouldn't be a reachable case now. Created attachment 447374 [details]
Patch 1.1
Address code review:
- Reword Changelog entry to reflect true root cause
- Remove mistaken assertion for token position
- Fix nits
Comment on attachment 447374 [details] Patch 1.1 View in context: https://bugs.webkit.org/attachment.cgi?id=447374&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:486 > + console.assert(adjustedCaretPosition >= 0, adjustedCaretPosition); Is this actually possible to have happen? The comment suggests that it is. If so, we shouldn't have a `console.assert`. If not, I'd remove the `if` since it's effectively dead code. Comment on attachment 447374 [details] Patch 1.1 View in context: https://bugs.webkit.org/attachment.cgi?id=447374&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:486 >> + console.assert(adjustedCaretPosition >= 0, adjustedCaretPosition); > > Is this actually possible to have happen? The comment suggests that it is. If so, we shouldn't have a `console.assert`. If not, I'd remove the `if` since it's effectively dead code. I'd keep both. The condition is a fail-safe. The unhandled exception happened as a side-effect from a bug elsewhere (completions returned with an unexpectedly long prefix). That's been fixed in this patch and it will not trigger again for _this_ scenario. But mutating `this._completionPrefix` elsewhere can lead to the same issue. An unhandled exception in Web Inspector is noisy and the bug is also dependent on user input: for example, the caret must be placed just so for the adjusted position to be negative. Clamping ensures that the out-of-range exception cannot happen. The assertion is here because I expect to hit this again once we implement mid-token completions where `this._completionPrefix` may be mutated. It will be a reminder to ensure the `adjustedCaretPosition` is correctly computed. Comment on attachment 447374 [details] Patch 1.1 View in context: https://bugs.webkit.org/attachment.cgi?id=447374&action=review > Source/WebInspectorUI/ChangeLog:3 > + Web Inspector: Crash when moving cursor mid-token after receiving CSS property name completions NIT: Make sure to update this to match the new bug name. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:485 > + // Handle the case where the caret is not placed at the end of the prefix and adjustment leads to a negative index. > + // For example: b|order NIT: I'd drop this comment, as it is only really applicable to the case you've corrected with the fix in CSSKeywordCompletions.js, so it shouldn't occur any more. >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:486 >>> + console.assert(adjustedCaretPosition >= 0, adjustedCaretPosition); >> >> Is this actually possible to have happen? The comment suggests that it is. If so, we shouldn't have a `console.assert`. If not, I'd remove the `if` since it's effectively dead code. > > I'd keep both. The condition is a fail-safe. > > The unhandled exception happened as a side-effect from a bug elsewhere (completions returned with an unexpectedly long prefix). That's been fixed in this patch and it will not trigger again for _this_ scenario. > > But mutating `this._completionPrefix` elsewhere can lead to the same issue. An unhandled exception in Web Inspector is noisy and the bug is also dependent on user input: for example, the caret must be placed just so for the adjusted position to be negative. Clamping ensures that the out-of-range exception cannot happen. > > The assertion is here because I expect to hit this again once we implement mid-token completions where `this._completionPrefix` may be mutated. It will be a reminder to ensure the `adjustedCaretPosition` is correctly computed. I think the distinction here is that we shouldn't need a fail-safe for a condition that shouldn't be reachable under current circumstances. The correctness of the input here relies on our own code being correct, not on the user doings things "just right". I think Devin is right that we should drop the `if`. I'd also suggest adding a FIXME here that links to the mid-token completion bug to make it harder for a future implementer to miss. Both the fail-safe as well as just letting the issue run its course will result in unexpected behavior from the user perspective anyways, as far as I can tell. Also, an unhandled exception should only be noisy in engineering builds; it should be silent to the average user in a release build. Created attachment 448505 [details]
Patch 1.2
Address code review comments:
- dropped clamping of `adjustedCaretPosition`
- updated Changelog bug name
- added FIXME reference to bug for supporting mid-token completions
Comment on attachment 448505 [details] Patch 1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=448505&action=review Should have thought of this earlier, but we should add a test case for this that would have failed previously and passes now in `/LayoutTests/inspector/unit-tests/css-keyword-completions.html` Otherwise this patch is in good shape. > Source/WebInspectorUI/ChangeLog:26 > + A fail-safe in `WI.SpreadsheetTextField._showSuggestionsView()` also prevents the adjusted caret position > + from ever going negative and throwing an exception. Oops: Left over from a previous iteration of the patch, I think - This doesn't appear to apply any more. Created attachment 448954 [details]
Patch 1.3
Add test for mid-token completion expecting to return zero results. Remove obsolete comments from Changelog
Comment on attachment 448954 [details] Patch 1.3 View in context: https://bugs.webkit.org/attachment.cgi?id=448954&action=review rs=me, just need to clean up the LayoutTests ChangeLog. > LayoutTests/ChangeLog:9 > + patch Oops? I'd just not include any notes here instead. Created attachment 448962 [details]
Patch 1.3
Carry over R+; Fix nit in Changelog
Committed r287934 (245965@main): <https://commits.webkit.org/245965@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448962 [details]. |
Created attachment 447359 [details] Video recording of bug *Steps to reproduce* - Click to add a new CSS declaration in the Styles sidebar - Type `margin` in the name field - Without committing, move the cursor to the beginning of the name: `|margin` - Type the letter `s` See attached video. *Result* Web Inspector crashes with unhandled exception due to the cursor position being out of range.