GNOME Bugzilla – Bug 788426
Crash (hyperlink related)
Last modified: 2017-10-02 17:22:53 UTC
Created attachment 360770 [details] backtrace I had a seemingly fresh g-t window open. Just one window, one tab. I can't remember if I had done anything there before, probably not. The prompt was in the upmost line. I typed "man bash" and pressed Enter. The cursor moved to the beginning of the next line, but the bash manual never appeared. A few seconds later g-t crashed (I guess those few seconds are when apport collects the crash details). I have padding: 15px; for testing purposes. I might have clicked / scrolled with the touchbar, perhaps while the mouse was over the padding, dunno. What the hack is that x = 1370052432, y = 21962 in the backtrace? Uninitialized garbage, result of overflow...? Suspicious.
It was shortly after a reboot. As such, "man bash" took longer than usual to start up (e.g. even the binaries of "man", "groff" etc. and "less" as well had to be read from the disk – if we got this far at all, which I cannot tell).
Also, for the record: I have the patch from bug 754936 installed. Maybe (hopefully) that's the culprit (although I find it unlikely because that's about regex match and not about hyperlink).
I'm almost certain the window had the default 80x24 size. My font size is 7x15.
bbox is uninitialized and that's okay. The problem is: glong col = pos.x / m_char_width; This results in col == -1, and later rowdata->cells[col].attr.hyperlink_idx is accessed. I guess this fix should do it: - if (rowdata && col < rowdata->len) { + if (rowdata && col >= 0 && col < rowdata->len) { We should double check if row needs similar safety caps too, or if that's handled correctly by find_row_data(). We shold also do some root cause analysis on how pos.x could be negative. A simple division (without padding correction) results in correct location computation (highlighting coordinates aren't off by the giant padding), so it's already relative to the upper-left cell's corner. Maybe related to bug 732185?
When the mouse is moved, the coordinates are confined to the actual terminal area (non-padding) area. When some output is produced, however, the coordinates are not confined. After the division with the charcell size, both coordinates are easily -1 or even smaller in case of huge paddings; or greater than or equal to the width/height.
"When some output is produced" -> This is near the end of emit_pending_signals() where both hyperlink_hilite_update() and match_hilite_update() are called with non-confined mouse coordinates. I guess (I hope!) match_hilite_update() handles it somewhere deep inside, although I'm not absolutely sure. As for hyperlink_hilite_update(), pixel_to_row() easily returns a row that's just scrolled out, and then we locate the hyperlink index of the corresponding offscreen text. I'm wondering how come that it doesn't cause an assertion failure similarly to bug 787283. The fix of 787283 introduced a check of view_coords_visible(pos), I think this check should also guard the lookup of the cell's data. Also I feel like after today's quick fix for 0.50.1, we should do a more proper cleanup, e.g. make sure that moving the mouse or printing data should handle the mouse-over-padding case the same way.
Created attachment 360788 [details] [review] Fix v2 (v1 was inlined in a previous comment)
IMHO you first check !view_coords_visible() and in that case just unhilite and return. And then instead of doing the computation manually, just use confined_grid_coords_from_view_coords().
IMHO you should...
> IMHO you first check !view_coords_visible() and in that case just unhilite > and return. I've thought about it. That would get rid of checking view_coords_visible() twice (which I also don't like). However, there's more to the story than just unhiliting and returning. See the end of the method: something about invalidating the regex match as well, changing the mouse cursor, emitting the proper signal, let alone some useful debugging messages throughout the function. These would all need to be duplicated then which is even uglier and much more fragile. > And then instead of doing the computation manually, just use > confined_grid_coords_from_view_coords(). Sure, will do.
Created attachment 360793 [details] [review] Fix v3 > just use confined_grid_coords_from_view_coords() Did you think of something like this? (Note that there's no point in "confined_" once it needs to be within such a condition anyways.)
Looks fine, just one nit: + if (view_coords_visible(pos)) { There's now 2 invocations of this in the function, just store it in a local var on the first occsation then use that later. Ok for 0-50 too. Thanks!
Submitted, closing.