Closed Bug 1179048 Opened 8 years ago Closed 7 years ago

~4,000 instances of 'WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!' emitted from dom/events/EventListenerManager.cpp during mochitest devtools testing on linux64 debug

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: erahm, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Use mouseout instead of mouseleave in devtools
14.02 KB, patch
Details | Diff | Splinter Review 
>  3927 [NNNNN] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file dom/events/EventListenerManager.cpp, line 369

This warning [1] is currently one of the top 10 most verbose warnings and only shows up during the mochitest devtools chrome tests.

> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm122-tests1-linux64-build13.txt:2030
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm122-tests1-linux64-build1.txt:1292
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm114-tests1-linux64-build7.txt:591
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm114-tests1-linux64-build23.txt:14

It shows up in 179 tests, a few of the more verbose ones:

> 313 - browser/devtools/netmonitor/test/browser_net_sort-03.js
> 305 - browser/devtools/netmonitor/test/browser_net_filter-03.js
> 69 - browser/devtools/netmonitor/test/browser_net_sort-02.js

[1] https://hg.mozilla.org/mozilla-central/annotate/291614a686f1/dom/events/EventListenerManager.cpp#l367
Blocks: logspam
This is devtools issue. I've asked them to fix the issue, and filed at least one bug.
How can we get frontend people to use debug builds more often? Or should we start adding this kinds of warnings to the browserconsole or webconsole?
(In reply to Olli Pettay [:smaug] from comment #2)
> How can we get frontend people to use debug builds more often? Or should we
> start adding this kinds of warnings to the browserconsole or webconsole?

For warnings like these (intended for webdevs) we should really make them enabled in release, use the browser console, and only spew once per page. There are plenty of instances in layout where this would make sense as well.
Yes, I agree that placing these messages in the chrome page console / browser console would increase chances that action is taken.
(In reply to Eric Rahm [:erahm] from comment #3)
> For warnings like these (intended for webdevs) we should really make them
> enabled in release, use the browser console, and only spew once per page.
> There are plenty of instances in layout where this would make sense as well.
I'd say intended for browser chrome developers.
Investigating the JS callstack during |browser/devtools/netmonitor/test/browser_net_sort-03.js| points to widgets/Tooltip.js as the culprit:

> 0 Tooltip.prototype.startTogglingOnHover(baseNode = [object XULElement], targetNodeCb = [function]) ["resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/widgets/Tooltip.js":403]
>    this = [object Object]
Using mouseleave in chrome code generates a warning in docshell about
performance which notes mouseout should be used instead.
Attachment #8628347 - Flags: review?(nfitzgerald)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Attachment #8628347 - Flags: review?(nfitzgerald) → review+
(In reply to Olli Pettay [:smaug] from comment #5)
> (In reply to Eric Rahm [:erahm] from comment #3)
> > For warnings like these (intended for webdevs) we should really make them
> > enabled in release, use the browser console, and only spew once per page.
> > There are plenty of instances in layout where this would make sense as well.
> I'd say intended for browser chrome developers.

If this is only for browser chrome devs we might consider making it an assert once I land attachment #8628347 [details] [diff] [review].
That might be good, though if one uses some addons in debug build and those addons use
mouseenter/leave, it might be somewhat annoying to see those asserts.
But perhaps that isn't any real issue.
try run shows we still have 1590 instances:
>   1590 [NNNNN] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file dom/events/EventListenerManager.cpp, line 369

A dxr search [1] is showing a few files under devtools still using mouseleave:
  - breadcrumbs.js
  - markup-view.js
  - style-inspector-overlays.js

[1] https://dxr.mozilla.org/mozilla-central/search?limit=100&redirect=false&q=mouseleave%20path%3Abrowser/devtools%2F
This updates the rest of the devtools to use mouseout. There are now no
warnings in the try run.
Attachment #8628564 - Flags: review?(nfitzgerald)
Attachment #8628347 - Attachment is obsolete: true
Attachment #8628564 - Flags: review?(nfitzgerald) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/12b85be1c5a0 - I won't be able to do the full range of failures justice, you'll want to look at all of https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=85ce4ef19b93&filter-searchStr=devtools&filter-resultStatus=testfailed&tochange=3c124eec5ebd including the ones that appear to be known-intermittents but happened far far more frequently in that range than they should have.
Patrick (:pbro) has worked on the DevTools tooltips a lot, he may have some advice about the failures if needed.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> Patrick (:pbro) has worked on the DevTools tooltips a lot, he may have some
> advice about the failures if needed.

The primary culprit appears to be:
browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js

This test does not seem to fail on linux though, you can see the results of multiple retriggers here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24c24fa6765e

Patrick, any thoughts on what this could have broken?
Flags: needinfo?(pbrosset)
A few other failures of note:
- unstarred - 1928 INFO TEST-UNEXPECTED-FAIL | browser/devtools/markupview/test/browser_markupview_keybindings_01.js | Test timed out - expected PASS
- frequent - 6904 INFO TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_styleinspector_tooltip-closes-on-new-selection.js | Test timed out - expected PASS
I was unable to repro the browser_styleinspector_transform-highlighter-03.js on linux, I did manage to get a failure after 49 runs on OSX 10.10.3.

It stalls out at the '197 INFO Checking that the highlighter gets hidden when hovering a non-transform property' step both locally and in the try run.
The only way I can repro this behavior is if my mouse cursor happened to be over the location of the rules panel prior to testing, this does not seem to happen prior to this patch.

Olli, any thoughts?
Flags: needinfo?(bugs)
we have still some mouseenter/leave listeners devtools, and we should try to get rid of them.
I don't know how the patch itself could trigger that.
Flags: needinfo?(bugs)
(In reply to Eric Rahm [:erahm] from comment #17)
> The primary culprit appears to be:
> browser/devtools/styleinspector/test/browser_styleinspector_transform-
> highlighter-03.js
> 
> This test does not seem to fail on linux though, you can see the results of
> multiple retriggers here:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=24c24fa6765e
> 
> Patrick, any thoughts on what this could have broken?
I don't know about this particular test, but changing all mouseleave for mouseout is bound to cause changes in behavior. mouseleave only fires when you leave the element, not when you leave child elements. mouseout though fires also if you leave a child element.
In the rule-view, for example (where css rules are displayed, in the inspector panel), we display a bunch of different tooltips when you move your mouse over some properties.
Just changing mouseleave to mouseout in Tooltip.js is probably not enough then, we will need some more changes to make sure the tooltip behavior remains unchanged.
We have similar tooltips in the inspector markup-view and in the debugger. They all use Tooltip.js.
Flags: needinfo?(pbrosset)
mouseleave is fired whenever you mouseout any element. It is just that mouseleave doesn't bubble.
The performance issue with mouseenter/leave is that there are _lots_ of more those events than
mouseover/out, and we particularly optimize out firing mouseenter/leave if there are no listeners for those.
This looks like a dupe of bug 1138787.
Patrick, in bug 971203, comment 7 you mention that to switch over to mouseout "we also need to early return from _onBaseNodeMouseOut if the event target is a child of baseNode." 

Can you elaborate on determining if the event target is a child of baseNode? I'd like to see if adding that check fixes the test failures from this patch.
Flags: needinfo?(pbrosset)
(In reply to Olli Pettay [:smaug] from comment #23)
> mouseleave is fired whenever you mouseout any element. It is just that
> mouseleave doesn't bubble.
> The performance issue with mouseenter/leave is that there are _lots_ of more
> those events than
> mouseover/out, and we particularly optimize out firing mouseenter/leave if
> there are no listeners for those.
Thanks Olli for the precision.

(In reply to Eric Rahm [:erahm] from comment #25)
> Patrick, in bug 971203, comment 7 you mention that to switch over to
> mouseout "we also need to early return from _onBaseNodeMouseOut if the event
> target is a child of baseNode." 
> 
> Can you elaborate on determining if the event target is a child of baseNode?
> I'd like to see if adding that check fixes the test failures from this patch.
So _onBaseNodeMouseOut will be called when the event bubbles from children of _baseNode. But we only want to execute this function if the mouse is actually outside of _baseNode, not when it moved out of one of its children, but is still over _baseNode.
So I guess we could do something like:
_onBaseNodeMouseOut: function(e) {
  if (this._baseNode.contains(e.target)) {
    // skip descendants
    return;
  }
  ... the rest of the function ...
}
Flags: needinfo?(pbrosset)
This is a WIP. I think I've done what Patrick suggested, but I am now seeing a timeout during 'browser/devtools/inspector/test/browser_inspector_highlighter-hover_02.js' (on Linux at least).

At this point it might make more sense for someone in devtools to take over.
Attachment #8628564 - Attachment is obsolete: true
Assignee: erahm → nobody
Status: ASSIGNED → NEW
Panos, any chance you could look into this further? It looks like Patrick is out for a while.
Flags: needinfo?(past)
Sorry, I'm very busy with another project at the moment and I am off on vacation next week. bgrins and mikeratcliffe have some experience with this code, but I wouldn't be surprised if they are also swamped these days.
Flags: needinfo?(past)
I am swamped getting the storage inspector tests working on E10S... I would be happy to take a look when I finish though.

If there is no hurry then feel free to assign it to me.
Thank you Michael, not a huge rush so if you can take a look when you have time that would be great.

I'll ping this bug again when it hits #1 most verbose.
Assignee: nobody → mratcliffe
This has now reach #1.
Thanks Eric for putting this together.
There's a problem with the patch though whereby moving the mouse outside of the markup-view doesn't hide the highlighter and doesn't remove the node hover background style.
The same happens with the CssTransformHighlighter when hovering over properties like "transform:rotate(10deg)" in the rule-view.
I'll try to improve the patch a little bit.
This second version should get rid of the problems mentioned in my last comment.
I don't have a lot of time to push this any further, but hopefully this helps Mike.
Attachment #8636156 - Attachment is obsolete: true
I ran the latest update through try, it looks like there are still some failures. Of note:

> browser/devtools/styleinspector/test/browser_styleinspector_tooltip-closes-on-new-selection.js | Test timed out - expected PASS
> browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js | Uncaught exception - at chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js:62 - TypeError: hs._onMouseLeave is not a function
(In reply to Eric Rahm [:erahm] from comment #38)
> I ran the latest update through try, it looks like there are still some
> failures. Of note:
> 
> > browser/devtools/styleinspector/test/browser_styleinspector_tooltip-closes-on-new-selection.js | Test timed out - expected PASS
For info, this test has been basically perma-failing on Linux in the recent days (see bug 1093431), so that's not cause by your patch I believe.
(In reply to Eric Rahm [:erahm] from comment #38)
> > browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js | Uncaught exception - at chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js:62 - TypeError: hs._onMouseLeave is not a function
This one should be an easy fix, the test just overrides the _onMouseLeave callback, which now doesn't exist anymore.
This remains the #1 most verbose warning during testing.
Assignee: mratcliffe → nobody
Unbitrotted the patch and updated a test that was referencing _onMouseLeave.
Attachment #8645750 - Attachment is obsolete: true
Alright latest try seems pretty happy, apparently M-dt1 on OSX is just a sad sad place and the failures are unrelated. Lets finally get this landed.
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13fe047c4015
Use mouseout instead of mouseleave in devtools. r=fitzgen
https://hg.mozilla.org/mozilla-central/rev/13fe047c4015
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1290680
Depends on: 1290063
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.