Closed Bug 675669 Opened 9 years ago Closed 9 years ago

Synthesized mouse event dispatched to incorrect frame tree in some mobile mochitests

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED INVALID
mozilla8

People

(Reporter: jdm, Assigned: jdm)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

With variations on the patches in bug 668283 and its dependencies applies, as well as patches to get waitForFocus running, the following situation occurs when running content/base/test in the overarching mochitest harness:

1. content/base/test/test_bug330925.xhtml runs successfully
2. content/base/test/test_bug33199.html starts, gains focus, and attempts to dispatch a synthesized mouse event
3. the event is dispatched, but is not seen by the correct handler, and the test sits there until it times out

This occurs because at the time when the event is dispatched, the frame tree that is built for the dispatch still reflects the content of the previous test file (test_bug330925.xhtml).

The problem is made harder because adding breakpoints to functions like nsFrameManagerBase::SetRootFrame causes the test to pass. Similarly, adding an assertion to nsFrameManagerBase::SetRootFrame and using XPCOM_DEBUG_BREAK=stack causes the test to pass, but XPCOM_DEBUG_BREAK=warn causes it to fail again. Therefore, this suggests that this is a timing issue, but I'm confused about what sort of non-determinism could be involved here.
I've determined that using utils.sendMouseEvent as synthesizeMouseEvent currently does causes problems here, while utils.sendMouseEventToWindow makes the test pass. The relevant code that this affects:

>  if (aToWindow) {
>    nsIPresShell* presShell = presContext->PresShell();
>    if (!presShell)
>      return NS_ERROR_FAILURE;
>    nsCOMPtr<nsIViewObserver> vo = do_QueryInterface(presShell);
>    if (!vo)
>      return NS_ERROR_FAILURE;
>    nsIViewManager* viewManager = presShell->GetViewManager();
>    if (!viewManager)
>      return NS_ERROR_FAILURE;
>    nsIView* view = viewManager->GetRootView();
>    if (!view)
>      return NS_ERROR_FAILURE;
>
>    status = nsEventStatus_eIgnore;
>    return vo->HandleEvent(view, &event, PR_FALSE, &status);
>  }
>  return widget->DispatchEvent(&event, status);

Apparently the widget dispatch gets us in trouble here.
In case anybody else wants to follow along at home, here is the minimum set of patches that allows you to run |TEST_PATH=content/base/test/foo make -C objdir/ mochitest-plain| and see the tests time out.
Assignee: nobody → josh
Boom.
Attachment #551604 - Flags: review?(ehsan)
Does this change what happens when users actually click on things, or just what happens when synthesizing events?
(In reply to David Baron [:dbaron] from comment #5)
> Does this change what happens when users actually click on things, or just
> what happens when synthesizing events?

I suspect it's quite possible to hit this without synthesizing, since this happens underneath PresShell::HandleEvent. I've filed bug 677405 for auditing other uses of these functions too.
Comment on attachment 551604 [details] [diff] [review]
Ignore paint suppression for hit testing

FWIW, this _can_ happen whenever the user clicks on something before the initial paint supression timeout fires.  Although that might be unlikely in practice, since it's really odd to imagine that the user has meaningfully clicked on something without seeing it on the screen.
Attachment #551604 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/6835028a54e8

VICTORY!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
I don't understand why we would want to ignore paint suppression for event handling. Don't we want event handling to act on what is actually painted to the screen?
What I was suggesting in comment 5 is that if this applies to actual user interaction rather than just event synthesis, then it's the wrong fix.  Sounds like tnikkel thinks that that's the case, in which case I think this ought to get backed out.
If there is a test sending mouse clicks during paint suppression the test should probably run off a timeout from the load event, or later.
While I understand these arguments, the previous behaviour is arguably worse - instead of dispatching events to a page that can't be seen, we were dispatching events to the previous content. This doesn't sound right to me at all.
The test in question is running off of waitForFocus.

In reply to comment 10, here's why our current behavior is incorrect.  By ignoring the paint supression, we end up dispatching the event to the old page, which is no longer active.  By doing so, we might even cancel the navigation in progress I guess, if the click is dispatched to a link for example.  Also, it's not as clear cut as you think, I guess.  What if the event in question is a three finger swipe down?  The desired effect in that case would be for the page to appear scrolled all the way to down, but without this patch this would not happen.

_If_ this behavior is actually visible to users, what I think it would seem like is for the input operation to not have any results, or have the wrong results.
(In reply to Josh Matthews [:jdm] from comment #13)
> While I understand these arguments, the previous behaviour is arguably worse
> - instead of dispatching events to a page that can't be seen, we were
> dispatching events to the previous content.

But the previous content is still visible, no?  Does this patch break (in some cases) the ability to click on the right link shortly after clicking the wrong link by accident?
The event gets dispatched to what is currently visible. It is getting sent to the previous content because the previous content is still visible. The results of the event should be visible on the previous page. If the user does a three finger swipe while the previous page is still visible and it scrolls the next page I think that would be confusing.
It's implicit in my previous comment but I'll make it clear: we currently draw the previous page while the new page has its painting suppressed.
Ok, I think I agree with your belief that this should be backed out. I think waitForFocus should also be modified to wait for a paint to avoid this issue.
I don't think it needs to wait for a paint. It probably just needs to start the test in a setTimeout(0) if the load event hasn't finished firing yet.
Waiting for a paint won't work, we paint while painting is suppressed, we just paint a background color only and nothing more. To ensure that js runs after painting suppression ends it is enough to run the js at some point including or after a timeout from the load event. At the latest we unsuppress painting immediately after we return from dispatching the load event (in DocumentViewerImpl::LoadComplete).
waitForFocus already waits for focus and load events before running the callback inside a setTimeout(0). Obviously that's not enough.
Ok, so then when nsSubDocumentFrame::BuildDisplayList is called for the problematic GetFrameForPoint call what happens exactly (there is some code near the beginning that determines which document to build a display list for)?
http://hg.mozilla.org/integration/mozilla-inbound/rev/b02e49916a3e - backout.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.