RESOLVED FIXED 200889
Clicking the search icon on ae.com hangs the web content process 
https://bugs.webkit.org/show_bug.cgi?id=200889
Summary Clicking the search icon on ae.com hangs the web content process
Wenson Hsieh
Reported 2019-08-19 10:03:14 PDT
<rdar://problem/54359330>
Attachments
Patch (7.69 KB, patch)
2019-08-19 10:07 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
WIP patch (7.69 KB, patch)
2019-08-19 10:08 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Alternate approach (10.05 KB, patch)
2019-08-19 20:56 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Patch (15.24 KB, patch)
2019-08-20 09:25 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Patch (15.18 KB, patch)
2019-08-20 09:29 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2019-08-19 10:07:28 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 2 2019-08-19 10:08:48 PDT
Created attachment 376695 [details] WIP patch
Wenson Hsieh
Comment 3 2019-08-19 20:56:35 PDT
Created attachment 376740 [details] Alternate approach
Ryosuke Niwa
Comment 4 2019-08-19 22:10:47 PDT
Comment on attachment 376740 [details] Alternate approach View in context: https://bugs.webkit.org/attachment.cgi?id=376740&action=review > Source/WebCore/dom/Position.cpp:636 > + auto& renderer = *node->renderer(); > + return is<RenderElement>(renderer) && !Position::hasRenderedNonAnonymousDescendantsWithHeight(downcast<RenderElement>(renderer)); This doesn't seem right. It's possible for a text node to appear as direct children of a document node. > LayoutTests/ChangeLog:13 > + * fast/events/focus-anchor-with-tabindex-hang-expected.txt: Added. > + * fast/events/focus-anchor-with-tabindex-hang.html: Added. Please add another test which uses getSelection().modify(~) to reproduce the hang.
Wenson Hsieh
Comment 5 2019-08-20 09:23:36 PDT
Comment on attachment 376740 [details] Alternate approach View in context: https://bugs.webkit.org/attachment.cgi?id=376740&action=review >> Source/WebCore/dom/Position.cpp:636 >> + return is<RenderElement>(renderer) && !Position::hasRenderedNonAnonymousDescendantsWithHeight(downcast<RenderElement>(renderer)); > > This doesn't seem right. It's possible for a text node to appear as direct children of a document node. Hm…I’m a bit confused by this comment. This existing early return: if (!node->renderer()->isInline()) return true; …should ensure that we always consider the document element to have visually distinct ends, even in a page like this: <html> <style> html, body { display: inline; } </style> </html> Perhaps I misunderstood something? >> LayoutTests/ChangeLog:13 >> + * fast/events/focus-anchor-with-tabindex-hang.html: Added. > > Please add another test which uses getSelection().modify(~) to reproduce the hang. Good call! Added a new test.
Wenson Hsieh
Comment 6 2019-08-20 09:25:53 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 7 2019-08-20 09:29:02 PDT
Created attachment 376776 [details] Patch
Ryosuke Niwa
Comment 8 2019-08-20 11:14:12 PDT
(In reply to Wenson Hsieh from comment #5) > Comment on attachment 376740 [details] > Alternate approach > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376740&action=review > > >> Source/WebCore/dom/Position.cpp:636 > >> + return is<RenderElement>(renderer) && !Position::hasRenderedNonAnonymousDescendantsWithHeight(downcast<RenderElement>(renderer)); > > > > This doesn't seem right. It's possible for a text node to appear as direct children of a document node. > > Hm…I’m a bit confused by this comment. This existing early return: > > if (!node->renderer()->isInline()) > return true; > > …should ensure that we always consider the document element to have visually > distinct ends, even in a page like this: Oh, I missed that. Cool.
WebKit Commit Bot
Comment 9 2019-08-20 11:46:42 PDT
Comment on attachment 376776 [details] Patch Clearing flags on attachment: 376776 Committed r248909: <https://trac.webkit.org/changeset/248909>
WebKit Commit Bot
Comment 10 2019-08-20 11:46:44 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.