Closed
Bug 1378017
Opened 8 years ago
Closed 7 years ago
inspector shows a non existing event
Categories
(DevTools :: Inspector, defect, P2)
Tracking
(firefox57 fix-optional)
RESOLVED
WORKSFORME
| Tracking | Status | |
|---|---|---|
| firefox57 | --- | fix-optional |
People
(Reporter: karlcow, Assigned: miker)
Details
Attachments
(2 files)
empty-event-listener-popup.png
With Firefox Nightly 56.0a1 (2017-06-29) (64-bit)
This is a spin-off of https://bugzilla.mozilla.org/show_bug.cgi?id=1376050
Create an account on orbitz.com
Go to: https://www.orbitz.com/user/account
Click "other travelers"
Inspect the birth date field.
See how it has an associated event with it.
Click the event and notice that it is empty.
capture d ecran 2017-06-29 a 17 19 12
Chrome and Safari do not display an event at all.
(Mistakenly reported on https://github.com/devtools-html/debugger.html/issues/3251 )
|
||
Comment 1•8 years ago
|
||
Mike, maybe this is something you can take a look at?
status-firefox57:
--- → fix-optional
Flags: needinfo?(mratcliffe)
|
|
||
Comment 2•8 years ago
|
||
|
|
||
Updated•8 years ago
|
Priority: -- → P2
|
Assignee | |
Comment 3•8 years ago
|
||
The situation there is this:
We count events without entering debug mode but that can give us some false positives.
If we enter debug mode it will be much more accurate but could slow the world down.
These days we can probably get away with entering debug mode to check.
Flags: needinfo?(mratcliffe)
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
OS: Unspecified → All
Hardware: Unspecified → All
| Comment hidden (mozreview-request) |
|
|
||
Comment 6•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8980683 [details]
Bug 1378017 - inspector shows a non existing event
https://reviewboard.mozilla.org/r/246852/#review253268
Indeed, the code is quite simple, I'm interested to see what it does to performance. Please make sure you push to talos before landing.
Also, I commented about possibly breaking the html/window/document event grouping feature below.
::: devtools/server/actors/inspector/event-parsers.js
(Diff revision 2)
> - if (node.nodeName.toLowerCase() === "html") {
> - let winListeners =
> - Services.els.getListenerInfoFor(node.ownerGlobal) || [];
> - let docElementListeners =
> - Services.els.getListenerInfoFor(node) || [];
> - let docListeners =
> - Services.els.getListenerInfoFor(node.parentNode) || [];
> -
> - listeners = [...winListeners, ...docElementListeners, ...docListeners];
Aren't we going to lose this feature though?
The new `_hasEventListeners` getter does not check for window, docElement and document listeners for <html> node anymore.
Attachment #8980683 -
Flags: review?(pbrosset)
|
|
Assignee | |
Comment 7•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8980683 [details]
Bug 1378017 - inspector shows a non existing event
https://reviewboard.mozilla.org/r/246852/#review253462
::: devtools/server/actors/inspector/event-parsers.js
(Diff revision 2)
> - if (node.nodeName.toLowerCase() === "html") {
> - let winListeners =
> - Services.els.getListenerInfoFor(node.ownerGlobal) || [];
> - let docElementListeners =
> - Services.els.getListenerInfoFor(node) || [];
> - let docListeners =
> - Services.els.getListenerInfoFor(node.parentNode) || [];
> -
> - listeners = [...winListeners, ...docElementListeners, ...docListeners];
Yes, I didn't have time to read over the patch before submitting it... will fix this now.
|
|
Assignee | |
Comment 8•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8980683 [details]
Bug 1378017 - inspector shows a non existing event
https://reviewboard.mozilla.org/r/246852/#review253498
::: devtools/server/actors/inspector/event-parsers.js
(Diff revision 2)
> - if (node.nodeName.toLowerCase() === "html") {
> - let winListeners =
> - Services.els.getListenerInfoFor(node.ownerGlobal) || [];
> - let docElementListeners =
> - Services.els.getListenerInfoFor(node) || [];
> - let docListeners =
> - Services.els.getListenerInfoFor(node.parentNode) || [];
> -
> - listeners = [...winListeners, ...docElementListeners, ...docListeners];
Actually, _hasEventListeners() uses getEventListenerInfo(), which *does* check for window, docElement and document listeners for the <html> node.
| Comment hidden (mozreview-request) |
|
|
||
Comment 10•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8980683 [details]
Bug 1378017 - inspector shows a non existing event
https://reviewboard.mozilla.org/r/246852/#review253930
Looks good to me, but please push to talos to check the impact of the change.
Attachment #8980683 -
Flags: review?(pbrosset) → review+
|
|
Assignee | |
Comment 11•7 years ago
|
||
ochameaus advice is to send multiple runs to DAMP:
With patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d74df74034dd54df229f655f49c199f065237bdd
Without patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceddf85a04d859ffacd9cad5079c188774774ffa
When they have run through we should get more accurate results.
|
|
Assignee | |
Comment 12•7 years ago
|
||
I knew that this issue was no longer reproduce-able with this testcase but because this problem keeps resurfacing I decided to fix it once and for all.
Seeing the performance hit we would take with the fix I decided to take a look at all the historic event bubble bugs to ensure that they are still unfixed but I can't reproduce this issue anywhere.
I cannot reproduce this issue anywhere. I can't find the exact changeset where I rewrote the core functionality because the file has been broken up and no longer exists but this now works fine without any further changes... I have verified this with various older bugs.
If we can find a situation where this is not fixed I would be happy to fix it.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
|
||
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•