RESOLVED FIXED 197091
Web Inspector: REGRESSION: Elements: "Inspect Element" context menu often doesn't select that element 
https://bugs.webkit.org/show_bug.cgi?id=197091
Summary Web Inspector: REGRESSION: Elements: "Inspect Element" context menu often doe...
Devin Rousso
Reported 2019-04-18 22:08:16 PDT
# STEPS TO REPRODUCE: 1. go to any large-ish page 2. right click on a nested/specific element (e.g. not the background or the <body>) => inspector opens, sometimes on the Elements tab with a different node selected and sometimes not even on the Elements tab
Attachments
Patch (2.47 KB, patch)
2019-04-18 22:16 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (3.24 KB, patch)
2019-04-19 08:50 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2019-04-18 22:08:25 PDT
<rdar://problem/49953728>
Devin Rousso
Comment 2 2019-04-18 22:16:55 PDT
Created attachment 367786 [details] Patch
Joseph Pecoraro
Comment 3 2019-04-18 23:03:06 PDT
Comment on attachment 367786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367786&action=review > Source/WebCore/ChangeLog:9 > + Delay the `inspect` event fron firing with the focued node until the frontend has had a Typo: "fron" > Source/WebCore/ChangeLog:11 > + node-to-id, so the focused node's id would no longer be valid. Interesting, so why is this happening now? Is it related to lazy initialization!? > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:509 > + if (m_nodeToFocus->document().page() == &m_inspectedPage) This check feels like it came after some kind of experimentation. It might deserve a comment since it is very non-obvious. Should we have cleared `m_nodeToFocus` when the page changed? The reason I ask is that `m_nodeToFocus` is a `RefPtr<Node>`, so if it is non-null it is holding a node which in turn is keeping that entire node's Document and Page alive. If that page is not the inspected page, then it sounds like Web Inspector might somehow be keeping an entire page alive.
Devin Rousso
Comment 4 2019-04-19 08:28:40 PDT
Comment on attachment 367786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367786&action=review >> Source/WebCore/ChangeLog:11 >> + node-to-id, so the focused node's id would no longer be valid. > > Interesting, so why is this happening now? Is it related to lazy initialization!? That's my guess, but I'm not entirely sure why 🤔 >> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:509 >> + if (m_nodeToFocus->document().page() == &m_inspectedPage) > > This check feels like it came after some kind of experimentation. It might deserve a comment since it is very non-obvious. > > Should we have cleared `m_nodeToFocus` when the page changed? > > The reason I ask is that `m_nodeToFocus` is a `RefPtr<Node>`, so if it is non-null it is holding a node which in turn is keeping that entire node's Document and Page alive. If that page is not the inspected page, then it sounds like Web Inspector might somehow be keeping an entire page alive. I think I had this backwards in my mind. We should be clearing `m_nodeToFocus` if the owner frame becomes detached from the main frame (or the owner is the main frame and it's about to navigate/deallocate). This check can then be removed.
Devin Rousso
Comment 5 2019-04-19 08:50:05 PDT
Created attachment 367801 [details] Patch This could be designed much better, but I think it would be better to tackle that in <https://webkit.org/b/197025>.
Joseph Pecoraro
Comment 6 2019-04-19 16:34:05 PDT
Comment on attachment 367801 [details] Patch r=me! Nice, I like this much better.
WebKit Commit Bot
Comment 7 2019-04-19 17:30:40 PDT
Comment on attachment 367801 [details] Patch Clearing flags on attachment: 367801 Committed r244476: <https://trac.webkit.org/changeset/244476>
WebKit Commit Bot
Comment 8 2019-04-19 17:30:42 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.