298961 - Plugins get no notification through NPP_SetWindow when switching between tabs

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Tinic Uro]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

Generally I would expect that plugins are always aware of their window position
and state. This does not seem to be case when a new tab is created and switched to. 



Reproducible: Always

Steps to Reproduce:
1. Open FireFox
2. Go to www.macromedia.com which has two .SWF files on the page
3. Create a new tab and switch to it

Actual Results:  
FireFox will not notify the Flash plugin about the fact that the user just
switched to a new tab.


Expected Results:  
Ideally NPP_SetWindow should be called and the CGrafPtr pointer in NP_Port::port
should become NULL.

Why is this important? Macromedia will be transitioning the Flash Player to use
OpenGL for final blitting in the future, achieving a 20-30% performance increase
for all content and tear free drawing. This will require to setup a OpenGL
drawing region using the AGL/NGL frameworks. If we do not know when a switch to
a tab happens we can not hide the OpenGL region and it will remain in place.

Safari handles this correctly, we get notification through NPP_SetWindow and are
able to hide the OpenGL region every time tabs are switched.

You can contact me in case you need a test plugin binary.

Comment 1

[Rafael Ebron (:rebron)]

over to josh or jst?

Comment 2

[Simon Fraser [no longer active]]

I'm pretty sure we do call SetWindow when switching tabs (maybe not quite at the
right time, see bug 277067). For a tab that's being hidden, the cliprect will be
set to an empty rect (although I think the port is still non-null).

Comment 3

[Simon Fraser [no longer active]]

I verified that both Firefox and Camino notify plugins of tab switches by
calling SetWindow(). For a plugin in a hidden tab, the port is not null, but the
clipRect is empty (height and width are zero). I understand that this does not
make it possible for the plugin to tell if it is simply scrolled offscreen in
the current tab, or in a background tab.

I'm loath to change it to pass a null port, because I have no idea which other
plugins would be adversely affected by this change. We should float this on the
macplugin-dev mailing list.

Comment 4

[Josh Aas]

This can be taken of the 1.5b5 blocker list I think. Apparently Macromedia
solved their problem because Flash 8.0.22 does't exhibit it.

Comment 5

[Josh Aas]

http://www.kaourantin.net/2005/07/why-does-flash-player-8-on-mac-perform.html

This indicates that 8.0.22 behavior might not be optimal because of this bug.
Tinic - did you work around this bug successfully in 8.0.22?

Comment 6

[Tinic Uro]

(In reply to comment #5)
> http://www.kaourantin.net/2005/07/why-does-flash-player-8-on-mac-perform.html
> 
> This indicates that 8.0.22 behavior might not be optimal because of this bug.
> Tinic - did you work around this bug successfully in 8.0.22?

No we did not. I would not have been wise to break Flash in older FireFox versions ;-) 8.0.22 which we 
just released to the public disables OpenGL support for FireFox, we have a specific check in there. 

We check the user agent string for the occurence of "AppleWebKit" to see if the browser is Safari. So if 
you change the user agent string to include AppleWebKit somewhere OpenGL should be turned on.

I also sent a custom build to Josh which always enables OpenGL. I can also rebuild 8.0.22 with OpenGL 
enabled if you need it.

Comment 7

[Simon Fraser [no longer active]]

Tinic: could you explain why what we provide now (SetWindow() with empty
clipRect) is insufficient for your needs?

Comment 8

[Simon Fraser [no longer active]]

I think we might want to expand the plugin API to add tab switching calls, and
then add code to call them appropriately (see bug 277067). It would be nice to
do this for 1.5.

Comment 9

[Emmy]

Noticed the change in state for this bug, and would like to comment on why 
Macromedia feels the bug is essential.

Flash rendering performance on Firefox is currently much slower than Safari. 
The degraded performance is particularly perceptible when viewing video and 
graphics intensive media. The Flash Player team can’t enable the rendering 
optimizations for Firefox until the API is in place. If the API change is 
pushed beyond the 1.5 release, this will result in us having to live without 
the benefit of some really compelling optimizations for quite a while due to 
our release cycle.

Comment 10

[Simon Fraser [no longer active]]

Agreed. This is the kind of API change that has to get in earlier, rather than
later. Requesting reconsideration.

Comment 11

[Simon Fraser [no longer active]]

I've retested this, and found that, indeed, we don't call SetWindow() on the
plugin when a tab is being hidden. This should block 1.5b5, since it's known to
cause issues with Flash, and RealPlayer. The resulting bugs (leaving stray
frames on the new tab) have previously been construed as security issues.

Comment 12

[Mike Connor [:mconnor]]

Need to get traction on this ASAP, plussing for now.  Josh/Simon, this needs to
happen this week or we'll drop it again.

Comment 13

[Simon Fraser [no longer active]]

Attachment: Patch

This patch tidies up some code in nsObjectFrame to ensure that we call
SetWindow() on the plugin when the clip rect changes. This causes SetWindow()
to get called for plugins in a tab which is being hidden. However, it does not
fix the _timing_ of that call (it may still be too late, see bug 277067).

I also removed some XP_MAC #ifdefs.

Comment 14

[Josh Aas]

Comment on attachment 197583 [details] [diff] [review]
Patch

Tested with a version of the Flash plugin that has OpenGL enabled for Gecko
browsers. This patch stops the plugin from drawing over tabs it shouldn't.

Don't we have some utility function for comparing rects without testing every
field? Fix that if you can, but otherwise this looks fine.

Comment 15

[Josh Aas]

BTW - I only tested on Mac OS X 10.4.2. Somebody should take this patch for a
spin on Windows, testing various plugins.

Comment 16

[Simon Fraser [no longer active]]

The changes in the patch are all in #ifdef (XP_MACOSX) sections.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 197583 [details] [diff] [review]
Patch

sr=jst

Comment 18

[Scott MacGregor]

Comment on attachment 197583 [details] [diff] [review]
Patch

Josh is going to land this on the trunk so we can get some trunk baking /
plugin testing going on there before this goes into the branch.

Comment 19

[Josh Aas]

landed on trunk

Comment 20

[Paul Betlem]

Excellent. My sincere thanks to the dev, qa, and code reviewers for making this
happen so quickly. The Flash Player team will run some tests early next week and
follow-up if we encounter any issues.

Comment 21

[Scott MacGregor]

We wanted QA to give plugins a spin in the trunk builds.

Comment 22

[Simon Fraser [no longer active]]

Be sure to test Java, QuickTime, RealPlayer (e.g. weather.com) and Flash with
this change.

Comment 23

[Marcia Knous [:marcia]]

Simon: Can we get a test case to use to verify this?  

Comment 24

[Simon Fraser [no longer active]]

(In reply to comment #23)
> Simon: Can we get a test case to use to verify this?  

Josh should be able to help you out here.

Comment 25

[Marcia Knous [:marcia]]

Tested using today's trunk build - Mozilla/5.0 (Macintosh; U; PPC Mac OS X
Mach-O; en-US; rv:1.9a1) Gecko/20050930 Firefox/1.6a1 using a version of the
Flash plugin that has OpenGL enabled for Gecko browsers. Did not see any
bleedthrough - I tested a number of sites including weather.com, ran numerous
Quicktime trailers, a few of the flash top animation sites including
http://www.ikea.com/ms/sv_SE/kampanj/fy06_dromkok/dromkok.html.

I did see an issue with scrolling on the Macromedia site (when you scroll down
you get flickering in the flash part of the site. If Macromedia doesn't address
the bug on their side then we should get one on file. 

Comment 26

[Scott MacGregor]

Comment on attachment 197583 [details] [diff] [review]
Patch

approving for the branch now that we've gotten our QA feedback from this.

Comment 27

[Josh Aas]

landed on branch

Comment 28

[Steven Michaud [:smichaud] (Retired)]

As a result (I think) of this change, nsIPluginInstance::SetWindow() is now
_never_ called with pluginWindow == NULL.  That used to be a signal that the
plugin should be made invisible.

Also, pluginWindow->window->port (i.e. the GrafPort) is never NULL ... though
there was some discussion (I think) of using this as a signal that the user
had switched away from a tab.

Am I right?  And if so, was this deliberate?

(By the way, both Firefox and Camino now seem to behave this way.)

Comment 29

[Simon Fraser [no longer active]]

(In reply to comment #28)
> As a result (I think) of this change, nsIPluginInstance::SetWindow() is now
> _never_ called with pluginWindow == NULL.  That used to be a signal that the
> plugin should be made invisible.

If so, that wasn't deliberate. The signal that the plugin should be made
invisible is that the clipRect is empty.

> Also, pluginWindow->window->port (i.e. the GrafPort) is never NULL ... though
> there was some discussion (I think) of using this as a signal that the user
> had switched away from a tab.

That was discussed, but never implemented.

> Am I right?  And if so, was this deliberate?

What the patch in this bug did was to ensure that we always call SetWindow()
when the clipRect changes.

> (By the way, both Firefox and Camino now seem to behave this way.)

Yes, they should behave the same way now (particularly that bug 312563 was
checked in for Camino).

Comment 30

[Steven Michaud [:smichaud] (Retired)]

>> As a result (I think) of this change, nsIPluginInstance::SetWindow() is now
>> _never_ called with pluginWindow == NULL.  That used to be a signal that
>> the plugin should be made invisible.
>
>If so, that wasn't deliberate. The signal that the plugin should be made
>invisible is that the clipRect is empty.

The MRJ Plugin has been using pluginWindow == NULL in this way for years
(since well before I started playing with it).  So, whether or not this was
legitimate, I'll bet other plugins are doing the same thing.

Also, as far as I can tell, stopping the use of pluginWindow == NULL on tab
switching wasn't required to fix this bug (298961).  (Tell me if I'm wrong,
though.)

Whatever the rights or wrongs of this particular issue, it's now moot for the
Java Embedding Plugin -- I spent a couple of extra days banging my head
against this, and found workarounds for all the issues I could identify.

It helped that I long ago decided to treat the zero-clipping and pluginWindow
== NULL signals identically -- and interpret both as signals that the plugin
should (in effect) be made invisible.  Your change showed up places where I
hadn't quite achieved this ... but 20-odd hours of head-banging was enough to
set things right.

Also, the problems were subtle enough that I didn't notice them right away,
even in Firefox 1.5 Beta 2 (which contained the fix for bug 298961).

I think it's too bad, though, that you made a pretty fundamental change in the
de-facto plugin interface without warning anyone about it ... at least outside
the circle of those discussing this particular bug.

(Side note:  When you scroll the browser window, the plugin window switches
back and forth several times a second between being zero-clipped and being
"normally" clipped.  Calling SetWndow() with pluginWindow == NULL only
happened on switching tabs or creating a new tab (actually it didn't always
happen even then, just most of the time).  Because the pluginWindow == NULL
signal was used only when it was actually needed, when I received it I used a
relatively expensive way of making the plugin invisible -- something far too
expensive to use several times a second (I actually tried it yesterday).  It
turns out I don't really need this "expensive" set-invisible method in Carbon
browsers.  But I _did_ need it in Cocoa browsers ... at least until I found a
workaround.  Without the "expensive" set-invisible method or some kind of
workaround, parts of applets with dynamic displays (like the NOAA weather
applets) would leak out from a background tab into the foreground tab.  For
those who are curious, my workaround (the one that made it unnecessary for me
to use the "expensive" set-invisible method even in Cocoa browsers) is
contained in the MyNSView _propagateDirtyRectsToOpaqueAncestors method in
Controller.m.)

Comment 31

[Steven Michaud [:smichaud] (Retired)]

Sorry, I didn't mean to reopen this bug.