Open Bug 1029451 Opened 6 years ago Updated 10 months ago

[e10s] Synthesized mouseover blocked by EventUtils.disableNonTestMouseEvents(true) in mochitest-browser and dt

Categories

(Core :: DOM: Events, defect, P5)

defect

Tracking

()

Tracking Status
e10s + ---

People

(Reporter: miker, Unassigned)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file, 7 obsolete files)

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.
Component: Mochitest Chrome → DOM: Events
Product: Testing → Core
To be clear, synthesized mouseover events are blocked when EventUtils.disableNonTestMouseEvents(true) is set.
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
This bug blocks bug 1029451, which is M6, so I propose to upgrade this bug to M6 as well.
(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.
Blocks: e10s-tests
NI Eddy to get an assignee
Flags: needinfo?(ejpbruel)
Tom said he'd work on this.
Assignee: nobody → ttromey
Flags: needinfo?(ejpbruel)
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.
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.
A few spots were not preserving mIsSynthesizedForTests, so
EventUtils.disableNonTestMouseEvents could not work properly -- it
rejected too many events.
Attachment #8445121 - Attachment is obsolete: true
Attachment #8586829 - Flags: review?(botond)
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+
Fix wrapping.
Attachment #8586829 - Attachment is obsolete: true
Attachment #8586829 - Flags: review?(ehsan)
> Avoid line wrapping between a parameter type and a parameter name.

Whoops.
Fixed in the follow-up.
Attachment #8586955 - Flags: review?(ehsan)
Attachment #8586955 - Flags: review?(ehsan) → review+
The try run had some unrelated failures, so rebasing to get a cleaner
run.  Carrying over the r+.
Attachment #8586955 - Attachment is obsolete: true
Attachment #8587978 - Flags: review+
Keywords: checkin-needed
Hey ttromey, any update on this?
Flags: needinfo?(ttromey)
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)
Some good news -- I managed to reproduce the timeout locally using ChaosMode.
Now on to debugging...
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.
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 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 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-
(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)
event.mFlags.mIsSynthesizedForTests = sDisableNonTestMouseEvents; for that synthetic mouse event in
presshell should be fine, I think.
Flags: needinfo?(bugs)
Here's the updated version.
Attachment #8598864 - Attachment is obsolete: true
Attachment #8599336 - Flags: review?(bugs)
Comment on attachment 8599336 [details] [diff] [review]
preserve mIsSynthesizedForTests on mouse events

Looks rather orange on try
Attachment #8599336 - Flags: review?(bugs)
(but r+ for the C++ changes)
(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 :-(
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 :-(
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
Flags: needinfo?(bugs)
queue more events in PresShell? What you mean with that?

But I think changing the testcase would be fine.
Flags: needinfo?(bugs)
(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.
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.
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
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.
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.
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)
(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)
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.
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
Flags: needinfo?(ttromey)
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)
Priority: -- → P5

I never did get back to this :(
I wonder whether it is even needed any more.

Assignee: ttromey → nobody
You need to log in before you can comment on or make changes to this bug.