Last Comment Bug 298961 - Plugins get no notification through NPP_SetWindow when switching between tabs
: Plugins get no notification through NPP_SetWindow when switching between tabs
Status: RESOLVED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal with 3 votes (vote)
: ---
Assigned To: Simon Fraser
:
Mentors:
Depends on: 277067
Blocks: 309691
  Show dependency treegraph
 
Reported: 2005-06-27 18:07 PDT by Tinic Uro
Modified: 2005-10-23 14:14 PDT (History)
16 users (show)
mconnor: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (6.19 KB, patch)
2005-09-27 10:30 PDT, Simon Fraser
jaas: review+
jst: superreview+
mscott: approval1.8b5+
Details | Diff | Review

Description Tinic Uro 2005-06-27 18:07:35 PDT
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) 2005-08-10 22:01:21 PDT
over to josh or jst?
Comment 2 Simon Fraser 2005-08-18 11:10:11 PDT
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 2005-09-08 23:13:57 PDT
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 2005-09-13 09:45:51 PDT
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 2005-09-13 10:18:05 PDT
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 2005-09-13 11:20:59 PDT
(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 2005-09-13 11:33:48 PDT
Tinic: could you explain why what we provide now (SetWindow() with empty
clipRect) is insufficient for your needs?
Comment 8 Simon Fraser 2005-09-14 13:24:15 PDT
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 2005-09-22 22:40:31 PDT
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 2005-09-23 08:56:44 PDT
Agreed. This is the kind of API change that has to get in earlier, rather than
later. Requesting reconsideration.
Comment 11 Simon Fraser 2005-09-23 13:23:05 PDT
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] 2005-09-26 18:45:34 PDT
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 2005-09-27 10:30:56 PDT
Created attachment 197583 [details] [diff] [review]
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 2005-09-27 13:18:31 PDT
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 2005-09-27 13:22:13 PDT
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 2005-09-27 16:27:06 PDT
The changes in the patch are all in #ifdef (XP_MACOSX) sections.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-28 15:05:26 PDT
Comment on attachment 197583 [details] [diff] [review]
Patch

sr=jst
Comment 18 Scott MacGregor 2005-09-28 15:30:03 PDT
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 2005-09-28 15:41:09 PDT
landed on trunk
Comment 20 Paul Betlem 2005-09-28 15:53:41 PDT
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 2005-09-30 09:37:21 PDT
We wanted QA to give plugins a spin in the trunk builds.
Comment 22 Simon Fraser 2005-09-30 09:42:00 PDT
Be sure to test Java, QuickTime, RealPlayer (e.g. weather.com) and Flash with
this change.
Comment 23 Marcia Knous [:marcia - use ni] 2005-09-30 13:25:00 PDT
Simon: Can we get a test case to use to verify this?  
Comment 24 Simon Fraser 2005-09-30 13:40:39 PDT
(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 - use ni] 2005-09-30 16:33:32 PDT
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 2005-09-30 16:54:59 PDT
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 2005-09-30 17:04:19 PDT
landed on branch
Comment 28 Steven Michaud [:smichaud] (Retired) 2005-10-21 14:43:48 PDT
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 2005-10-21 15:51:54 PDT
(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) 2005-10-23 14:11:32 PDT
>> 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) 2005-10-23 14:14:13 PDT
Sorry, I didn't mean to reopen this bug.

Note You need to log in before you can comment on or make changes to this bug.