197091 2019-04-18 22:08:16 -0700 Web Inspector: REGRESSION: Elements: "Inspect Element" context menu often doesn't select that element 2019-04-19 17:30:42 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 hi hi commit-queue ews-watchlist hi inspector-bugzilla-changes joepeck webkit-bug-importer oldest_to_newest 1528703 0 hi 2019-04-18 22:08:16 -0700 # 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 1528704 1 hi 2019-04-18 22:08:25 -0700 <rdar://problem/49953728> 1528705 2 367786 hi 2019-04-18 22:16:55 -0700 Created attachment 367786 Patch 1528709 3 367786 joepeck 2019-04-18 23:03:06 -0700 Comment on attachment 367786 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. 1528757 4 367786 hi 2019-04-19 08:28:40 -0700 Comment on attachment 367786 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. 1528766 5 367801 hi 2019-04-19 08:50:05 -0700 Created attachment 367801 Patch This could be designed much better, but I think it would be better to tackle that in <https://webkit.org/b/197025>. 1528947 6 367801 joepeck 2019-04-19 16:34:05 -0700 Comment on attachment 367801 Patch r=me! Nice, I like this much better. 1528978 7 367801 commit-queue 2019-04-19 17:30:40 -0700 Comment on attachment 367801 Patch Clearing flags on attachment: 367801 Committed r244476: <https://trac.webkit.org/changeset/244476> 1528979 8 commit-queue 2019-04-19 17:30:42 -0700 All reviewed patches have been landed. Closing bug. 367786 2019-04-18 22:16:55 -0700 2019-04-19 08:50:02 -0700 Patch bug-197091-20190418221654.patch text/plain 2533 hi ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBiODgyNzcwZDJhNWU2ZDQ5OWEyNGMxZDE4N2RiODM0NzhlNTA5NDQ4Li5h YzI3YWIyODY0NzEzYmZlNDg1MDM0YWIwYzJlNTFjNjA4Y2E3YjYzIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMjAgQEAKKzIwMTktMDQtMTggIERldmluIFJvdXNzbyAgPGRyb3Vzc29AYXBwbGUuY29t PgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IFJFR1JFU1NJT046IEVsZW1lbnRzOiAiSW5zcGVj dCBFbGVtZW50IiBjb250ZXh0IG1lbnUgb2Z0ZW4gZG9lc24ndCBzZWxlY3QgdGhhdCBlbGVtZW50 CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTcwOTEK KyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzQ5OTUzNzI4PgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIERlbGF5IHRoZSBgaW5zcGVjdGAgZXZlbnQgZnJv biBmaXJpbmcgd2l0aCB0aGUgZm9jdWVkIG5vZGUgdW50aWwgdGhlIGZyb250ZW5kIGhhcyBoYWQg YQorICAgICAgICBjaGFuY2UgdG8gcmVxdWVzdCB0aGUgZG9jdW1lbnQuIE90aGVyd2lzZSwgcmVx dWVzdGluZyB0aGUgZG9jdW1lbnQgY2xlYXJzIHRoZSBtYXBwaW5nIG9mCisgICAgICAgIG5vZGUt dG8taWQsIHNvIHRoZSBmb2N1c2VkIG5vZGUncyBpZCB3b3VsZCBubyBsb25nZXIgYmUgdmFsaWQu CisKKyAgICAgICAgKiBpbnNwZWN0b3IvYWdlbnRzL0luc3BlY3RvckRPTUFnZW50LmNwcDoKKyAg ICAgICAgKFdlYkNvcmU6Okluc3BlY3RvckRPTUFnZW50OjpkaWRDcmVhdGVGcm9udGVuZEFuZEJh Y2tlbmQpOgorICAgICAgICAoV2ViQ29yZTo6SW5zcGVjdG9yRE9NQWdlbnQ6OmdldERvY3VtZW50 KToKKyAgICAgICAgKFdlYkNvcmU6Okluc3BlY3RvckRPTUFnZW50Ojpmb2N1c05vZGUpOgorCiAy MDE5LTA0LTE4ICBSb3NzIEtpcnNsaW5nICA8cm9zcy5raXJzbGluZ0Bzb255LmNvbT4KIAogICAg ICAgICBbV2luQ2Fpcm9dIE5vbi11bmlmaWVkIGJ1aWxkIGZhaWxzIHRvIGxpbmsgVG9vbHMKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2luc3BlY3Rvci9hZ2VudHMvSW5zcGVjdG9yRE9NQWdl bnQuY3BwIGIvU291cmNlL1dlYkNvcmUvaW5zcGVjdG9yL2FnZW50cy9JbnNwZWN0b3JET01BZ2Vu dC5jcHAKaW5kZXggMzM5NTVhYzUzMTcwZWZiMGZhZDk1MzJjNzZhZjBkMjE1MjAyNzM4ZS4uMTU3 ODU2ZjE3YzgwYmYyYTY1NjhhYmJiMmU4ODQwOTE3Y2UwZmFmMCAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvaW5zcGVjdG9yL2FnZW50cy9JbnNwZWN0b3JET01BZ2VudC5jcHAKKysrIGIvU291 cmNlL1dlYkNvcmUvaW5zcGVjdG9yL2FnZW50cy9JbnNwZWN0b3JET01BZ2VudC5jcHAKQEAgLTMx Myw5ICszMTMsNiBAQCB2b2lkIEluc3BlY3RvckRPTUFnZW50OjpkaWRDcmVhdGVGcm9udGVuZEFu ZEJhY2tlbmQoSW5zcGVjdG9yOjpGcm9udGVuZFJvdXRlciosCiAgICAgZm9yIChhdXRvKiBtZWRp YUVsZW1lbnQgOiBIVE1MTWVkaWFFbGVtZW50OjphbGxNZWRpYUVsZW1lbnRzKCkpCiAgICAgICAg IGFkZEV2ZW50TGlzdGVuZXJzVG9Ob2RlKCptZWRpYUVsZW1lbnQpOwogI2VuZGlmCi0KLSAgICBp ZiAobV9ub2RlVG9Gb2N1cykKLSAgICAgICAgZm9jdXNOb2RlKCk7CiB9CiAKIHZvaWQgSW5zcGVj dG9yRE9NQWdlbnQ6OndpbGxEZXN0cm95RnJvbnRlbmRBbmRCYWNrZW5kKEluc3BlY3Rvcjo6RGlz Y29ubmVjdFJlYXNvbikKQEAgLTUwNyw2ICs1MDQsMTMgQEAgdm9pZCBJbnNwZWN0b3JET01BZ2Vu dDo6Z2V0RG9jdW1lbnQoRXJyb3JTdHJpbmcmIGVycm9yU3RyaW5nLCBSZWZQdHI8SW5zcGVjdG9y OjoKICAgICBtX2RvY3VtZW50ID0gZG9jdW1lbnQ7CiAKICAgICByb290ID0gYnVpbGRPYmplY3RG b3JOb2RlKG1fZG9jdW1lbnQuZ2V0KCksIDIsICZtX2RvY3VtZW50Tm9kZVRvSWRNYXApOworCisg ICAgaWYgKG1fbm9kZVRvRm9jdXMpIHsKKyAgICAgICAgaWYgKG1fbm9kZVRvRm9jdXMtPmRvY3Vt ZW50KCkucGFnZSgpID09ICZtX2luc3BlY3RlZFBhZ2UpCisgICAgICAgICAgICBmb2N1c05vZGUo KTsKKyAgICAgICAgZWxzZQorICAgICAgICAgICAgbV9ub2RlVG9Gb2N1cyA9IG51bGxwdHI7Cisg ICAgfQogfQogCiB2b2lkIEluc3BlY3RvckRPTUFnZW50OjpwdXNoQ2hpbGROb2Rlc1RvRnJvbnRl bmQoaW50IG5vZGVJZCwgaW50IGRlcHRoKQpAQCAtMTEwNSw3ICsxMTA5LDcgQEAgdm9pZCBJbnNw ZWN0b3JET01BZ2VudDo6aW5zcGVjdChOb2RlKiBpbnNwZWN0ZWROb2RlKQogCiB2b2lkIEluc3Bl Y3RvckRPTUFnZW50Ojpmb2N1c05vZGUoKQogewotICAgIGlmICghbV9mcm9udGVuZERpc3BhdGNo ZXIpCisgICAgaWYgKCFtX2RvY3VtZW50UmVxdWVzdGVkKQogICAgICAgICByZXR1cm47CiAKICAg ICBBU1NFUlQobV9ub2RlVG9Gb2N1cyk7Cg== 367801 2019-04-19 08:50:05 -0700 2019-04-19 17:30:40 -0700 Patch bug-197091-20190419085004.patch text/plain 3318 hi ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3MGJlYzFjNDVlOGNmM2M4MjE1OGE4MDkwOWZhZjI3MmI5YWYzMDQ4Li45 OThjY2NhZTQ2NDA4ZjA2ZWM2ZjdiMTY5ODUyMDIxMjgzMGQ2OTdlIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMjIgQEAKKzIwMTktMDQtMTkgIERldmluIFJvdXNzbyAgPGRyb3Vzc29AYXBwbGUuY29t PgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IFJFR1JFU1NJT046IEVsZW1lbnRzOiAiSW5zcGVj dCBFbGVtZW50IiBjb250ZXh0IG1lbnUgb2Z0ZW4gZG9lc24ndCBzZWxlY3QgdGhhdCBlbGVtZW50 CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTcwOTEK KyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzQ5OTUzNzI4PgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIERlbGF5IHRoZSBgaW5zcGVjdGAgZXZlbnQgZnJv biBmaXJpbmcgd2l0aCB0aGUgZm9jdWVkIG5vZGUgdW50aWwgdGhlIGZyb250ZW5kIGhhcyBoYWQg YQorICAgICAgICBjaGFuY2UgdG8gcmVxdWVzdCB0aGUgZG9jdW1lbnQuIE90aGVyd2lzZSwgcmVx dWVzdGluZyB0aGUgZG9jdW1lbnQgY2xlYXJzIHRoZSBtYXBwaW5nIG9mCisgICAgICAgIG5vZGUt dG8taWQsIHNvIHRoZSBmb2N1c2VkIG5vZGUncyBpZCB3b3VsZCBubyBsb25nZXIgYmUgdmFsaWQu CisKKyAgICAgICAgKiBpbnNwZWN0b3IvYWdlbnRzL0luc3BlY3RvckRPTUFnZW50LmNwcDoKKyAg ICAgICAgKFdlYkNvcmU6Okluc3BlY3RvckRPTUFnZW50OjpkaWRDcmVhdGVGcm9udGVuZEFuZEJh Y2tlbmQpOgorICAgICAgICAoV2ViQ29yZTo6SW5zcGVjdG9yRE9NQWdlbnQ6OndpbGxEZXN0cm95 RnJvbnRlbmRBbmRCYWNrZW5kKToKKyAgICAgICAgKFdlYkNvcmU6Okluc3BlY3RvckRPTUFnZW50 OjpnZXREb2N1bWVudCk6CisgICAgICAgIChXZWJDb3JlOjpJbnNwZWN0b3JET01BZ2VudDo6Zm9j dXNOb2RlKToKKyAgICAgICAgKFdlYkNvcmU6Okluc3BlY3RvckRPTUFnZW50OjpkaWRDb21taXRM b2FkKToKKwogMjAxOS0wNC0xOCAgRnVqaWkgSGlyb25vcmkgIDxIaXJvbm9yaS5GdWppaUBzb255 LmNvbT4KIAogICAgICAgICBJbXBsZW1lbnQgS2V5ZWREZWNvZGVyR2VuZXJpYyBhbmQgS2V5ZWRF bmNvZGVyR2VuZXJpYwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvaW5zcGVjdG9yL2FnZW50 cy9JbnNwZWN0b3JET01BZ2VudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9pbnNwZWN0b3IvYWdlbnRz L0luc3BlY3RvckRPTUFnZW50LmNwcAppbmRleCAzMzk1NWFjNTMxNzBlZmIwZmFkOTUzMmM3NmFm MGQyMTUyMDI3MzhlLi40NmRkODJiNTg3OWY3N2QwZmRhNWM0YjZkODA0NTZlMjMxMmU5OGU5IDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9pbnNwZWN0b3IvYWdlbnRzL0luc3BlY3RvckRPTUFn ZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9pbnNwZWN0b3IvYWdlbnRzL0luc3BlY3RvckRP TUFnZW50LmNwcApAQCAtMzEzLDE2ICszMTMsMTUgQEAgdm9pZCBJbnNwZWN0b3JET01BZ2VudDo6 ZGlkQ3JlYXRlRnJvbnRlbmRBbmRCYWNrZW5kKEluc3BlY3Rvcjo6RnJvbnRlbmRSb3V0ZXIqLAog ICAgIGZvciAoYXV0byogbWVkaWFFbGVtZW50IDogSFRNTE1lZGlhRWxlbWVudDo6YWxsTWVkaWFF bGVtZW50cygpKQogICAgICAgICBhZGRFdmVudExpc3RlbmVyc1RvTm9kZSgqbWVkaWFFbGVtZW50 KTsKICNlbmRpZgotCi0gICAgaWYgKG1fbm9kZVRvRm9jdXMpCi0gICAgICAgIGZvY3VzTm9kZSgp OwogfQogCiB2b2lkIEluc3BlY3RvckRPTUFnZW50Ojp3aWxsRGVzdHJveUZyb250ZW5kQW5kQmFj a2VuZChJbnNwZWN0b3I6OkRpc2Nvbm5lY3RSZWFzb24pCiB7CiAgICAgbV9oaXN0b3J5LnJlc2V0 KCk7CiAgICAgbV9kb21FZGl0b3IucmVzZXQoKTsKKyAgICBtX25vZGVUb0ZvY3VzID0gbnVsbHB0 cjsKICAgICBtX21vdXNlZE92ZXJOb2RlID0gbnVsbHB0cjsKKyAgICBtX2luc3BlY3RlZE5vZGUg PSBudWxscHRyOwogCiAgICAgRXJyb3JTdHJpbmcgdW51c2VkOwogICAgIHNldFNlYXJjaGluZ0Zv ck5vZGUodW51c2VkLCBmYWxzZSwgbnVsbHB0cik7CkBAIC01MDcsNiArNTA2LDkgQEAgdm9pZCBJ bnNwZWN0b3JET01BZ2VudDo6Z2V0RG9jdW1lbnQoRXJyb3JTdHJpbmcmIGVycm9yU3RyaW5nLCBS ZWZQdHI8SW5zcGVjdG9yOjoKICAgICBtX2RvY3VtZW50ID0gZG9jdW1lbnQ7CiAKICAgICByb290 ID0gYnVpbGRPYmplY3RGb3JOb2RlKG1fZG9jdW1lbnQuZ2V0KCksIDIsICZtX2RvY3VtZW50Tm9k ZVRvSWRNYXApOworCisgICAgaWYgKG1fbm9kZVRvRm9jdXMpCisgICAgICAgIGZvY3VzTm9kZSgp OwogfQogCiB2b2lkIEluc3BlY3RvckRPTUFnZW50OjpwdXNoQ2hpbGROb2Rlc1RvRnJvbnRlbmQo aW50IG5vZGVJZCwgaW50IGRlcHRoKQpAQCAtMTEwNSw3ICsxMTA3LDcgQEAgdm9pZCBJbnNwZWN0 b3JET01BZ2VudDo6aW5zcGVjdChOb2RlKiBpbnNwZWN0ZWROb2RlKQogCiB2b2lkIEluc3BlY3Rv ckRPTUFnZW50Ojpmb2N1c05vZGUoKQogewotICAgIGlmICghbV9mcm9udGVuZERpc3BhdGNoZXIp CisgICAgaWYgKCFtX2RvY3VtZW50UmVxdWVzdGVkKQogICAgICAgICByZXR1cm47CiAKICAgICBB U1NFUlQobV9ub2RlVG9Gb2N1cyk7CkBAIC0yMTM2LDYgKzIxMzgsMTUgQEAgTm9kZSogSW5zcGVj dG9yRE9NQWdlbnQ6OmlubmVyUGFyZW50Tm9kZShOb2RlKiBub2RlKQogCiB2b2lkIEluc3BlY3Rv ckRPTUFnZW50OjpkaWRDb21taXRMb2FkKERvY3VtZW50KiBkb2N1bWVudCkKIHsKKyAgICBpZiAo bV9ub2RlVG9Gb2N1cyAmJiAmbV9ub2RlVG9Gb2N1cy0+ZG9jdW1lbnQoKSA9PSBkb2N1bWVudCkK KyAgICAgICAgbV9ub2RlVG9Gb2N1cyA9IG51bGxwdHI7CisKKyAgICBpZiAobV9tb3VzZWRPdmVy Tm9kZSAmJiAmbV9tb3VzZWRPdmVyTm9kZS0+ZG9jdW1lbnQoKSA9PSBkb2N1bWVudCkKKyAgICAg ICAgbV9tb3VzZWRPdmVyTm9kZSA9IG51bGxwdHI7CisKKyAgICBpZiAobV9pbnNwZWN0ZWROb2Rl ICYmICZtX2luc3BlY3RlZE5vZGUtPmRvY3VtZW50KCkgPT0gZG9jdW1lbnQpCisgICAgICAgIG1f aW5zcGVjdGVkTm9kZSA9IG51bGxwdHI7CisKICAgICBSZWZQdHI8RWxlbWVudD4gZnJhbWVPd25l ciA9IGRvY3VtZW50LT5vd25lckVsZW1lbnQoKTsKICAgICBpZiAoIWZyYW1lT3duZXIpCiAgICAg ICAgIHJldHVybjsK