544211 - [OOPP] Freeze on right-clicking windowless (wmode=opaque) flash-app (Flash 10.1 beta 2)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Matthew Cline]

Attachment: Flash app which will cause the problem

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100203 Minefield/3.7a1pre ID:20100203031016

Flash 10.1 beta 2 (10.1.51.66)

The flash app loaded by the test case attachment will cause Firefox to freeze; manually killing the child process in which the plugin runs will un-freeze it.  Removing wmode="opaque" from the parameters prevents the freeze from happening.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20100203 Minefield/3.7a1pre

This works fine for me on Windows. I guess this is with dom.ipc.plugins.enabled to true?

Comment 2

[Matthew Cline]

It happens with dom.ipc.plugins.enabled set to either true or false.  When I was checking this fact out, one time I had to right-click on it twice to get the freeze, with the first one working fine.  Did you try right-clicking it multiple times?

Comment 3

[Matthew Cline]

Oh, you have to restart your browser for changing dom.ipc.plugins.enabled to take effect.  Yes, if I turn it to false and then restart Firefox then the problem goes away.

Comment 4

[Karl Tomlinson (:karlt)]

With a build based on 1be234301318 and with x86_64 Shockwave Flash 10.0 r42 I don't get the hang, but I am missing the window for the context menu.

Comment 5

[Matthew Cline]

The hang requires Flash 10.1 beta 2 (10.1 d51/10.1.51.66).  Downgrading back to 10.0 gives the "no freeze but no menu" behaviour of the preceding comment.

Comment 7

[Jim Mathies [:jimm]]

(In reply to comment #6)
> 
> *** This bug has been marked as a duplicate of bug 538918 ***

Oops, sorry, this was linux, 538918 was win only.

Comment 8

[Karl Tomlinson (:karlt)]

Similar behavior with Flash Player 10.1 beta 3 (LNX 10,1,51,95).

Comment 9

[Karl Tomlinson (:karlt)]

The behavior now (probably since bug 544945 landed) is that the first right button click appears to do nothing - actually it starts a nested event loop, but there is no context menu.  A second button click causes a hang.  This can be reproduced even in a Flash embed with no script.

data:text/html,<embed%20width="98%"%20height="98%"%20type="application/x-shockwave-flash"%20wmode="opaque">

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: PLT calls from libflashplayer during menu creation

This trace is from Flash Player 10, LNX 10,0,42,34.

Flash Player is using gtk_menu_popup().
The timestamp for gtk_menu_popup comes from the ButtonPress event.

gtk_menu_popup will try to grab the pointer from the plugin X client.  I expect this will fail if the button has not yet been released, because the browser X client already has an the automatic pointer grab between ButtonPress and ButtonRelease.

Comment 11

[Karl Tomlinson (:karlt)]

Chromium does not show the context menu either (only tested LNX 10,0,42,34).

Opera shows the context menu.  It does an XUngrabPointer in the browser before the plugin shows the menu.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: mouse drag event testcase

Pressing the mouse button on the red embed (with a plugin that doesn't handle
the mouse event), then dragging out of the browser window, then releasing the
mouse button produces this sequence of events (simplified a little):

EMBED: mousedown
EMBED: mouseout
Window: mouseover
Window: mouseup
Window: mouseout

When the embed is changed to an empty Flash Player (no script), none of these
events are produced.  I don't know whether that is due to the preventDefault after the plugin handles the event or some other reason.

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: ungrab the pointer before passing ButtonPress events to plugins

This patch ungrabs the pointer in the same way (AFAIK) as Opera does.

Removing the grab causes the sequence of events when dragging as described in comment 12 to change to:

EMBED: mousedown
EMBED: mouseout
Window: mouseover
Window: mouseout

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to comment #12)
> When the embed is changed to an empty Flash Player (no script), none of these
> events are produced.  I don't know whether that is due to the preventDefault
> after the plugin handles the event or some other reason.

Er, that was because I forgot to make the Flash plugin windowless.
When it is windowless, it behaves like the test plugin or a div element.

Comment 15

[Karl Tomlinson (:karlt)]

In Opera and KHTML on Linux the events from a button press on a div, drag out
of the browser, and button release are:

DIV: mousedown 
DIV: mouseout 
BODY: mouseover 
BODY: mouseup
There is no mouseout.

In Firefox on Linux the behavior is the same as the embed in comment 12:

DIV: mousedown
DIV: mouseout
Window: mouseover
Window: mouseout
Window: mouseover
Window: mouseup
Window: mouseout 

Firefox on Mac is similar (though the last mouse out doesn't happen on button
release, but on subsequent pointer motion).

Safari on Mac is
DIV: mousedown 
DIV: mouseout 
BODY: mouseover 
BODY: mouseout
(no mouseup)

Firefox on Windows is sometimes

DIV: mousedown
DIV: mouseout
Window: mouseover
Window: mouseout
Window: mouseover
Window: mouseup
There is no mouseout.

but sometimes

DIV: mousedown
DIV: mouseout
Window: mouseover
Window: mouseout (on leaving the browser window)
Window: mouseup (without a mouseover)
Window: mouseover
There is no subsequent mouseout.

Comment 16

[Karl Tomlinson (:karlt)]

Smaug, how important is it that we try to continue sending the mouseup event
to the html element before the mouseout event when the mousedown event occurred on an embed?

The issue that we have is that we only get the mouseup while the browser
process has the pointer grab.  The plugin process can't really create a proper
menu window without a pointer grab, but, because it is a different X client to
browser process, it can't steal the pointer grab from the browser.

The easiest fix is to release the pointer grab in the browser if the button
press is being sent to a plugin, even though we don't know whether the plugin
will want to create a menu window.

It is going to be pretty hard to detect exactly when the plugin wants to grab
the pointer, in order to minimize the change in event behavior.  This would
require getting between the plugin process and the X server, either through
intercepting the requests by providing a proxy display or through interposing
global function symbols from libraries that are likely to be used to make the grab request.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> Smaug, how important is it that we try to continue sending the mouseup event
> to the html element before the mouseout event when the mousedown event occurred
> on an embed?
You mean <embed> like in the testcase?

If mousedown has been dispatched, mouseup should be dispatched too, IMO.
Otherwise cases where mousedown triggers JS to start following mouse
(for example move some img) would continue following mouse moves even
after the mouse has actually been released.


> 
> The issue that we have is that we only get the mouseup while the browser
> process has the pointer grab.  The plugin process can't really create a proper
> menu window without a pointer grab, but, because it is a different X client to
> browser process, it can't steal the pointer grab from the browser.
Why would browser need pointer grab if mouse isn't down?
And why would plugin need the event, if it hasn't got mouse down?

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to comment #17)
> You mean <embed> like in the testcase?

Yes.

> If mousedown has been dispatched, mouseup should be dispatched too, IMO.

OK, thanks.  I'll see if I can come up with some other ideas.

> Why would browser need pointer grab if mouse isn't down?

The situation here is that the mouse is down.

> And why would plugin need the event, if it hasn't got mouse down?

With a windowless plugin, the browser receives the X ButtonPress event, but passes a synthetic ButtonPress event to the plugin (when the mousedown is sent to the <embed>).

We need to decide whether to release the pointer grab before the plugin responds to say whether it handled the event (so JS may yet start following the mouse).

Comment 19

[Karl Tomlinson (:karlt)]

One option might be to

* UngrabPointer before passing ButtonPress events to the plugin

* Monitor the button state on subsequent mouse events so that a synthetic
  mouseup event can be generated when a mouse event with a different button
  state is received.  (nsEvent and DOM MouseEvent do not carry button state
  information but GDK and X mouse events do.)

Coming up with sensible coordinates for the mouseup event could be
difficult.  Maybe anything outside the browser window would be OK, or maybe
something based on the last LeaveNotify event, if there was such an event.

Although the mouseup wouldn't be delivered at the actual time of button release, it would be delivered at a time suitable to keep document button state consistent.

Comment 20

[Karl Tomlinson (:karlt)]

However, there are already situations where mouseup events are not received by
the document that received the mousedown.
e.g.

* A button press that produces a browser context menu on any element (if the
  button release is not received before the menu grabs the pointer and occurs
  outside the document's window).

* A document in an iframe, if the button is released outside the iframe.

It seems that a document cannot rely on receiving a mouseup event after a
mousedown.

Although I can see that getting the mouseup event on the document that
received the mousedown is useful (particularly given that DOM mouse events do
not carry information about button state), I get the impression that the DOM
mouseup event was only intended to be delivered to the element under the
pointer at the time of button release.  The fact that the document sometimes
gets the mouseup event even when the release occurred outside the document
seems to be a quirk of implementation rather than a specified behavior.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

IIRC there is a bug somewhere discussing about whether or not to capture mouseup
outside the browser window. Or the bug isn't exactly about that but close to it.
it is IIRC some bug 3XXXXX.

I'll try to find it.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

One thing to test is scroll bar handling. What happens if you click and drag the scrollbar thumb and release mouse somewhere outside the window.

See also Bug 328931 and the bugs mentioned in the comments.

Comment 23

[Karl Tomlinson (:karlt)]

(In reply to comment #20)

> * A document in an iframe, if the button is released outside the iframe.

Actually the mouseup is received in the iframe with button 1 if
preventDefault() is not called.  It is received on the element that received
the mousedown (not the document), as if the default action (selection?) is
capturing the mouse to the element.

Another case where a mouseup is not sent is when dragging selected text out of
the browser.  The mousedown is sent at the start of the drag, and on X11 no
further mouse events are sent until the pointer returns (after releasing the
drag) to the browser window.  At that point the mouseout is sent but no
mouseup.  On NT the mouseout is received at completion of the drag, but
similarly no mouseup is received.

BTW, bug 74348 proposes button state in mouse events and there is a little
discussion about whether a guaranteed mouseup from outside the document is an
alternative.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to comment #22)
> One thing to test is scroll bar handling. What happens if you click and drag
> the scrollbar thumb and release mouse somewhere outside the window.

The element with the scroll bar (not the document) gets the mouseup, and no
other elements receive mouse events, as if something is capturing the mouse in
the element.

(On NT, when the pointer is beyond a particular distance from the scrollbar,
whether in or out of the browser, the thumb jumps back to the original
position, though this is not due to any mouseout.)

The proposed fix for this bug would not affect scroll bars implemented by the browser, as it only changes behavior when the mousedown is on a (windowless) plugin.  It would only affect document-implemented scroll bars if a plugin was involved in the scroll bar implementation.

Does content JS have access to the same capture-mouse-to-element behavior that
scroll bars and selection use?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #23)
> Actually the mouseup is received in the iframe with button 1 if
> preventDefault() is not called.  It is received on the element that received
> the mousedown (not the document), as if the default action (selection?) is
> capturing the mouse to the element.
This sounds right

> 
> Another case where a mouseup is not sent is when dragging selected text out of
> the browser.  The mousedown is sent at the start of the drag, and on X11 no
> further mouse events are sent until the pointer returns (after releasing the
> drag) to the browser window.  At that point the mouseout is sent but no
> mouseup.  On NT the mouseout is received at completion of the drag, but
> similarly no mouseup is received.
Dragging is different case, and in that case web page can handle drag events.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #24)
> Does content JS have access to the same capture-mouse-to-element behavior that
> scroll bars and selection use?
setCapture(), Bug 503943

Comment 27

[Johnny Stenback (:jst)]

Given how broken and inconsistent we've been regarding handling of events in
and around plugins in webpages over the years, let alone the inconsistencies
between browsers, I seriously doubt pages depend on any particular behavior in
this particular case. Now having said that, I'm fine with us fixing this
"right" if it's feasible to do so in a reasonable amount of time, but I'd be
fine with us doing more or less whatever to prevent the freeze and dealing with
the details here at a later point.

Comment 28

[Karl Tomlinson (:karlt)]

FWIW switching tabs while the mouse button is down will cancel a capture
(because the view gets hidden), so, even with setCapture, a document can't rely
on receiving a mouseup.

Comment 29

[Karl Tomlinson (:karlt)]

Thanks for the comments and info.  I'm now not really concerned about the
document getting a mouseup.

My only real concern with this is setCapture, which relies quite heavily on
mouseup.  The loss of the pointer grab means that the button-up may not be
received to cancel the element capture.  When the plugin actually creates a
menu the deactivation of the browser window cancels the element capture.
However, if we just ungrab the pointer without the plugin creating a menu and
the mouse button is released outside the browser (without shifting focus to
another window), the element capture will persist until the next button down.

Comment 30

[Karl Tomlinson (:karlt)]

Attachment: ungrab the pointer before passing ButtonPress events to plugins v2

This is intended for mozilla-central and 1.9.2.
Like attachment 431803 [details] [diff] [review], but using GDK.

Comment 31

[Karl Tomlinson (:karlt)]

Attachment: preparation for handling synthesized button-up events for ending element pointer capture

The intention is to receive synthesized NS_MOUSE_BUTTON_UP events as proposed
in comment 19 when we find out that we missed a button up event.  These events
are not sent to the DOM because we don't know the refPoint for the event, but
they can still cancel the element pointer capture.

Comment 32

[Karl Tomlinson (:karlt)]

Attachment: provide synthesized button-up events when native button-up events are missed

This won't detect if the button has been released and pressed again outside
Gecko but should protect against common cases, and ensure that if the user
takes their fingers off everything, they'll get back to a sensible state.

Comment 33

[Karl Tomlinson (:karlt)]

Attachment 434161 [details] [diff] won't be suitable for 1.9.2.
I need to work out whether the predecessor to setCapture needs something similar.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 434159 [details] [diff] [review]
ungrab the pointer before passing ButtonPress events to plugins v2

Name XDisplay() something else? It looks like an X API to me.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #28)
> FWIW switching tabs while the mouse button is down will cancel a capture
> (because the view gets hidden), so, even with setCapture, a document can't rely
> on receiving a mouseup.
That is actually a bug, I think.
setCapture() may very well need some tweaking still.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #29)
> Thanks for the comments and info.  I'm now not really concerned about the
> document getting a mouseup.
Yeah, I'm not really concerned either. I just feel that if possible we should
do the right thing.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 434162 [details] [diff] [review]
provide synthesized button-up events when native button-up events are missed

>+nsWindow::CheckButtonState(GdkEventCrossing *aGdkEvent)
Could the method name somehow indicate that this actually may dispatch an event.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

So does the widget level patch change Bug 328931?

And what happens when (using mouse events) one implements page level drag
and mouse gets over a plugin?

Comment 39

[Karl Tomlinson (:karlt)]

(In reply to comment #35)
> > FWIW switching tabs while the mouse button is down will cancel a capture
> > (because the view gets hidden), so, even with setCapture, a document can't rely
> > on receiving a mouseup.
> That is actually a bug, I think.

I'm not clear on what would be a better behavior there.  I expect that dispatching a mouseup to a page that the user cannot see might produce some unexpected results.

It seems that some sort of cancel event may be needed.
Perhaps the element should not receive any mouseout events until the capture is cancelled, at which point the element will get mouseout.

(In reply to comment #36)
> Yeah, I'm not really concerned either. I just feel that if possible we should
> do the right thing.

OK.  I understand the appeal of paired button events, and it would be nice to avoid having this additional situation where we don't get them.
This change feels the least scary of the options that I can think of for 1.9.2.

We can continue chasing the ideal of paired button events in the future, but I fear there may always be cases where dispatching the mouseup does not make sense.

(In reply to comment #37)
> >+nsWindow::CheckButtonState(GdkEventCrossing *aGdkEvent)
> Could the method name somehow indicate that this actually may dispatch an
> event.

CheckDispatchMissingButtonUp maybe.

(In reply to comment #38)
> So does the widget level patch change Bug 328931?

No.  The synthesized button-up events don't make it to content JS, so there is no change from the widget level patch visible.  (The testcases for bug 328931 are no longer available but bug 328931 comment 2 suggests that the behavior is now quite different to that reported.)

> And what happens when (using mouse events) one implements page level drag
> and mouse gets over a plugin?

The patch here modifies the behavior of drags implemented using simple DOM mouse events only if the mousedown happens over a windowless plugin and is released over a foreign window (including windowed plugins).

Comment 40

[Karl Tomlinson (:karlt)]

If redesigning the windowless plugin event model is an option, then (in the future) we may wish to consider sending events to plugins as a default action.  i.e. only after full DOM event processing has happened and has not done a preventDefault().

That way the ungrab hack here and anything the plugin does wouldn't affect DOM event processing, assuming content JS that wanted predictable behavior would call preventDefault().

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

"CheckDispatchMissingButtonUp maybe."

Maybe just DispatchMissingButtonUp.
If there isn't anything really missing, that name indicates that no event is
dispatched.

Comment 42

[Karl Tomlinson (:karlt)]

With XDisplay -> GetXDisplay
and CheckButtonState -> DispatchMissedButtonReleases:

http://hg.mozilla.org/mozilla-central/rev/3ab9a0d6658e
http://hg.mozilla.org/mozilla-central/rev/ecc4ba9284ed
http://hg.mozilla.org/mozilla-central/rev/e9ff9b2212e5

Comment 43

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/8aff18a5620d
http://hg.mozilla.org/projects/firefox-lorentz/rev/a4690293e873
http://hg.mozilla.org/projects/firefox-lorentz/rev/a7cded8f5925

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 45

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430

Comment 46

[Al Billings [:abillings - ex-MoCo]]

Juan or Marcia, since you've been testing Lorentz with Flash 10.1 beta 2, can you verify that this is fixed on the nightly 1.9.2 builds?