(This problem is related to bug 162134. That bug is mostly resolved by the Java Embedding Plugin (http://javaplugin.sourceforge.net/), but some issues remain with Mozilla (1.7.3, 1.7.5, 1.8a and the most recent nightlies). The problem reported here seems to be the cause of those issues. I'm the Java Embedding Plugin's author.) On OS X, Mozilla (all recent versions, including the nightlies) waits too long to make an applet invisible when switching away from a tab that contains an applet. The result is that, if an applet is frequently redrawn, parts of it will appear in the "wrong" tab (not the tab containing the applet). This occurs even when using recent versions of the Java Embedding Plugin, and despite the fact that using the Java Embedding Plugin seems to completely resolve bug 162134 for recent versions of Firefox and Camino. The problem described here occurs whether or not the "second" tab also contains an applet. But I'm not (yet) able to demonstrate it except for the case where both the "first" and "second" tabs contain applets. Even in that case, you must use version 0.8.9 or later of the Java Embedding Plugin -- it can be made to log the relevant debugging information. In subsequent messages I'll "attach" logs generated in recent versions of Mozilla, Firefox and Camino. I'll annotate the logs to show how Firefox and Camino get things right, but Mozilla gets them wrong. To make the Java Embedding Plugin (0.8.9 or later) log the relevant information, use the Java Control Panel ("Java 1.4.X Plugin Settings") to add the following to your "Runtime Parameters": "-Djep.debug.visibility=true" If you check the "Use Java console" box, the debugging information will appear in the Java Console. Otherwise it will be written to the "~/Library/Logs/Java Console.log" file.

There are two ways an applet can be made invisible -- using showHideApplet to make it invisible or clipping it to a width or height of zero. Whichever of these commands is used first on a visible applet will make it invisible. Likewise there are two ways an applet can be made visible -- using showHideApplet to make it visible or clipping it to a non-zero width and height. The lines starting and ending with four asterisks ("****") are annotations that I've added. The procedure used to generate this log was as follows: 1. Make sure the latest JEP version (currently 0.8.9) is installed. 2. Start Mozilla 1.7.5. 3. Visit http://java.sun.com/applets/jdk/1.4/demo/applets/Animator/example4.html. 4. Open a new tab, and in it visit http://www.fantasyasylum.com/. 5. Switch to the first tab (containing the Animator applet) -- debris from the NewsTicker applet (from the second tab) should appear. Note that, in the log, the Animator applet is made visible (clipped to a non-zero width and height) before the NewsTicker applet is made invisible (using showHideApplet). 6. Switch to the second tab (containing the NewsTicker applet) -- debris from the Animator applet should appear. Note that, in the log, the NewsTicker applet is made visible (using showHideApplet) before the Animator applet is made invisible (using showHideApplet). 7. Switch to the first tab (containing the Animator applet) -- debris from the NewsTicker applet should appear. Note that, in the log, the Animator applet is made visible (using showHideApplet) before the NewsTicker applet is made invisible (using showHideApplet). 8. Switch to the second tab (containing the NewsTicker applet) -- debris from the Animator applet should appear. Note that the log for this step is the same as for step 6. 9. Quit Mozilla 1.7.5.

See comment #2 for more information on what's in these logs. This log was generated as follows: 1. Make sure the latest JEP version (currently 0.8.9) is installed and "-Djep.debug.visibility=true" is included in the Java Console's "Runtime Parameters". 2. Start Camino 0.8.2. 3. Visit http://java.sun.com/applets/jdk/1.4/demo/applets/Animator/example4.html. 4. Open a new tab, and in it visit http://www.fantasyasylum.com/. 5. Switch to the first tab (containing the Animator applet) -- no debris from the NewsTicker applet (from the second tab) should appear. Note that, in the log, the Animator applet is made visible (using showHideApplet) after the NewsTicker applet is made invisible (using showHideApplet). 6. Switch to the second tab (containing the NewsTicker applet) -- no debris from the Animator applet should appear. Note that, in the log, the NewsTicker applet is made visible (using showHideApplet) after the Animator applet is made invisible (using showHideApplet). 7. Switch to the first tab (containing the Animator applet) -- no debris from the NewsTicker applet should appear. Note that the log for this step is the same as for step 5. 8. Switch to the second tab (containing the NewsTicker applet) -- no debris from the Animator applet should appear. Note that the log for this step is the same as for step 6. 9. Quit Camino 0.8.2.

See comment #2 for more information on what's in these logs. This log was generated as follows: 1. Make sure the latest JEP version (currently 0.8.9) is installed and "-Djep.debug.visibility=true" is included in the Java Console's "Runtime Parameters". 2. Start Firefox 1.0. 3. Visit http://java.sun.com/applets/jdk/1.4/demo/applets/Animator/example4.html. 4. Open a new tab, and in it visit http://www.fantasyasylum.com/. 5. Switch to the first tab (containing the Animator applet) -- no debris from the NewsTicker applet (from the second tab) should appear. Note that, in the log, the Animator applet is made visible (clipped to non-zero width and height) after the NewsTicker applet is made invisible (using showHideApplet). 6. Switch to the second tab (containing the NewsTicker applet) -- no debris from the Animator applet should appear. Note that, in the log, the NewsTicker applet is made visible (clipped to non-zero width and height) after the Animator applet is made invisible (using showHideApplet). 7. Switch to the first tab (containing the Animator applet) -- no debris from the NewsTicker applet should appear. Note that the log for this step is (basically) the same as for step 5. 8. Switch to the second tab (containing the NewsTicker applet) -- no debris from the Animator applet should appear. Note that the log for this step is the same as for step 6. 9. Quit Firefox 1.0.

This is basically identical to the log for Mozilla 1.7.5. It was made with the latest Mozilla 1.8 nightly (2005-01-05-06-trunk). I include it for the sake of completeness.

(Following up comment #0) > (This problem is related to bug 162134. That bug is mostly resolved > by the Java Embedding Plugin (http://javaplugin.sourceforge.net/), > but some issues remain with Mozilla (1.7.3, 1.7.5, 1.8a and the most > recent nightlies). The problem reported here seems to be the cause > of those issues. I'm the Java Embedding Plugin's author.) It turns out that, under special circumstances, Firefox also has this timing problem. The evidence still suggests that Camino never has it. For more information see recent comments on bug 162134, particularly the following: https://bugzilla.mozilla.org/show_bug.cgi?id=162134#c184 Whenever Firefox has this problem, my logs indicate that it has made an applet invisible at the "wrong" time: When switching between tabs (both containing applets), the applet in the "second" tab (the one being switched to) is made visible before the applet in the "first" tab (the one being switched from) is made invisible. As a result, debris from the "first" applet is displayed in the "second" applet's tab. Otherwise Firefox always makes the "first" applet invisible before it makes the "second" applet visible. This strongly suggests that the Firefox timing problem, like the Mozilla one, is the browser's fault.

See comment #2 for more information on what's in these logs. This log demonstrates what happens when you "trick" Firefox into displaying parts of an applet in the "wrong" tab. It was generated as follows: 1. Make sure the latest JEP version (currently 0.8.9) is installed and "-Djep.debug.visibility=true" is included in the Java Console's "Runtime Parameters". 2. Start Firefox 1.0. 3. Visit http://java.sun.com/applets/jdk/1.4/demo/applets/Animator/example4.html. 4. Open a new tab, and in it visit http://www.fantasyasylum.com/. 5. Click once on the text in the second tab's location bar -- "http://www.fantasyasylum.com/" will be highlighted. 6. Switch to the first tab (containing the Animator applet) -- no debris from the NewsTicker applet (from the second tab) should appear. Note that, in the log, the Animator applet is made visible (clipped to non-zero width and height) after the NewsTicker applet is made invisible (using showHideApplet). 7. Switch to the second tab (containing the NewsTicker applet). "http://www.fantasyasylum.com/" will no longer be highlighted, but the text-entry cursor will be flashing in the second tab's location bar. No debris from the Animator applet should appear. Note that, in the log, the NewsTicker applet is made visible (clipped to non-zero width and height) after the Animator applet is made invisible (using showHideApplet). 8. Switch to the first tab (containing the Animator applet) -- no debris from the NewsTicker applet should appear. Note that the log for this step is (basically) the same as for step 6. 9. Switch to the second tab (containing the NewsTicker applet). "http://www.fantasyasylum.com/" will now be highlighted, and debris from the Animator applet in the first tab should now appear in the second tab. Note that, in the log, the NewsTicker applet is made visible (using showHideApplet) before the Animator applet is made invisible (using showHideApplet). 10. Switch to the first tab (containing the Animator applet) -- no debris from the NewsTicker applet should appear. Note that the log for this step is (basically) the same as for step 6. 11. Switch to the second tab (containing the NewsTicker applet). "http://www.fantasyasylum.com/" will still be highlighted, and debris from the Animator applet in the first tab should again appear in the second tab. Note that the log for this step is the same as for step 9. 12. Quit Firefox 1.0.

Is this a problem with a recent (and here recent means "today") build?

The results and the logs are the same with Mozilla (I tested 2005-01-30-04-trunk). But, interestingly, the results are different for Firefox (I tested 2005-01-30-08-trunk). The procedure described in comment #6 now only reproduces the problem inconsistently (I'd guess about 25% as often as with Firefox 1.0). When the problem does occur, the logs still always show that the applets' visibility was changed in the "wrong" order -- i.e. the applet the tab you're switching to is made visible before the applet in the tab you're switching away from is made invisible. When the problem doesn't occur, the logs still always show that the applets' visibility was changed in the "right" order. "http://www.fantasyasylum.com/" is now highlighted in step 7 (it isn't with Firefox 1.0). So the problem isn't completely solved, but it does happen less often. Do you have any idea when the source-code change took place that led to this change in results? These tests were all done in Mac OS X 10.3.7 with Java 1.4.2 Update 2.

The procedure described at https://bugzilla.mozilla.org/show_bug.cgi?id=162134#c180 also works less often with the latest Firefox nightly (2005-01-30-08-trunk) than it does for Firefox 1.0 ... though it definitely does still work some of the time.

There have been several recent (since Firefox 1.0) checkins that could have affected this. See http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/view/src/nsViewManager.cpp (revisions 3.380, 3.378, 3.373, 3.369, 3.368, 3.364, 3.361, 3.360).

> The procedure described at > https://bugzilla.mozilla.org/show_bug.cgi?id=162134#c180 also works > less often with the latest Firefox nightly (2005-01-30-08-trunk) > than it does for Firefox 1.0 ... though it definitely does still > work some of the time. I take this back. After further tests, I now think this procedure "works" about as often in 2005-01-30-08-trunk as it does in Firefox 1.0.

> But, interestingly, the results are different for Firefox (I tested > 2005-01-30-08-trunk). The procedure described in comment #6 now > only reproduces the problem inconsistently ... After lots of mucking around with the Firefox nightlies available at http://archive.mozilla.org/pub/firefox/ and with 'cvs diff', I've tracked down the code changes that led to this change in Firefox's behavior. They're not at all when or where I expected to find them, and they're only very tangentially related to the problem at hand -- i.e. knowing about them is basically no help at all in finding a solution to this problem. But I need to warn others off this particular red herring, so here goes: The changes took place on 2004-07-14 and 2004-07-19 to a Chrome file! (Revisions 1.40 and 1.41 at http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml.) (Related changes took place to another, similar file on 2004-07-14 -- revision 1.101 at http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml. But these aren't relevant to our bug 277067.) I guess this shows how long Firefox 1.0 development took place on a separate branch, off the main trunk. The patch I'm posting here is meant to be applied to the Firefox 1.0 source tarball, and changes its behavior to match that of the most recent Firefox nightlies with regard to the procedure described in comment #6 (and referred to again in comment #8). The key change (wrt our bug) takes place at the very end of the updateCurrentBrowser() function: Previously setFocus() was usually called directly. Now it's always called via setTimeout(). A timeout of '0' is specified. But setTimeout() is presumably called asynchronously even with a zero timeout. This seems to be what makes the difference. (It isn't just the timing within the updateCurrentBrowser() function. Calling setFocus() via setTimeout() probably makes updateCurrentBrowser() return more quickly, but adding a delay before or after the call to setFocus() doesn't make bug 277067 happen more frequently.) Side note: Since setFocus() is called asynchronously, the lines that set document.commandDispatcher.suppressFocusScroll to 'true' and then to 'false' should probably be moved inside the setFocus() function.

> But setTimeout() is presumably called asynchronously even with a > zero timeout. This should be rephrased: "But setFocus() is presumably called asynchronously (via setTimeout()) even when setTimeout() is called with a zero timeout."

Steven, if that patch isn't for this bug, could you please file a separate bug on toolkit on it? Please cc me on the bug.

There's no need to file a separate bug. My "patch" isn't a fix -- it's proof of (or evidence supporting) what I was saying in my comment #12. (I suppose my "side note" might warrant a bug report, though. If you agree, I'll file a bug on that issue sometime in the next few days.)

Oh, ok. Yes, please file a bug on the side note.

> I suppose my "side note" might warrant a bug report, though. It's already been taken care of: Revision 1.42 for http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml. Revision 1.102 for http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml.

too late for 1.8b1

I might come up with a patch in bug 162134 that fixes this. Stay tuned.

Stephen: can you, from the plugin, dump out information about the QuickDraw graphics port in the various cases where the bug occurs? It might be that having text in the URL bar changes the state of the GrafPort when you're switching tabs. This might be the port origin, or clipping. Also, since the caret may be blinking in the URL bar, there's some timing-related stuff here too.

> Stephen: can you, from the plugin, dump out information about the > QuickDraw graphics port in the various cases where the bug occurs? I'll look into it. Probably the best place to do this is somewhere in the "MRJ Plugin JEP". Once I have it working, maybe I'll post it at a special "Testing" project on the JEP's SourceForge project page. (Or I suppose I could even post it here.) Please give me a specific list of information, to make sure I include what you need.

You might want to wait until we've nailed the tab switching interaction to see if this is still an issue, but what I'd like to see is: Current QD port (out param of GetPort()). Origin of port (print the port bounds returned by GetPortBounds()) Clipping rect of port (bounds of rgn after GetClip())

As best I can tell, this patch completely fixes the timing problem, in both Mozilla and Firefox. It should work with the latest CVS versions of Mozilla and Firefox and with the Mozilla 1.8b1 source tarball. (The Firefox 1.0.1 tarball requires a slightly different patch, which I'll post in the next message.) The nerve-center of tab-switching in Mozilla and Firefox is nsDeckFrame::IndexChanged() (in layout/xul/base/src). When you switch tabs, it (correctly) calls nsDeckFrame::HideBox() on the contents of the "old" tab before it calls nsDeckFrame::ShowBox() on the contents of the "new" tab -- i.e. it hides the old tab before displaying the new one (or it tries to). But (under the hood) the call to ShowBox() has (more or less) immediate results (in the form of calls to "paint" the new tab's contents), while (on OS X) the results of HideBox() are almost always delayed (on OS X the old tab gets hidden on a call to nsObjectFrame::Notify() (in layout/generic/nsObjectFrame.cpp), which is called periodically on a timer). This means that, other things being equal, the old tab is almost always hidden _after_ the new tab has been displayed. So if the old tab's contents are being redrawn frequently, it will often leave ghosts in the new tab. On Mozilla, other things are always (basically) equal -- so you often see tab ghosts. On Firefox other operations fiddle with the timing (sometimes delaying the calls to "paint" the new tab), so the results are less clear-cut. But (as we've seen) playing certain tricks can make even Firefox show tab ghosts pretty consistently. To solve the problem at the source, we need to make sure the old tab is hidden synchronously, without any delay. I chose to do this by sending a custom event to the "plugin instance owner". (It was actually a lot harder to figure out how to do this than to find the original problem -- partly because custom events and the Mozilla-family display hierarchy are almost completely undocumented.) This patch has been tested on Mozilla 1.8a6, Mozilla 1.8b1, Firefox 1.0.1 and the latest CVS versions of both browsers ... but only with Java applets. If you can compile Mozilla and/or Firefox, please test this patch with other kinds of plugins. For reasons that I don't understand (and which may be confined to my own system), I often had to "touch" the source files and compile a second time. So if you apply this patch and continue to see ghosts, please try that before reporting that the patch doesn't work.

I'm a little confused here. I would have thought that the widget/view for the old tab must get hidden before the new tab is shown. I'll have to debug through the widget code to see if this is really the case.

You might find the JEP's "-Djep.debug.visibility=true" setting useful. I also ended up porting that debugging info to the MRJ Plugin (so that I could see it with either Java 1.3.1 or Java 1.4.X). It turns out there isn't any difference in behavior between the Java versions, and the JEP's logs are (I think) more readable, and hence more useful. But I could post my MRJ Plugin JEP debugging patch, if you'd like to see it. I tracked down this bug's cause by (laboriously) using printf() statements to trace backwards from calls to SetWindow() in the MRJ Plugin. If you think it might be useful, I could post this, too. Finally, I'd suggest starting your debugging with Mozilla -- the problem is much more clear-cut there.

So.. this approach fails if there are two applets in the "old" tab, right? Why are we doing a sync paint on the tab we're switching to, anyway?

> So.. this approach fails if there are two applets in the "old" tab, > right? Sigh ... as written my patch will fail in this case. But that's what code review is for :-) It'd be pretty trivial to change the patch to send "hideplugin" events to every plugin in a tab. I'll do that. > Why are we doing a sync paint on the tab we're switching to, anyway? Mozilla isn't doing any explicit painting (as far as I can tell). It seems that the paint events just follow naturally (and quickly) on the plugin being "show"n.

Ah, I see. The paint happens off a PLEvent, while shutting down painting for the applet happens off a 17ms (interesting choice here) timer.... So perhaps what we should be doing is that hiding a view should automatically hide all descendant widgets? Right now we only propagate the hidden state to descendant views without frames, which won't catch the case here.

My patch (attachment 174692 [details] [diff] [review]) in bug 162134 attempted to have the widget code invalidate the plugin on hide, so as an ancestor widget of the content area is getting hidden on tab switch, that could should work.

But that patch doesn't work: https://bugzilla.mozilla.org/show_bug.cgi?id=162134#c216 I'll try to find out why.

> It'd be pretty trivial to change the patch to send "hideplugin" > events to every plugin in a tab. I'll do that. Done. I've tested that, when you switch from a tab with multiple applets in it, a "hideplugin" message is sent to each plugin (or more precisely to each plugin's nsPluginInstanceOwner). This patch should work for current CVS versions of Mozilla and Firefox and Mozilla 1.8b1.

> It'd be pretty trivial to change the patch to send "hideplugin" > events to every plugin in a tab. I'll do that. Done again. This patch should work for Firefox 1.0.1 (and possibly Mozilla 1.7.5/1.7.6, but I haven't tested that).

I see why my widget Invalidate() patch doesn't work; the synchronous paint never happens, because of an earlyl short-cirtuit in the widget code ("hey, I'm not visible, nothing to do!").

The problem with these changes is that they only fix view hiding and showing by nsDeckFrame, and not by other means. I think the fix needs to be more general. Having said that I'm not a fan of the synchronous paint hack, not only because it causes visible flashing on tab switching. I think what really needs to happen is some kind of notification fired from views when their visibility changes.

This is an alternative patch. What it does is to add the ability to register an nsIViewVisibilityListener on a view, which gets notified when the computed visibility of the view will change (ie. when it changes from being on-screen to off-screen, or vice versa). This is triggered from widget code; when a widget is hidden or shown, we notify the views for it and all descendent widgets. In nsObjectFrame.cpp, the plugin instance owner implements nsIViewVisibilityListener, and responds to the notification by adjusting the clip rect and sending the plugin an update event. This will happen *before* the tab switch (which is the point of this whole exercise). Roc: I'd appreciate comments on the view manager hacking. It seems a bit heavy-weight, but I'm out of alternatives.

> synchronous paint hack As far as I can tell, my patch doesn't do any painting on a "hideplugin" event. It calls FixUpPluginWindow() with inPaintState set to ePluginPaintIgnore. At most (if the visibility state has changed) FixUpPluginWindow() fiddles with the clipping rectangle and calls SetWindow() in the plugin. And there'd be no reason for a plugin to paint itself when it's being hidden (I know that the JEP just sets a "noupdate" flag). So what you say about a visible flash puzzles me -- maybe it has a different cause. In any case I'm compiling your patch ... and will test it when it's ready an hour or two from now :-)

> > synchronous paint hack This is what I was doing in my non-working patch ealier, and what camino does. Both called Invalidte(PR_TRUE) on the widget, which turns into a Paint() message on the frame, which, in turn, forces the plugin's clip rect to be updated. That's why it's a hack :) The question remains of what to do to the plugin on tab hide. My patch does empties the clip rect, then sends an update event, which I think will work for most plugins. Remember that we have to make this work for all plugins, not just JEP.

I misunderstood. I thought you were calling my patch a "synchronous paint hack". You were referring to your own patch(es).

Moreover, it seems to me that only a badly written plugin (or a badly implemented interface to the plugin) would need a "synchronous paint hack". When some object that can paint the screen is hidden, at most it should blank the part of the screen it "owns" (or "owned"). Once it leaves the new tenant can take care of moving around (or throwing out) the on-screen furniture. But it does sound like you have some very ill-behaved tenants to deal with :-)

I've now tried out Simon Fraser's patch (attachment 175801 [details] [diff] [review]), compiled into the latest CVS version of Mozilla. Mostly it worked fine (with the JEP and with Apple's QuickTime movie trailers). I didn't see any ghosts, even with the test-cases cited in this bug. I also didn't notice any screen flashes on tab switching ... but maybe that's because I have a CRT display (NEC MultiSync FE950+), not an LCD one :-) I did, though, have trouble with the following Shockwave object: http://www.forgefx.com/casestudies/prenticehall/ph/catapult/catapult.htm It usually got stuck partway through either a "show" or a "hide", and I had to manually resize the window to fix the display. (I wanted to test the Real plugin. But I couldn't find examples of Real videos that didn't open in separate, pop-up windows without tab controls.)

Re: director issues Director has its own set of problems. These are helped by turning off the "Hardware rendering" setting via the context menu. Re: Real examples www.weather.com has a bunch. Taking bug (hope that's ok Steven). BTW, if you want to chat, I'm often on #camino at irc.mozilla.org.

> Taking bug (hope that's ok Steven). No problem. My main interest is just in getting this bug fixed (since it does effect the JEP). By the way, I should point out that my own patch doesn't have any rendering problems with the Shockwave example ... though switching to or from a tab containing that object does seem unusually slow.

I don't really like driving these notifications down into widget. Also I don't like having view visibility notifications that don't fire on all visibility changes. How about we add a field to nsIView recording the child view manager even if we don't hook up that child into the view manager tree? Then we could walk the view tree from nsViewManager::SetViewVisibility firing view visibility change listeners without involving widget.

Adding a field to nsIView sounds OK.

> I don't really like driving these notifications down into > widget. Also I don't like having view visibility notifications > that don't fire on all visibility changes. It sounds like both of these objections are addressed to my patches (attachment 175780 [details] [diff] [review] and attachment 175781 [details] [diff] [review]), not Simon's (attachment 175801 [details] [diff] [review], which doesn't use the widget hierarchy at all). Of course I only used the widget hierarchy because I had to -- as I noted in the comment to my DispatchViaWidgetHierarchy() method, only the widget hierarchy extends past viewports (which means that the nsIBox objects on which nsDeckFrame::HideBox() and ShowBox() are called generally (always?) have no visible children, and neither do the views associated with them). > How about we add a field to nsIView recording the child view manager > even if we don't hook up that child into the view manager tree? Then > we could walk the view tree from nsViewManager::SetViewVisibility > firing view visibility change listeners without involving widget. Here you seem to be suggesting a way around the viewport problem, so that I'd no longer have to use the widget hierarchy. This sounds very reasonable, but I'm having trouble figuring out how it should work. What's the "child view manager" -- where does it come into play, and where would you record it in the new nsIView field? I take your point about being more comprehensive with the visibility notifications (Simon made the same point), but I don't (yet) know where else to put them (besides nsDeckFrame::HideBox() and nsDeckFrame::ShowBox()). Could you give me some suggestions? I assume you're just talking about plugins (and not about sending visibility change notifications to other kinds of objects) ... but let me know if I'm wrong. Simon's patch is quite different from mine. And I wonder if his patch won't cause trouble for some kinds of plugins by trying to do too much (e.g. by repainting the screen the screen on a "hide" event, instead of just calling SetWindow() in the plugin). But Simon does know much more than I do about all the different kinds of plugin that the Mozilla family of browsers has to deal with. If you (Robert) are willing to flesh out your suggestions a bit, I'd like to keep working on my patch, in parallel with Simon's.

(In reply to comment #41) > (I wanted to test the Real plugin. But I couldn't find examples of > Real videos that didn't open in separate, pop-up windows without tab > controls.) You can just figure out the URLs of those popups and open them directly rather than by the original, popup-creating links :-) Here's an example I recently happened upon: <http://www.scifi.com/battlestar/33_full_episode/>

> You can just figure out the URLs of those popups and open them > directly rather than by the original, popup-creating links :-) > Here's an example I recently happened upon: > <http://www.scifi.com/battlestar/33_full_episode/> Thanks for this suggestion! I've been making heavy use of it over the last couple of days.

My previous patches didn't stop Real videos from bleeding into other tabs. Now I've "borrowed" some of Simon's changes to nsObjectFrame::FixUpPluginWindow() (and ScrollPositionDidChange() and Paint()) to fix this problem ... but without breaking the following Shockwave object (as Simon's patch does): http://www.forgefx.com/casestudies/prenticehall/ph/catapult/catapult.htm The change on my original patch(es) is quite small, and the crucial part seems to be that I now (like Simon) make sure that (when a plugin has been made invisible) the plugin's clipRect is empty before SetWindow() is called. Unlike Simon, I'm not sending any extra update events to the plugin. (I also moved the code that fires the "hideplugin" event from nsDeckFrame.cpp to nsViewManager.cpp::SetViewVisibility(), so that this happens whenver a plugin is hidden -- not just when you change tabs. I'm no longer sure I interpreted Robert's remarks correctly -- he may very well have been talking about Simon's patch, and not mine. But this move seemed like a good idea.)

Comment on attachment 176073 [details] [diff] [review] Fix for CVS Mozilla and Firefox (rev3) > EXPORTS = \ >+ nsContainerFrame.h \ >+ nsFrame.h \ > nsFrameList.h \ >+ nsHTMLContainerFrame.h \ > nsHTMLParts.h \ >+ nsHTMLReflowCommand.h \ > nsHTMLReflowMetrics.h \ > nsHTMLReflowState.h \ > nsIAnonymousContentCreator.h \ > nsICanvasFrame.h \ > nsIFrame.h \ > nsIFrameDebug.h \ >@@ -96,13 +100,15 @@ > nsILineIterator.h \ > nsIObjectFrame.h \ > nsIPageSequenceFrame.h \ > nsIScrollableFrame.h \ > nsIScrollableViewProvider.h \ > nsIStatefulFrame.h \ >+ nsObjectFrame.h \ > nsReflowType.h \ >+ nsSplittableFrame.h \ > $(NULL) I think we need to do this without adding new exports and dependencies. >diff -U 6 -r src.old/view/src/Makefile.in src/view/src/Makefile.in >--- src.old/view/src/Makefile.in Sat Feb 5 17:16:38 2005 >+++ src/view/src/Makefile.in Wed Mar 2 15:28:47 2005 >@@ -52,12 +52,17 @@ > REQUIRES = xpcom \ > string \ > gfx \ > widget \ > dom \ > pref \ >+ content \ >+ layout \ >+ locale \ >+ necko \ >+ plugin \ > $(NULL) We can't make view dependent on clients of view.

So should I add a "FireHidePluginEvent" method somewhere in "layout" that can be called from nsViewManager::SetViewVisibility()?

> So should I add a "FireHidePluginEvent" method somewhere in "layout" > that can be called from nsViewManager::SetViewVisibility()? Or would that still be too much of dependency of "view" on "layout"? (I assume you're not against doing anything from SetViewVisibility() ... or are you?)

I think we have to fire something from SetViewVisibility(), but I'm not convinced that firing an event is the right thing; I'm not sure that such events would always reach their targets. What if the plugin is in an <iframe>?

I'm not terribly sure how what I've been calling the "widget hierarchy" works, but I strongly suspect that it includes everything, everywhere. So if I search it for nsObjectFrame objects I should find every plugin. I need to try my code out with some tough cases. You've already mentioned a plugin in an <iframe>. It should be easy enough to test that. Can you come up with a set of test cases that (if they all work) you'd accept as proof of my hunch? ... Hopefully not too large a set :-)

Note that firing events from view or frame code is risky business -- consider an event handler that does window.close() (with its immediate teardown of all the views and frames).

Yes, it's dangerous to recurse through a widget (or view or frame) hierarchy if something you do there might change the hierarchy while you're working on it. (This is true even if what you're doing isn't firing events.) And no, I hadn't thought of this -- thanks for pointing it out. But I don't think it's possible to fix bug 277067 without this kind of operation. So the question becomes -- is there some way to make it safe? I notice variables called "kungFuDeathGrip" scattered throughout the Mozilla source code. Maybe one of these would do the trick. How about if I changed FireEventInPluginOwners() to look like this: static void FireEventInPluginOwners(nsIView *aView, const nsAString& aEventName) { if (!aView || !aView->HasWidget()) return; #ifdef NS_DEBUG printf("Bug277067: FireEventInPluginOwner(), view's child frames:\n"); #endif nsCOMPtr<nsIWidget> kungFuDeathGrip = aView->GetWidget(); DispatchViaWidgetHierarchy(kungFuDeathGrip, aEventName); }

Maybe I should also put a "death grip" on the view manager itself: nsCOMPtr<nsIViewManager> kungFuDeathGrip2 = this;

> Yes, it's dangerous to recurse through a widget (or view or frame) >hierarchy if something you do there might change the hierarchy while > you're working on it. (This is true even if what you're doing isn't > firing events.) That's not true. All the layout code runs on the UI thread, so there are no thread synchronization issues. Assuming your recursion is just under a function call, nothing else is running that can mess with layout data structures.

> That's not true. Not correct. It's not a threading issue. The "event" I "fire" might (say) destroy the entire hierarchy I'm working with, including its top node, thus (potentially) invalidating the pointers I'm using. I thought this was exactly what you were saying :-)

> I thought this was exactly what you were saying :-) Well, what Boris was saying.

You're right: I mentally skipped the "if something you do there" part of the sentence.

I've done some housecleaning on my patches, and some more testing. The meat of the patch has been moved yet again (to nsObjectFrame.cpp), and can now be called from anywhere in the source tree with only minimal additional dependencies. I've done my best to safely handle any changes that we can reasonably expect to happen while dispatching "events" to all the plugins in a given widget hierarchy. I'd say that these don't include the closing of the current browser window: None of the available interfaces (as far as I know) gives plugins a handle to this window. And though it's true that a plugin might find this information independently (the JEP does), I can't see how it would ever be kosher for a plugin to close the current browser window. The worst _reasonable_ thing a plugin could do (on an "event" handler) would be to destroy itself. So I (temporarily) added code to call Destroy() on the current plugin instance every fifth time the "hideplugin" handler was called. Oddly, this didn't have any effect on the Real, QuickTime or Shockwave plugins. But it "worked" with the MRJ Plugin (the applet really was destroyed), and otherwise had no ill effects (no crashes, hangs, or other visible errors). I tested to be sure my code can find (and send "events" to) a plugin in an iframe -- it can.

> None of the available interfaces (as far as I know) gives plugins a handle to > this window. But you're dispatching a DOM event to the content node. This means it will propagate through the DOM, firing DOM event handlers as it goes. These can execute arbitrary JavaScript off the event...

Do you really think I need to deal with the case of the "hideplugin" event handler (indirectly) closing the current browser window? If so, I can give it a try.

No, I think that firing DOM events synchronously from inside frame code is something we just shouldn't be doing, period.

By the way, when I call AddEventListener() for the "hideplugin" event, I specify that the event should be "captured", so that it won't "bubble" up to other event targets. This may mean that it _won't_ really propagate through DOM.

> I think that firing DOM events synchronously from inside frame code > is something we just shouldn't be doing, period. Then how would you solve this problem (which is a very serious one on OS X)?

I agree with Boris. I think we should seek a more procedural approach (like crawling either the widget or view hierarchies manually). Roc: what's involved in linking the view managers together, as you suggested above? Steven: my patch demonstrates that we can do this without firing events. We just need to find the cleanest way to do it.

> This may mean that it _won't_ really propagate through DOM. You can't prevent capturing event listeners higher up the DOM tree from seeing the event... > Then how would you solve this problem Simon's approach (notifying the plugin instances directly) seems like a better one on this count. I agree that forcing a paint is not nearly as clean as actually hiding the plugin window; perhaps use of Simon's notification system could be combined with your approach to hiding the plugin?

> I think we should seek a more procedural approach (like crawling > either the widget or view hierarchies manually) I _am_ doing this (most particularly in DispatchViaWidgetHierarchy()). > Simon's approach (notifying the plugin instances directly) seems > like a better one on this count. I'm not wedded to using nsIDOMEvents. I just thought it seemed simpler (and more convenient). I guess not ... :-) > perhaps use of Simon's notification system could be combined with > your approach to hiding the plugin? Sounds reasonable. I'll try to do this.

(In reply to comment #70) > I agree with Boris. I think we should seek a more procedural approach (like > crawling either the widget or view hierarchies manually). Roc: what's involved > in linking the view managers together, as you suggested above? > > Steven: my patch demonstrates that we can do this without firing events. We just > need to find the cleanest way to do it. View managers get linked in nsDocumentViewer::MakeWindow here: http://lxr.mozilla.org/mozilla/source/layout/base/nsDocumentViewer.cpp#1973 You'd need to tackle the "don't-link" case there and call a new method on nsIView to set the "owned but unlinked" child view manager (e.g., call it SetOwnedViewManager()). Then you can have nsViewManager::SetViewVisibility traverse all the child views and view managers, following the unlinked view manager children as well as the normal child views.

> I'm not wedded to using nsIDOMEvents. I just thought it seemed > simpler (and more convenient). I guess not ... :-) Actually, now that most of the code is in nsObjectFrame.cpp, it'd be _very_ simple to add a HandlePluginEvent() method to nsObjectFrame and just call _that_ from DispatchViaWidgetHierarchy() (or whatever function replaces it). Then I can forget nsIDOMEvent like a bad dream :-)

You have to avoid adding any new dependencies in the view module.

> You'd need to tackle the "don't-link" case there ... I'll look into this. But I already have DispatchViaWidgetHierarchy(), which seems to do everything that's needed. Is browsing the "widget hierarchy" somehow fragile? Are future changes likely to change how it works (or break it)? Is this something that's not _supposed_ to work? :-)

> You have to avoid adding any new dependencies in the view module. You don't want me to add any exports to the view module (which would mean I couldn't use nsView::GetViewFor())? Or you don't want me to add to the view module's REQUIRES list (in Makefile.in)? If the latter, I don't see how it'd be possible to do what needs to be done from nsViewManager::SetViewVisibility().

> Or you don't want me to add to the view module's REQUIRES list (in > Makefile.in)? Yes, this. New dependencies are to be avoided at all costs, as they impact the future maintainability of the code. My patch doesn't add any new dependencies (but it does export a new interface, which is OK).

I've tried patches from both Steven and Simon, neither work against Firefox 1.0.4 sources. Simons patch fails building in nsWindow::RecursiveNoteVisibilityWillChange with mFirstChild not being defined. I'm having this bug hit me pretty hard with Komodo on OSX. We're porting Komodo, and using a Scinilla port which is HIView based. We then have a XBL widget which embeds the plugin using html:embed in the widgets content. This widget is then used in a xul tab widget (not the tabbed browser widget). When switching to a tab that contains only xul (no plugin) I have to scroll the tabs content to get it to refresh. Simon has suggested one possible workaround for us, so I will be trying that this week, but if the patches could be freshend that might handle our needs until this issue is resolved.

It'll be a while before I have the time to start working on this again ... so here's a stupid question: Did you try applying the patches "by hand", instead of just using the "patch" command?

Hi Steven, I assume that was directed at me. Yes, I applied the patches by hand. I've been focusing more on Simon's patch at the moment. Shane

not gonna make it.

Just an FYI, both patches work for me on trunk, and I was able to get the last patch (176603) to work with FF1.0.4. While it doesn't solve all my draw issues, I am leaning towards the remaining problems being related to using hiview objects that handle events directly rather than depending on the plugin api NPP_HandleEvent call.

Would this bug cause the problem where if you view an applet in osx and hit back the screen is not refrehsed properly where the applet frame was? instead there is just a big white area left over where the applet used to be. This bug occurred in all the nightlies I have tried (including the Jan 20th build) and happened ever since the CFRunLoop was added. Should a new bug be filed or is this the same problem? Greg

> is this the same problem? It's very difficult to tell ... which shouldn't surprise you if you've skimmed through the rest of the comments here. The only way to be sure is to try one or more of these patches yourself. Since the patches are now rather old, you'll apparently need to make some changes to them (possibly only minor ones). At the very least, you'll probably need to apply the patches "by hand" (instead of using the "patch" command), looking through the destination files to find the proper context for each "chunk". You will, of course, also need to be able to build Mozilla/Firefox/Camino from scratch. If this doesn't make sense to you, I'm afraid you'll have to wait until somebody else does the work (of retesting these patches on recent nightlies). Whether or not you can test the patches, you _can_ do the following: Provide examples of URLs that trigger the problem you report, and find out exactly which nightly they started occuring with.

I've posted a simple plugin (non-java) to repro this problem in bug 307262

I finally resolved the last of my paint issues, and can validate that, at least of us, patch 176601 works fine to fix this issue (against moz 1.8 branch).

slight correction on my last statement...last remaining issue is if the plugin is in an iframe, and you replace the iframe contents. it sometimes leaves a blank space where the plugin was. This is the same visual effect that plugins in tabs would sometimes do.

This bug needs to get fixed, and I don't have time.

I should add a note regarding comment #88, we were putting XUL, with a XBL widget that wraps our plugin, into the iframe. I've since changed that whole area, and we currently are down to one last paint issue with regards to the plugin... In Komodo, we have multiple tabs (if multiple files open) with our plugin taking 100% of the tabpanel. We have one tab that has XUL content. Our "close all" command does not close the XUL tab. If I close all files, all the plugin tabs are removed, and the XUL tab does not get repainted. I'm at a point where I'm considering a rewrite to make scintilla a XUL widget/element rather than going through the plugin layer. That would (hopefully) solve a lot of our issues, and remove a lot of our patches we've generated to work around plugin limitations.

Any chance of this getting addressed for FF2? We've been using the patches in Komodo for the past 6 months with pretty good results, though not 100%. I'll attach updated patches that we've been applying to the 1.8.0 and 1.8 branches.

this is split into two patches since our windows patch executable sometimes crashes on large patches

and here's part2

FYI, recently I've recieved in-house reports of similar symptoms on windows when using MOZILLA_1_8_BRANCH, have not been able to repro.

Comment on attachment 217040 [details] [diff] [review] part1.patch I don't think I'm a good person for this review.

which branch are the patches meant for?

The patches apply to both MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH. We have a bit of a patch system that allows us to differentiate our patches when necessary, but these will apply to both, with some offsets.

These patches only change behaviour on OSX, right?

biesi: there are no good reviewers for these patches, but we can't leave them on the floor.

They are for osx only. They are based on patches from Steven Michaud and/or Simon Fraser (earlier in this bug), perhaps they can review?

Okay, we need some Mac people to give us some attention here.

This patch adds a view depencency on layout. Are we OK with that? As discussed earlier in this bug, I'm not fully happy with the patches.

> This patch adds a view depencency on layout. As I said in comment #77, I'm not sure there's a reasonable way to avoid this. (Though, as I noted in comment #74, I probably _can_ deal with another objection by Boris Zbarsky.) I still don't have much spare time. But this is an important bug. And if people are willing to live with a view dependency (Mac OS X only) on layout, I figure I can set aside some time in the next few weeks to bring this (long delayed) work to a conclusion. I'll even make a good faith effort to avoid the view dependency on layout ... but it that's an ironclad condition, then I'm afraid I've already spent too much time on this bug :-)

I can live with the view dependency on layout. Views are going to go away at some point anyway.

Comment on attachment 217040 [details] [diff] [review] part1.patch what I really wanted to know with my branch question is whether this is needed on trunk too, and whether this patch applies there. ok, so if you really think I should review this... 1. Don't use tabs, use spaces 2. +{ + if (!frame) + return; + nsIContent *content = frame->GetContent(); + if (!content) + return; + if (!widget) + return; + if (!aView) + return nsnull; + if (!aView) + return NS_ERROR_NULL_POINTER; When can either of those be null? Please don't nullcheck things unless they actually can be null. 3. should this really use DOM events, which can be caught by content too? 4. + frame = NS_STATIC_CAST(nsFrame *, view->GetClientData()); is this a safe cast? + nameUTF8 = NS_LossyConvertUTF16toASCII(name); LossyCopyUTF16toASCII(name, nameUTF8); and the UTF8 is a lie, this function does not at all guarantee ascii. I guess it does for frame names... + nameUTF8 += (const nsACString&) nsPrintfCString("0x%.8X", (void *) widgetLocal); what's this cast for? why not use %p as format specifier? is the default length of nsPrintfCString long enough? (Hm, why do frame names use nsAString rather than nsACString anyway?) + QueryInterface(NS_GET_IID(nsIObjectFrame), (void**)&objectFrame))) use the type safe CallQueryInterface instead + return; +} no point in this return + if (eventType == NS_LITERAL_STRING("hideplugin")) { Use EqualsLiteral (I think we have that on the 1.8 branches too) + // We need this to deal with bug 277067 on the Mac. For more info see Can you describe the actual purpose in the comment, at least briefly? forcing people to open a webbrowser to find out what the code is good for isn't so nice. I'm not convinced about putting historical info in comments anyway. that's what cvs logs are for. +class nsObjectFrameHelper { +public: + static nsresult DispatchEventToPluginOwners(nsIView *aView, const nsAString& aEventName); +}; why not put this on nsObjectFrame instead?

Comment on attachment 217041 [details] [diff] [review] part2.patch +EXPORTS = nsView.h this seems bad to me, but roc's the owner here... + // But on the Mac the "new" box gets shown synchronously, while the "old" + // one only gets hidden asynchronously Is that a bug and should that be fixed instead?

also: if this a problem in mac only, why does nsObjectFrame have to be uglified even more than it already is for it? Why can't this live in some platform specific file. I'd hate to see more hacks in the object frame.

It'll be a week or two before I have time to really get back into this. But, in the meantime, here are responses to a few of cbiesinger's comments: > 3. should this really use DOM events, which can be caught by content > too? I think I've found a way to avoid using DOM events (see comment #74). > +class nsObjectFrameHelper { > +public: > + static nsresult DispatchEventToPluginOwners(nsIView *aView, const nsAString& aEventName); > +}; > > why not put this on nsObjectFrame instead? That's what I plan to do. > + // But on the Mac the "new" box gets shown synchronously, while the "old" > + // one only gets hidden asynchronously > > Is that a bug and should that be fixed instead? Yes, this is the ultimate cause of the problem. And, if possible, it should (eventually) be fixed. But following that strategy is likely to be even more fraught with difficulties than the one that I ended up choosing, and to take even longer to bring results. Who knows what depends on that 17ms timer, and what alternatives (if any) are available? We now have working code (Shane Caraveo's tests show this). And this bug causes severe display problems on the Mac (see bug 162134), which really should be fixed soon. (In my experience bug 162134 is much worse on Firefox 1.5 than it was on previous versions.) Where changes are easy to make (e.g. avoiding the use of DOM events), I'm happy to make them. But anything more ambitious should wait for the next iteration. > if this a problem in mac only, why does nsObjectFrame have to be > uglified even more than it already is for it? Why can't this live in > some platform specific file. I'd hate to see more hacks in the > object frame. Yes, nsObjectFrame contains lots of OS-specific code. But that cuts both ways. It's very arbitrary to decide that you should stop accepting new OS-specific code in nsObjectFrame _now_.

> It's very arbitrary to decide that you should stop > accepting new OS-specific code in nsObjectFrame _now_. I'd have not accepted it earlier but I wasn't around then.

Don't export nsView.h. Move GetViewFor to nsIView instead.

Sorry to take so long to get back to this ... but it's only in the last couple of weeks that I've been able to put aside time for it. Since so much time has passed, and since Camino is now also quite badly effected by this bug, I've been more ambitious than I originally planned. My new patch fixes bug 277067 in all of Mozilla.org's browsers -- both Carbon-based (Firefox and SeaMonkey) and Cocoa-based (Camino). Most of my code is now in a new interface that I've called nsIPluginHelper. This makes it possible to get rid of the View dependency on Layout, and for my patch to work with shared builds of Camino. It also greatly reduces the amount of new OS-X-specific code in nsObjectFrame.cpp and nsObjectFrame.h. I found that my original "handler" code (formerly in nsPluginInstanceOwner::HandleEvent(), now in nsPluginInstanceOwner::HandleOFEvent()) had become very crashy (though the crashes didn't happen in my code, my code seemed to trigger crashes elsewhere). That's why I added calls to nsIPluginWidget::StartDrawPlugin() and EndDrawPlugin() -- which seems to have fixed the problem. I stopped nsPluginInstanceOwner::Notify() (which is called on a timer) from calling FixUpPluginWindow(ePluginPaintIgnore). As best I can tell, my fix makes these calls unnecessary (though more testing is needed to be sure of this). And getting rid of these calls eliminates the root cause of this bug (277067). I've (briefly) tested my fix on the trunk and the 1.8 branch (and I found that it functioned in those environments). But the trunk and the 1.8 branch are crawling with bugs, so I think initial tests should be done on the 1.8.0 branch (which is in reasonably good shape). I don't disagree that this patch should take the usual route -- getting landed first on the trunk, then on the 1.8 branch, and then (if everyone's still happy with it and that branch is still "live") on the 1.8.0 branch. But tests on the 1.8.0 branch are (currently) much more likely to shake out problems that are actually caused by this patch. Since I've added a new interface, testers will need to delete compreg.dat and xpti.dat in their profiles before running a patched browser for the first time (otherwise the nsIPluginHelper "service" won't get "registered"). If you're testing with Java applets (which you probably should be), you may need to downgrade the JEP to version 0.9.5+d: The current version (0.9.5+e) contains a workaround for this bug, and so will interfere with your tests. Shane: I'm particularly interested to hear the results of your tests ... that is if you haven't given up on this whole business :-)

I've built this against the 1.8 branch, which is what we're basing komodo 4 on. Seems to work as well as the older patches. I still have one condition where it messes up. I have tab widgets with embedded plugins (for the editor buffers), but one tab is xul content (our start page). If I have several files open, and do a "close all", the start page is left open (intentionaly), but has not repainted until I scroll or resize. I also see similar problems on windows (even with the older patches) on two of my coworkers machines, but my machine does not have the problem.

> I also see similar problems on windows (even with the older patches) > on two of my coworkers machines, but my machine does not have the > problem. Whatever you see on other platforms can't have anything to do with this patch -- it only has any effect on Mac OS X. > I have tab widgets with embedded plugins (for the editor buffers), > but one tab is xul content (our start page). If I have several > files open, and do a "close all", the start page is left open > (intentionaly), but has not repainted until I scroll or resize. If this problem is OS-X-specific, it might be caused by my having stopped FixUpPluginWindow() from being called from nsPluginInstanceOwner::Notify(). Try reverting nsPluginInstanceOwner::Notify() to the way it was before my patch. In other words, try replacing this: nsPluginPort* pluginPort = GetPluginPort(); with this: nsPluginPort* pluginPort = FixUpPluginWindow(ePluginPaintIgnore);

(In reply to comment #113) > > I also see similar problems on windows (even with the older patches) > > on two of my coworkers machines, but my machine does not have the > > problem. > > Whatever you see on other platforms can't have anything to do with > this patch -- it only has any effect on Mac OS X. I didn't mean the problems were related to this patch, but that similar problems to this bug (ie. paint issues with plugins in tabs) are happening on windows (with or without this patch). > > I have tab widgets with embedded plugins (for the editor buffers), > > but one tab is xul content (our start page). If I have several > > files open, and do a "close all", the start page is left open > > (intentionaly), but has not repainted until I scroll or resize. > > If this problem is OS-X-specific, it might be caused by my having > stopped FixUpPluginWindow() from being called from > nsPluginInstanceOwner::Notify(). It was happening with the older patches as well, so it's not new. > Try reverting > nsPluginInstanceOwner::Notify() to the way it was before my patch. In > other words, try replacing this: > > nsPluginPort* pluginPort = GetPluginPort(); > > with this: > > nsPluginPort* pluginPort = FixUpPluginWindow(ePluginPaintIgnore); > Tried it, it made no difference.

> I have tab widgets with embedded plugins (for the editor buffers), > but one tab is xul content (our start page). If I have several > files open, and do a "close all", the start page is left open > (intentionaly), but has not repainted until I scroll or resize. So this problem is unrelated to bug 277067? That's what it sounds like. (Among other things, bug 277067 happens on switching tabs, but this problem doesn't.) Have you found any problems that you think might be caused by my patch (any version, but especially the latest one)?

(In reply to comment #115) > > I have tab widgets with embedded plugins (for the editor buffers), > > but one tab is xul content (our start page). If I have several > > files open, and do a "close all", the start page is left open > > (intentionaly), but has not repainted until I scroll or resize. > > So this problem is unrelated to bug 277067? That's what it sounds > like. (Among other things, bug 277067 happens on switching tabs, but > this problem doesn't.) I think it is related to this bug, I'm not saying it is caused by your patch. > Have you found any problems that you think might be caused by my patch > (any version, but especially the latest one)? You latest patch is fine. It is no different (so far as I can tell) from the earlier patches, which have also worked well for us. I still have some problems, as I was trying to describe, that neither patch fixed.

(Perhaps a bit off-topic ... but oh well.) > I have tab widgets with embedded plugins (for the editor buffers), > but one tab is xul content (our start page). If I have several > files open, and do a "close all", the start page is left open > (intentionaly), but has not repainted until I scroll or resize. This sounds like it could be a problem _closing_ tabs. Do ghosts from one or more of the closed tabs get left in your "start page" (and is that why it needs to be repainted)? Are you sure that your plugin stops drawing when the browser calls NPP_Destroy() (or nsIPluginInstance::Destroy())? Is the browser in fact making these calls?

Bug #425715 has a testcase attached that reproduces this problem easily for me.

Bug 425715 turned out to be a dupe of this one and that one is quite visible. So would be great to get this fixed ASAP if we can't get it fixed for FF3

Implement roc's suggested fix: when a view's visibility is changed, nsIWidget::Show is called on the view's widget. Take advantage of that by walking the native widget tree in Cocoa looking for plugin ChildViews and notifying the associated plugin that it is no longer visible by calling SetWindow with an empty clipping rect. I cheat a little bit by creating using a freshly zeroed nsPluginWindow and passing that to SetWindow when hiding the plugin, because otherwise I'd need to get at the nsPluginInstanceOwner to find and modify the real nsPluginWindow.

rev the IID on nsIPluginWidget +static void hideChildPluginViews(NSView* aView) HideChildPluginViews I'm not sure if passing all-null as the nsluginWindow is safe, if we've never done that before. Can we just hide the Cocoa view or something? Perhaps not, but we need a better solution. + if (widget) + widget->HidePlugin(); + } else + hideChildPluginViews(view); Ew, curly braces all around please The only other thing I'm worried about is possible leaks due to cycles between the pluginstance and the widget. Can you check that out? Some of these classes should be using NSPR refcount logging at least.

Address roc's comments. The widget now has a weak (manually managed, raw) pointer to the nsPluginInstanceOwner. Use the nsPluginWindow owned by this to notify the plugin that it is no longer visible by setting the clip rect to all zeroes. It'd be slightly nicer to use nsPluginInstanceOwner::FixUpPluginWindow(ePaintDisable), but that is not part of the interface (and is OS X only, so it would be ugly to expose it via the interface). Next time we call in to the plugin, nsPluginInstanceOwner::FixUpPluginWindow will notice the clip rect has changed and call SetWindow again. The widget's reference to the plugin instance owner is set up when the widget is created, and cleared when the plugin instance is destroyed. From reviewing the code in nsObjectFrame.cpp and looking at the ownership graph, I can't see any case where we might end up with the widget holding a bad reference to a plugin instance owner, but it's quite possible I've missed something. Sorry about that last patch, it was pretty dumb. :-)

Comment on attachment 315901 [details] [diff] [review] patch v2 Looks good to me.

Comment on attachment 315901 [details] [diff] [review] patch v2 Requesting approval. Fixes a nasty bug we've had for quite a while. I think the main risk is leaving a pointer to a dead plugin instance owner on the associated widget, which could happen if I've misunderstood some aspect of the widget and plugin instance owner lifetimes. The code that uses this pointer is invoked for any QuickDraw plugin when hidden (e.g. switching tabs), so any regressions should show up relatively quickly.

This bug still effects the JEP, by the way -- though the JEP has long had an effective workaround for it. I hope you've been testing your patch with Java applets.

Can we please address steven's concern about testing with Java applets? Approval pending answer.

I've tested with the both current JEP (0.9.6.3) and the older version before it had a workaround (0.9.5+d). Without my patch applied, I see the expected problems where the applet content is drawn over other tabs when switching (the problem is far worse with 0.9.5+d, it took a lot more experimentation to see it happen with 0.9.6.3). With the patch applied, the applet content is only visible when the tab containing the applet is selected. There seem to be some other issues, e.g. some applets disappear or flash white and then repaint when the window gains or loses focus, but this happens with and without my patch. Is this a known problem with JEP?

Thanks, Matthew. It seems you've done quite a thorough job of testing. I'll of course test myself once you're patch has landed ... but it sounds like I shouldn't have anything to worry about. > There seem to be some other issues, e.g. some applets disappear or > flash white and then repaint when the window gains or loses focus, > but this happens with and without my patch. Is this a known problem > with JEP? When you switch tabs away from an applet that redraws continuously (like the NOAA's radar applets), you often see a ghost for a fraction of a second before it's replaced by the correct contents -- this the JEP's workaround for bug 277067 in action. But this doesn't seem to be what you're talking about. Sometimes applets don't draw completely when they're first loaded. This is one consequence of bug 416716, and probably also not what you're talking about. And (of course) an applet will temporarily go blank as you're resizing a window, then redraw when you're finished -- this is by design. I'm not aware of display problems that happen when a window gains or loses focus, though. Please give me some examples.

(In reply to comment #129) > Sometimes applets don't draw completely when they're first loaded. > This is one consequence of bug 416716, and probably also not what > you're talking about. Ah, this is part of what I was seeing. Cycling focus on the window causes the missing applet to paint. > I'm not aware of display problems that happen when a window gains or > loses focus, though. Please give me some examples. My local testcase has multiple copies of the applet from http://www.schubart.net/rc/ displayed (each in a position: fixed div). All but the first applet will flash white and then paint normally whenever the window gains or loses focus. It's pretty minor in a normal build, I think my changes to slow down the idle timer (or something else I tweaked) during testing might have made it look worse. The other thing I noticed is that the window management widgets (close, minimize, etc.) and title bar text colour don't change to reflect the focus state of the window, but only when a Java applet is displayed in the active tab when the focus changes. I've filed bug 430207 for that.

Thanks for the follow up. Approving.

Comment on attachment 315901 [details] [diff] [review] patch v2 a1.9+=damons

Fix a minor merge conflict.

mozilla/layout/generic/nsObjectFrame.cpp 1.652 mozilla/widget/public/nsIPluginWidget.h 1.6 mozilla/widget/src/cocoa/nsChildView.h 1.91 mozilla/widget/src/cocoa/nsChildView.mm 1.339