Open
Bug 1029451
Opened 10 years ago
Updated 2 years ago
[e10s] Synthesized mouseover blocked by EventUtils.disableNonTestMouseEvents(true) in mochitest-browser and dt
Categories
(Core :: DOM: Events, defect, P5)
Core
DOM: Events
Tracking
()
NEW
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: miker, Unassigned)
References
(Blocks 3 open bugs)
Details
Attachments
(1 file, 7 obsolete files)
9.69 KB,
patch
|
Details | Diff | Splinter Review |
Blocked by EventUtils.disableNonTestMouseEvents(true)... and 100% shouldn't be: - mouseover event.isSynthesized === false for synthesized events: - click - mouseover STR: 1. Apply the STR patch. 2. ./mach test browser/devtools/inspector/test/browser_inspector_disableNonTestMouseEvents.js 3. Change the value of TYPE to test other event types.
Reporter | ||
Updated•10 years ago
|
Component: Mochitest Chrome → DOM: Events
Product: Testing → Core
Reporter | ||
Comment 1•10 years ago
|
||
To be clear, synthesized mouseover events are blocked when EventUtils.disableNonTestMouseEvents(true) is set.
Reporter | ||
Comment 2•10 years ago
|
||
Updated•10 years ago
|
tracking-e10s:
--- → m7+
Updated•9 years ago
|
Summary: Synthesized mouseover blocked by EventUtils.disableNonTestMouseEvents(true) in mochitest-browser and dt → [e10s] Synthesized mouseover blocked by EventUtils.disableNonTestMouseEvents(true) in mochitest-browser and dt
Comment 3•9 years ago
|
||
This bug blocks bug 1029451, which is M6, so I propose to upgrade this bug to M6 as well.
Comment 4•9 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #3) > This bug blocks bug 1029451, which is M6, so I propose to upgrade this bug > to M6 as well. Sorry, that should have been bug 1029717.
Updated•9 years ago
|
Blocks: e10s-tests
Updated•9 years ago
|
Comment 6•9 years ago
|
||
Tom said he'd work on this.
Assignee: nobody → ttromey
Flags: needinfo?(ejpbruel)
Comment 7•9 years ago
|
||
If I change it to emit "click" and I remove the call to disableNonTestMouseEvents, it still fails, as the event actually seen is not marked mIsSynthesizedForTests. This happens in EventStateManager::CheckForAndDispatchClick, which creates a new WidgetMouseEvent but does not copy over this flag. I haven't looked to see what is going on with the other events yet.
Comment 8•9 years ago
|
||
It seems that the issue with "mouseover" is that the event winds up in PresShell::SynthesizeMouseMove, which drops the mIsSynthesizedForTests information. So, later in PresShell::ProcessSynthMouseMoveEvent, the event is re-created for dispatch without this.
Comment 9•9 years ago
|
||
A few spots were not preserving mIsSynthesizedForTests, so EventUtils.disableNonTestMouseEvents could not work properly -- it rejected too many events.
Updated•9 years ago
|
Attachment #8445121 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8586829 -
Flags: review?(botond)
Comment 10•9 years ago
|
||
Comment on attachment 8586829 [details] [diff] [review] preserve mIsSynthesizedForTests on mouse events Review of attachment 8586829 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable to me, but I don't know this code well enough to r+ it. Let's try :ehsan (:smaug would have been my first choice but he's overloaded with reviews at the moment). ::: layout/base/nsPresShell.h @@ +652,3 @@ > }; > + void ProcessSynthMouseMoveEvent(bool aFromScroll, bool > + aIsSynthesizedForTests); Avoid line wrapping between a parameter type and a parameter name.
Attachment #8586829 -
Flags: review?(ehsan)
Attachment #8586829 -
Flags: review?(botond)
Attachment #8586829 -
Flags: feedback+
Comment 11•9 years ago
|
||
Fix wrapping.
Attachment #8586829 -
Attachment is obsolete: true
Attachment #8586829 -
Flags: review?(ehsan)
Comment 12•9 years ago
|
||
> Avoid line wrapping between a parameter type and a parameter name.
Whoops.
Fixed in the follow-up.
Updated•9 years ago
|
Attachment #8586955 -
Flags: review?(ehsan)
Updated•9 years ago
|
Attachment #8586955 -
Flags: review?(ehsan) → review+
Comment 14•9 years ago
|
||
The try run had some unrelated failures, so rebasing to get a cleaner run. Carrying over the r+.
Attachment #8586955 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8587978 -
Flags: review+
Updated•9 years ago
|
Keywords: checkin-needed
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8882af540247
Flags: in-testsuite+
Keywords: checkin-needed
Comment 17•9 years ago
|
||
Backed out for intermittent timeouts. https://hg.mozilla.org/integration/mozilla-inbound/rev/0b743302cbfb https://treeherder.mozilla.org/logviewer.html#?job_id=8492845&repo=mozilla-inbound
Comment 19•9 years ago
|
||
Sorry about the delay. I've been working on this along two tracks. One is pushing various logging patches through try and trying to debug it remotely that way. However, the logging makes it much harder to see the bug. The other track is getting a Windows VM and development environment set up here so I can debug it directly. I'm moving slowly on this front. This is still one of my top priorities. I'll post more frequent updates here in the future.
Flags: needinfo?(ttromey)
Updated•9 years ago
|
Comment 20•9 years ago
|
||
Some good news -- I managed to reproduce the timeout locally using ChaosMode. Now on to debugging...
Comment 21•9 years ago
|
||
After a bunch more logging and debugging, I finally have a theory, which is that PresShell::SynthesizeMouseMove gets called with aIsSynthesizedForTests=false; and then later when the actually synthetic event is generated, it gets swallowed by the pending event: https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5593 After changing this, I haven't been able to reproduce locally -- but that isn't really definitive, as it's always been hit-or-miss. So I will push through try to see. It seems to me that ProcessSynthMouseMoveEvent could simply always set mIsSynthesizedForTests.
Comment 23•9 years ago
|
||
Here is a new version of the patch. I ran this one through the windows 7 opt try twelve times and could not make it fail. So I think I successfully found the intermittent. I'll of course do a full try run. This version unconditionally sets: event.mFlags.mIsSynthesizedForTests = true; in PresShell::ProcessSynthMouseMoveEvent. This is a bit odd because the member is named "for tests", but this isn't really the case here. It's possible to avoid this by making PresShell::SynthesizeMouseMove more complicated -- in particular making it not ever drop a synthesized event.
Attachment #8587978 -
Attachment is obsolete: true
Attachment #8598864 -
Flags: review?(ehsan)
Comment 24•9 years ago
|
||
Comment on attachment 8598864 [details] [diff] [review] preserve mIsSynthesizedForTests on mouse events Review of attachment 8598864 [details] [diff] [review]: ----------------------------------------------------------------- I think this is something that smaug can review better than I can...
Attachment #8598864 -
Flags: review?(ehsan) → review?(bugs)
Comment 25•9 years ago
|
||
Comment on attachment 8598864 [details] [diff] [review] preserve mIsSynthesizedForTests on mouse events PresShell::ProcessSynthMouseMoveEvent isn't right. ProcessSynthMouseMoveEvent is about dispatching a mouse events when users scrolls etc. Nothing to do with mIsSynthesizedForTests. Perhaps we want a global flag that these events should be processed as if mIsSynthesizedForTests was true, and that flag could be set in DOMWindowUtils so that tests could set if needed.
Attachment #8598864 -
Flags: review?(bugs) → review-
Comment 26•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #25) > Comment on attachment 8598864 [details] [diff] [review] > preserve mIsSynthesizedForTests on mouse events > > PresShell::ProcessSynthMouseMoveEvent isn't right. > ProcessSynthMouseMoveEvent is about dispatching a mouse events when > users scrolls etc. Nothing to do with mIsSynthesizedForTests. > > Perhaps we want a global flag that these events should be processed > as if mIsSynthesizedForTests was true, and that flag could be set in > DOMWindowUtils so that tests could set if needed. The issue is that if a mIsSynthesizedForTests event is generated and winds up in PresShell::SynthesizeMouseMove, it might be swallowed if another non-synthesized event shows up at just the right moment. This happens because PresShell only keeps note of a single synthetic mouse event. This would be ok, except that if sDisableNonTestMouseEvents is true, then the "swallowing" event is then later discarded. So, yeah, the approach in the patch is a bit of a hack. I could add a big comment if that would help. Another approach, maybe clearer, would be to change that line from "= true" to: event.mFlags.mIsSynthesizedForTests = sDisableNonTestMouseEvents; ... which might make it somewhat clearer. Yet another option I considered was to change ProcessSynthMouseMoveEvent to set a flag on the nsSynthMouseMoveEvent so that the mIsSynthesizedForTests would not be lost. I don't think a new global is needed. I think we have all the information, and it's just a matter of choosing the phrasing you like.
Flags: needinfo?(bugs)
Comment 27•9 years ago
|
||
event.mFlags.mIsSynthesizedForTests = sDisableNonTestMouseEvents; for that synthetic mouse event in presshell should be fine, I think.
Flags: needinfo?(bugs)
Comment 28•9 years ago
|
||
Here's the updated version.
Updated•9 years ago
|
Attachment #8598864 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8599336 -
Flags: review?(bugs)
Comment 30•9 years ago
|
||
Comment on attachment 8599336 [details] [diff] [review] preserve mIsSynthesizedForTests on mouse events Looks rather orange on try
Attachment #8599336 -
Flags: review?(bugs)
Comment 31•9 years ago
|
||
(but r+ for the C++ changes)
Comment 32•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #30) > Comment on attachment 8599336 [details] [diff] [review] > preserve mIsSynthesizedForTests on mouse events > > Looks rather orange on try Yeah, back to the drawing board :-(
Comment 33•9 years ago
|
||
If I take this version of the patch: https://bugzilla.mozilla.org/attachment.cgi?id=8586955&action=diff and change it to early-reject non-synthetic events in PresShell::SynthesizeMouseMove, like: + if (!aIsSynthesizedForTests && sDisableNonTestMouseEvents) { + return; + } ... then it works fine (tree seems orangey right now, but none in bc2): https://treeherder.mozilla.org/#/jobs?repo=try&revision=4290f670be98 I don't have a theory for why this would work but the patch setting mFlags.mIsSynthesizedForTests directly would not :-(
Comment 34•9 years ago
|
||
Here's the complete patch mentioned in comment #33. It seems to work. I think I was perhaps overthinking things earlier. Surely what was going on in the orange patch is that one of the synthetic events is preventing the test event from being dispatched; but this synthetic event is going to the wrong place. So, I think it's a question of either dropping all these synthetic events even earlier when sDisableNonTestMouseEvents is enabled (that's what this patch does); or maybe queuing more events in nsPresShell; or even just changing the test case and telling users of this testing API that they can't rely on synthetic mouseovers. I would like more feedback and/or advice on this.
Attachment #8599336 -
Attachment is obsolete: true
Updated•9 years ago
|
Flags: needinfo?(bugs)
Comment 35•9 years ago
|
||
queue more events in PresShell? What you mean with that? But I think changing the testcase would be fine.
Flags: needinfo?(bugs)
Comment 36•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #35) > queue more events in PresShell? What you mean with that? Currently PresShell::SynthesizeMouseMove allows just a single pending event at a time: https://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?from=nsPresShell.cpp#5593 I think what happens is that such an event arrives and blocks out the mIsSynthesizedForTests event, causing the test to fail.
Comment 37•9 years ago
|
||
Olli pointed out on irc that the event generated by SynthesizeMouseMove is dispatched when the refresh driver runs: if (!GetPresContext()->RefreshDriver()->AddRefreshObserver(ev, Flush_Display)) { ... so perhaps using requestAnimationFrame in the test would make it all work ok. I'll be trying that soon.
Comment 40•9 years ago
|
||
This has the requestAnimationFrame change in the test case. Initial results looked good so I sent it for a fuller test run.
Attachment #8601009 -
Attachment is obsolete: true
Comment 41•9 years ago
|
||
Well, that wasn't so good after all. Tomorrow I'll add logging again and see if I can figure out what exactly causes the event to be dropped.
Comment 44•9 years ago
|
||
Even more confused than before, because according to the latest log, the mouseover really is generated. It starts here with the call to nsContentUtils: 11:27:18 INFO - nsContentUtils::SendMouseEvent dispatching 322 to (13,13) This generates the NS_MOUSE_ENTER_WIDGET event. That's later turned into a mouse move: 11:27:18 INFO - entering SynthesizeMouseMove from PresShell::RecordMouseLocation, fromscroll=0 11:27:18 INFO - .. creating nsSynthMouseMoveEvent Which is dispatched and turned into a mouseover: 11:27:18 INFO - EventStateManager::NotifyMouseOver: 1 11:27:18 INFO - dispatching event, MOUSEOVER=1 The only theory I have right now is that the mouseover event is targeted at the wrong element.
Comment 47•9 years ago
|
||
Hey Tom, it seems unlikely that this bug is going to make it to devedition 40, doesn't it? Probably not a huge deal, since this just involves tests and not user-visible features, but I'd still like to confirm. Please let me know what you think.
Flags: needinfo?(ttromey)
Comment 48•9 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #47) > Hey Tom, it seems unlikely that this bug is going to make it to devedition > 40, doesn't it? Yeah :-( Last week I thought it would make it for sure. But after yesterday I am back to not knowing what is going wrong.
Flags: needinfo?(ttromey)
Comment 49•9 years ago
|
||
For whatever reason -- I can't explain it -- the most recent try run shows the test generally passing on non-e10s. It fails on e10s, which I'm happy(?) to report also happens here. So now I am debugging that. This debugging is going somewhat slowly. I do see the mouseover being delivered in the content process. However, evidently it does not get delivered to the correct element.
Comment 50•7 years ago
|
||
Hi, any progress on this one? I got the same problem that the "mouseover" would be blocked when I set EventUtils.disableNonTestMouseEvents(true). Thanks!
Blocks: 1274919
Updated•7 years ago
|
Flags: needinfo?(ttromey)
Comment 51•7 years ago
|
||
I'm sorry for the long delay on responding to this NI. I haven't worked on this in a couple of years. It's unclear to me if we even need it in devtools, seeing as we've lived without it so long. If you want to take it over, that's fine by me.
Flags: needinfo?(ttromey)
Updated•6 years ago
|
Priority: -- → P5
Comment 52•5 years ago
|
||
I never did get back to this :(
I wonder whether it is even needed any more.
Assignee: ttromey → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•