420527 - When Quicktime (another plugin?) is loaded in one tab, Flash content doesn't respond very readily to clicks

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Summary: When Quicktime (another plugins) is loaded in one tab, Flash content doesn't respond very readily to clicks

I'm using r115 of Flash; I first started noticing this (I think!) about a week ago, and just now figured out how to reproduce.

(Horrible summary; please tweak once we get a better picture of this bug).

Steps to Reproduce:

1. In one tab, load http://www.speedtest.net
2. In another tab, load http://fredrik.hubbe.net/plugger/test.mpg
3. Now, switch back to the speedtest.net tab
4. Try to click on a city

Actual Results:

Notice that it "takes" the click event but doesn't really do anything with it until the third or so click (or maybe it's just timing?)

It helps if you invoke Flash's context-menu; once you do that, a click seems to more readily be accepted on the Flash content

Expected Results:

Every click/mousedown event should be accepted on the first try

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The thing is, what I noticed while running a test for this bug with the above STR, all the events seem to be redirected to the hidden quicktime plugin. In detail you can see this after a speedtest has finished and you can rate your ISP. Without the quicktime tab you can move over the stars and they will be turned orange automatically. This doesn't happen if you have open an instance of Quicktime in another tab.

Comment 2

[Matthew Gregan [:kinetik]]

It looks like this is related to the CoreGraphics plugin drawing model changes from bug #344427.  If I load the same Flash-using (so no QuickTime is involved) site in two tabs, letting one run in CoreGraphics mode and forcing the other to run in QuickDraw mode, I see the behaviour described in the opening comment.  If I load the site in two tabs such that they're both in either CG or QD mode, they behave normally.  This is an artificial setup--we're not going to see Flash using both modes at the same time, but I wanted to try to eliminate QuickTime to make it easier to narrow the cause down.

To test this, I set a breakpoint in ns4xPlugin.cpp:_getvalue where the plugin requests the plugin drawing model and loaded the nike.com site in two tabs, once with the Flash in CoreGraphics mode and once in QuickDraw mode (by forcing the request for CG to return fails at the breakpoint).

Breakpoint set here:
http://mxr.mozilla.org/firefox/source/modules/plugin/base/src/ns4xPlugin.cpp#2159

In this situation, the tab with nike.com loaded in CoreGraphics mode is unresponsive to events in the same manner as described in the opening comment--it requires multiple quick clicks before the plugin starts accepting events, after which it seems to work okay until you switch to and from the tab with the site in QuickDraw mode.

I tried an old nightly (2007-03-31-04) from a little after the original CG drawing support landed, and I can reproduce the same behavior with mixed Flash/QuickTime plugins using that, too.

I don't have time to dig into this further right now.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

See also bug 422746, which has a precise regression range.

Comment 5

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

Josh, for this to block it needs a priority.

Comment 6

[Damon Sicore (:damons)]

P2.

Comment 7

[Damon Sicore (:damons)]

Josh, this has event and focus issues.  Do we need to make this a P1?

Comment 8

[Johnny Stenback (:jst)]

Is this Mac specific? The duped bug apparently is. Josh, I'm giving this to you, but feel free to reassign if you know a better owner.

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: flash content for testcase

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: quicktime content for testcase

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: testcase

I've been trying to debug this, but haven't made much progress so far.  The testcase loads some Flash content that paints sparkles following the pointer on mouse moves and some test QuickTime content sufficient to load the plugin.  The expected result is that the Flash content (running in CoreGraphics mode) should react to mouse move events when the mouse is moving over it.  What actually happens is that it only works when a QuickDraw plugin is not also active (in the same page or tab, if it's in another window everything works as expected).

The testcase starts with both pieces of plugin content hidden.

1.  Reveal the flash, mouse move works as expected
2.  Reveal the qt, mouse move in flash content no longer responds
3.  Hide qt, mouse move does not respond
4.  Hide flash, reveal flash, mouse move does not respond

To get the flash content to respond again, two things work:

a. Hide flash and qt, then reveal and hide qt, then reveal flash
b. Reload page and reveal flash

As far as I can tell, the expected events are being delivered to the plugin in all cases, it just seems to fail to react to them when there are both CoreGraphics and QuickDraw plugins active (or have been active recently without some state being reset).

Note that it's not necessary to use QuickTime to provoke this bug, I also see it if I use two pieces of Flash content and break in a debugger to force one to use the old QuickDraw painting model, but that doesn't make for a very practical test case.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Also visible with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008031904 Minefield/3.0b5pre

Matthew, to your mentioned points a and b I will add c as another workaround:

c. Move the mouse while holding down the left mouse button

This will also fire enough events so the Flash content responses to the send events?

Comment 13

[Matthew Gregan [:kinetik]]

D'oh, yes, I forgot to mention that behaviour.  Thanks for mentioning it.  The extra events we send are button down and button up (and perhaps got and lost focus, but if the plugin was already focused we don't send those again, and the behaviour is the same).

Also, when the plugin is in this state (not responding to mouse move), other events still work normally, e.g. tabbing between focusable items inside the flash (unfortunately the content I ended up using for the testcase doesn't allow you to test this much, I really need to find or make some flash that shows useful debug info on incoming events).

Did you change the plaform back to All on purpose, or was there a mid-air on the bug flags?  As far as I know, this bug is OS X only.

Comment 14

[Matthew Gregan [:kinetik]]

(In reply to comment #13)
> Did you change the plaform back to All on purpose, or was there a mid-air on
> the bug flags?  As far as I know, this bug is OS X only.


Never mind, it was the "hardware" field, and I guess "Macintosh" might be ambiguous in the world of Intel Macs.

Comment 15

[Matthew Gregan [:kinetik]]

Not making much progress with this, but a few more notes.

It seems that the mouse move events we pass into the plugin via the plugin API are not the ones that Flash is using for its own internal mouse move handling--if I add a conditional that drops nsPluginEventType_AdjustCursorEvent events on the floor before they're dispatched to the plugin, mouse motion behaves the same way in the test case (works normally until the QT content is loaded, then stops responding).  If anyone more familiar with OS X could point be to the APIs flash might be using to register for/track mouse movement events, that'd be very useful.

While testing, I noticed that it's possible to get the plugin to resume responding to mouse move events by cycling focus.  For example, in the testcase, reveal the flash then the QT, hide the QT (mouse move is still ignored at this point), then cycle focus by pressing tab.  Once the main content area receives focus (you can see the dotted focus outline appear), mouse movement in the flash content begins working again.  Changing between browser tabs or switching to another application and back has the same effect.  Note that some other types of focus change do not cause mouse movement to being working again, e.g. clicking directly on the flash content results in an NS_GOTFOCUS event being sent to the plugin widget, but it does not begin responding to mouse movement after this.  

Dropping focus events immediately in ChildView::sendFocusEvent doesn't make any difference to the situation where mouse movement starts working again when cycling focus, so whatever change is occurring that is causing flash to start responding has happened before we reach this method.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

> if I add a conditional that drops
> nsPluginEventType_AdjustCursorEvent events on the floor before
> they're dispatched to the plugin, mouse motion behaves the same way
> in the test case (works normally until the QT content is loaded,
> then stops responding)

If I remember right, the Flash plugin actually calculates mouse motion
on idle events (send in nsObjectFrame::Notify()).  Michelle can tell
me if I'm wrong, though.

Michelle, do you have any insights into this problem?

Comment 17

[Steven Michaud [:smichaud] (Retired)]

It seems nobody's done a regression range for this bug.

But from what you say in comment #2, Matthew, the trouble could have
started with the patch for bug 344427 (which started supporting
CoreGraphics drawing in plugins).

Comment 18

[Michelle Sintov]

(In reply to comment #16)
> If I remember right, the Flash plugin actually calculates mouse motion
> on idle events (send in nsObjectFrame::Notify()).  Michelle can tell
> me if I'm wrong, though.

Steven is correct; the Flash Player calculates mouse moves on nullEvent.

Comment 19

[Matthew Gregan [:kinetik]]

(In reply to comment #17)
> It seems nobody's done a regression range for this bug.
> 
> But from what you say in comment #2, Matthew, the trouble could have
> started with the patch for bug 344427 (which started supporting
> CoreGraphics drawing in plugins).

Right, the only known way to reproduce the problem is using plugins with mixed drawing models (CoreGraphics and QuickDraw).  Since support for the CoreGraphics drawing model is fairly recent (added in bug #344427), it's difficult to do a regression search past the patch where that functionality was added.  It's probably possible to backport the CoreGraphics patch to extend the tested regression range, but that would be a fairly slow and painful process.

Thanks for the tip about mouse movement being passed in via idle events, it seems obvious in retrospect.  Thanks to an earlier tip from Josh, I've already checked that we continue to deliver idle events to the appropriate plugin(s) when the Flash content in the testcase exhibits the unresponsive mouse movement behaviour.  I've just double checked and the coordinates for the mouse location look correct (in fact, the idleEvent we pass in to the plugin looks the same in the working and failing cases).

Steven, are you actively taking this bug? I see you've reassigned it to yourself.  I was working on it, but I'm not making fast progress, and I bet someone like you who knows the Mac code very well can find the problem quicker.  I'm happy to keep poking at it if you're busy with other bugs, though.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

> Steven, are you actively taking this bug?

Yes ... once I'm done with bug 357670, which is taking longer than I
thought (i.e. probably in the next 2-3 days).

I'm not sure I'll be able to fix it either, but a second pair of eyes
sometimes helps.  And I _have_ had a lot of experience (with the Java
Embedding Plugin) solving nasty plugin problems :-)

Comment 21

[Matthew Gregan [:kinetik]]

Using my testcase, I noticed that the visible region of our window changes (by calling DebugPrintWindow at opportune times) after the QuickTime plugin loads.  Digging a bit further, I found that Flash calls GetPortVisibleRegion, then PtInRgn.  When the Flash content is responding to mouse moves, PtInRgn is returning true, when it is not responding, PtInRgn is returning false.

Looking at the arguments to PtInRgn and the result, I noticed the following:

After loading the testcase and revealing the Flash content, you see PtInRgn calls like so while moving the mouse in the bottom left of the Flash area:

PtInRgn([471, 8], [0, 0, 1012, 1016]) -> 1

where the values are ([point], [bounds of region]) -> result.

After revealing the QT content, moving the mouse in approximately the same area results like so:

PtInRgn([-21, 0], [0, 0, 355, 800]) -> 0

After hiding the QT content and using tab to cycle focus until mouse moves begin working again, the result of the calls look like:

PtInRgn([-22, 2], [-492, -8, 520, 1008]) -> 1

By forcing PtInRgn to always return true, the Flash content works as expected in all cases (that is, it responds to mouse movement at all times, with the QT content loaded or hidden).

Watching the callers of SetPortVisibleRegion, the problem becomes rather obvious.  When painting a plugin, we call nsChildView::StartDrawPlugin just before asking the plugin to draw.  When the plugin is using the QD drawing model we change a bunch of state on the QD port before telling the plugin to paint.

The problem is that we don't bother restoring the state in nsChildView::EndDrawPlugin, presumably because it (used to) only matter for drawing via QD, and we always set up the appropriate state before asking a QD plugin to draw.  Once the CG drawing model is introduced, we end up with interleaved paint calls for the CG and QD plugins, which means StartDrawPlugin is leaving the QD port in a bad state.  Since Flash is using QD internally even when in CG mode, it ends up being affected by the state we last set on the QD port when drawing the QD model plugin.

I have to run now, but I'll post a patch later this evening that restores the appropriate state in EndDrawPlugin.

Comment 22

[Matthew Gregan [:kinetik]]

Taking bug since I've almost cracked it.

Comment 23

[Johnny Stenback (:jst)]

I wonder if bug 420527 might be due to the same problem?

Comment 24

[Matthew Gregan [:kinetik]]

Wrong bug number, did you mean bug 425715?

Comment 25

[Matthew Gregan [:kinetik]]

(In reply to comment #21)
> I have to run now, but I'll post a patch later this evening that restores the
> appropriate state in EndDrawPlugin.

This evening's patch doesn't fix all of the cases, so I won't attach it.  My testcase works correctly in some cases with it, but not others (depending on the order the plugin content is revealed), and the case where the QD plugin is in another tab doesn't work properly either.  I must be missing something, I'll dig further tomorrow.

Comment 26

[Johnny Stenback (:jst)]

(In reply to comment #24)
> Wrong bug number, did you mean bug 425715?

Yes, that's exactly what I meant, sorry.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

> Once the CG drawing model is introduced, we end up with interleaved
> paint calls for the CG and QD plugins, which means StartDrawPlugin
> is leaving the QD port in a bad state.

Good catch, Matthew!

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Though you'd think this shouldn't happen ... and therefore is an Apple bug.

Comment 29

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Alter nsChildView::StartDrawPlugin so that it sets the visibility of the plugin port (or just our window's port for CG plugins) appropriately.  It is necessary to do this for CoreGraphics plugins because they may use Carbon internally.

What happens in this case is that Flash calls GetWindowPort on the WindowRef we pass in via the plugin port, then uses the resulting port to query the visible region and perform hit tests (via GetVisibleRegion and then PtInRgn).  When a CG and QD mode plugin are painted in the same top level window, they end up sharing the underlying QD port.

When the QD plugin is painted, ::StartDrawPlugin would set the port up with visibility appropriate for that plugin only.  We skipped this setup for CG plugins because it didn't seem necessary.  This left the QD port set up incorrectly when painting the CG plugin.  Because Flash still uses the QD port internally, we would end up painting Flash in CG mode, the visible region of the QD port could be set up incorrectly and would cause mouse move hit tests to fail (thinking the move happened in an invisible plugin region).

Note that this fix does not solve the bug Johnny asked about (bug #425715).  While trying to debug this bug, I did notice that WebKit's plugin stuff has a call to an undocumented API (CallDrawingNotification via the tellQuickTimeToChill method on their plugin view class) which is used when moving or hiding QD mode plugins.  Not sure if this is related to the problem we're seeing in bug #425715, but thought I'd mention it in case it helps someone track that bug down.

Comment 30

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

Comment on attachment 313526 [details] [diff] [review]
patch v1

Wow, what a crazy bug. Congratulations ... I guess :-)

+#if 0
   // If this is a CoreGraphics plugin, nothing to do but prevent being
   // reentered.
   if (mPluginIsCG) {
     mPluginDrawing = PR_TRUE;
     return NS_OK;
   }
+#endif

Why not just delete this code?

Comment 31

[Matthew Gregan [:kinetik]]

Attachment: patch v2

No reason, just left in accidentally, thanks for pointing it out.  Updated patch deletes that chunk of code.

Comment 32

[Josh Aas]

Comment on attachment 313907 [details] [diff] [review]
patch v2

Nice work!

Comment 33

[Josh Aas]

landed on trunk, thanks Matthew!

Comment 34

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008040904 Minefield/3.0pre

Matthew: thanks for all your hard work!