Mozilla mistimes changing QuickDraw plugin visibility when switching tabs

RESOLVED FIXED in mozilla1.9

Status

()

P1
major
RESOLVED FIXED
14 years ago
a year ago

People

(Reporter: smichaud, Assigned: kinetik)

Tracking

Trunk
mozilla1.9
PowerPC
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b3 -
blocking1.8b5 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 10 obsolete attachments)

6.04 KB, text/plain
Details
4.95 KB, text/plain
Details
5.87 KB, text/plain
Details
5.89 KB, text/plain
Details
7.14 KB, text/plain
Details
15.93 KB, patch
Details | Diff | Splinter Review
28.59 KB, patch
Details | Diff | Splinter Review
9.29 KB, patch
Biesinger
: review-
Details | Diff | Splinter Review
3.42 KB, patch
Details | Diff | Splinter Review
28.65 KB, patch
Details | Diff | Splinter Review
8.73 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
(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.
(Reporter)

Comment 1

14 years ago
Created attachment 170380 [details]
Demonstration of timing problem in Mozilla 1.7.5

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.
(Reporter)

Comment 2

14 years ago
Created attachment 170381 [details]
Demonstration of correct timing in 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 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.
(Reporter)

Comment 3

14 years ago
Created attachment 170384 [details]
Demonstration of correct timing in Firefox 1.0

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.
(Reporter)

Comment 4

14 years ago
Created attachment 170388 [details]
Demo of timing problem in Mozilla 1.8 nightly

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.
(Reporter)

Comment 5

14 years ago
(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.

(Reporter)

Comment 6

14 years ago
Created attachment 170394 [details]
Demo of timing problem in Firefox 1.0

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.
Status: UNCONFIRMED → NEW
Component: General → Plug-ins
Depends on: 162134
Ever confirmed: true
Product: Mozilla Application Suite → Core
Assignee: general → nobody
QA Contact: general → core.plugins

Updated

14 years ago
Flags: blocking1.8b?
Flags: blocking-aviary1.1?
Is this a problem with a recent (and here recent means "today") build?
(Reporter)

Comment 8

14 years ago
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.

(Reporter)

Comment 9

14 years ago
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).
(Reporter)

Comment 11

14 years ago
> 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.
(Reporter)

Comment 12

14 years ago
Created attachment 173304 [details] [diff] [review]
Make Firefox 1.0's behavior match the latest nightlies (not a fix)

> 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.
(Reporter)

Comment 13

14 years ago
> 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.
(Reporter)

Comment 15

14 years ago
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.
(Reporter)

Comment 17

14 years ago
> 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
Flags: blocking1.8b?
Flags: blocking1.8b2?
Flags: blocking1.8b-

Comment 19

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

Comment 20

14 years ago
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.
(Reporter)

Comment 21

14 years ago
> 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.

Comment 22

14 years ago
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())
(Reporter)

Updated

14 years ago
Assignee: nobody → smichaud
(Reporter)

Comment 23

14 years ago
Created attachment 175761 [details] [diff] [review]
Fix for CVS Mozilla and Firefox

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.
(Reporter)

Comment 24

14 years ago
Created attachment 175762 [details] [diff] [review]
Fix for Firefox 1.0.1

Comment 25

14 years ago
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.
(Reporter)

Comment 26

14 years ago
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?
(Reporter)

Comment 28

14 years ago
> 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.

Comment 30

14 years ago
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.
(Reporter)

Comment 31

14 years ago
But that patch doesn't work:

https://bugzilla.mozilla.org/show_bug.cgi?id=162134#c216

I'll try to find out why.

(Reporter)

Comment 32

14 years ago
Created attachment 175780 [details] [diff] [review]
Fix for CVS Mozilla and Firefox (rev2)

> 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.
Attachment #175761 - Attachment is obsolete: true
(Reporter)

Comment 33

14 years ago
Created attachment 175781 [details] [diff] [review]
Fix for Firefox 1.0.1 (rev2)

> 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).
Attachment #175762 - Attachment is obsolete: true

Comment 34

14 years ago
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!").

Comment 35

14 years ago
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.

Comment 36

14 years ago
Created attachment 175801 [details] [diff] [review]
Patch to allow widget to notify plugins (via views) when hiding/showing

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.
(Reporter)

Comment 37

14 years ago
> 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 :-)

Comment 38

14 years ago
> > 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.
(Reporter)

Comment 39

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

(Reporter)

Comment 40

14 years ago
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 :-)

(Reporter)

Comment 41

14 years ago
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.)

Comment 42

14 years ago
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.
Assignee: smichaud → sfraser_bugs
(Reporter)

Comment 43

14 years ago
> 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.

Comment 45

14 years ago
Adding a field to nsIView sounds OK.
Status: NEW → ASSIGNED
(Reporter)

Comment 46

14 years ago
> 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/>
(Reporter)

Comment 48

14 years ago
> 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.

(Reporter)

Comment 49

14 years ago
Created attachment 176073 [details] [diff] [review]
Fix for CVS Mozilla and Firefox (rev3)

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.)
Attachment #175780 - Attachment is obsolete: true
(Reporter)

Comment 50

14 years ago
Created attachment 176075 [details] [diff] [review]
Fix for Firefox 1.0.1 (rev3)
Attachment #175781 - Attachment is obsolete: true

Comment 51

14 years ago
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.
(Reporter)

Comment 52

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

(Reporter)

Comment 53

14 years ago
> 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?)

Comment 54

14 years ago
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>?
(Reporter)

Comment 55

14 years ago
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).
(Reporter)

Comment 57

14 years ago
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);
}

(Reporter)

Comment 58

14 years ago
Maybe I should also put a "death grip" on the view manager itself:

nsCOMPtr<nsIViewManager> kungFuDeathGrip2 = this;

Comment 59

14 years ago
> 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.
(Reporter)

Comment 60

14 years ago
> 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 :-)

(Reporter)

Comment 61

14 years ago
> I thought this was exactly what you were saying :-)

Well, what Boris was saying.

Comment 62

14 years ago
You're right: I mentally skipped the "if something you do there" part of the
sentence.
(Reporter)

Comment 63

14 years ago
Created attachment 176601 [details] [diff] [review]
Fix for Mozilla 1.8b1 and CVS Mozilla and Firefox (rev4)

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.
Attachment #176073 - Attachment is obsolete: true
(Reporter)

Comment 64

14 years ago
Created attachment 176603 [details] [diff] [review]
Fix for Firefox 1.0.1 (rev4)
Attachment #176075 - Attachment is obsolete: true
> 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...
(Reporter)

Comment 66

14 years ago
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.
(Reporter)

Comment 68

14 years ago
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.

(Reporter)

Comment 69

14 years ago
> 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)?

Comment 70

14 years ago
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?
(Reporter)

Comment 72

14 years ago
> 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.
(Reporter)

Comment 74

14 years ago
> 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
:-)

Comment 75

14 years ago
You have to avoid adding any new dependencies in the view module.
(Reporter)

Comment 76

14 years ago
> 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? :-)

(Reporter)

Comment 77

14 years ago
> 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().

Comment 78

14 years ago
> 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).

Updated

14 years ago
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-

Comment 79

13 years ago
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.

Updated

13 years ago
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking1.8b2-
Flags: blocking1.8b-
(Reporter)

Comment 80

13 years ago
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?

Comment 81

13 years ago
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

Comment 82

13 years ago
not gonna make it. 
Flags: blocking1.8b4?
Flags: blocking1.8b4-
Flags: blocking-aviary1.1?

Comment 83

13 years ago
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.

Updated

13 years ago
Blocks: 300956

Comment 84

13 years ago
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

(Reporter)

Comment 85

13 years ago
> 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.

Updated

13 years ago
Blocks: 298961

Comment 86

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

Comment 87

13 years ago
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).

Comment 88

13 years ago
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.

Updated

13 years ago
Blocks: 309691

Updated

13 years ago
Blocks: 312563

Comment 89

13 years ago
This bug needs to get fixed, and I don't have time.
Assignee: sfraser_bugs → nobody
Severity: normal → major
Status: ASSIGNED → NEW
Priority: -- → P1

Updated

13 years ago
Blocks: 162134
No longer depends on: 162134

Comment 90

13 years ago
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.

Comment 91

13 years ago
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.

Comment 92

13 years ago
Created attachment 217040 [details] [diff] [review]
part1.patch

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

Comment 93

13 years ago
Created attachment 217041 [details] [diff] [review]
part2.patch

and here's part2

Comment 94

13 years ago
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.

Updated

13 years ago
Attachment #217040 - Flags: superreview?(roc)
Attachment #217040 - Flags: review?(cbiesinger)
Comment on attachment 217040 [details] [diff] [review]
part1.patch

I don't think I'm a good person for this review.
Attachment #217040 - Flags: review?(cbiesinger) → review?
which branch are the patches meant for?

Comment 97

13 years ago
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.

Comment 100

13 years ago
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.

Comment 102

13 years ago
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.
(Reporter)

Comment 103

13 years ago
> 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?
Attachment #217040 - Flags: review? → review-
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.
(Reporter)

Comment 108

13 years ago
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.
(Reporter)

Comment 111

12 years ago
Created attachment 225438 [details] [diff] [review]
Fix for mozilla1.8.0 branch (rev5)

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 :-)
Attachment #176601 - Attachment is obsolete: true
Attachment #176603 - Attachment is obsolete: true

Comment 112

12 years ago
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.
(Reporter)

Comment 113

12 years ago
> 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);

Comment 114

12 years ago
(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.


(Reporter)

Comment 115

12 years ago
> 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)?

Comment 116

12 years ago
(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.



(Reporter)

Comment 117

12 years ago
(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?
(Assignee)

Updated

11 years ago
Duplicate of this bug: 425715
(Assignee)

Comment 119

11 years ago
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
Flags: wanted1.9.0.x+
(Assignee)

Comment 121

11 years ago
Created attachment 315700 [details] [diff] [review]
patch v1

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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attachment #315700 - Flags: superreview?(roc)
Attachment #315700 - Flags: review?(joshmoz)
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.
(Assignee)

Comment 123

11 years ago
Created attachment 315901 [details] [diff] [review]
patch v2

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. :-)
Attachment #315700 - Attachment is obsolete: true
Attachment #315901 - Flags: superreview?(roc)
Attachment #315901 - Flags: review?(joshmoz)
Attachment #315700 - Flags: superreview?(roc)
Attachment #315700 - Flags: review?(joshmoz)
Comment on attachment 315901 [details] [diff] [review]
patch v2

Looks good to me.
Attachment #315901 - Flags: superreview?(roc) → superreview+

Updated

11 years ago
Attachment #315901 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 125

11 years ago
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.
Attachment #315901 - Flags: approval1.9?
(Assignee)

Updated

11 years ago
Summary: Mozilla mistimes changing Java applet visibility when switching tabs → Mozilla mistimes changing QuickDraw plugin visibility when switching tabs
(Reporter)

Comment 126

11 years ago
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.
(Assignee)

Comment 128

11 years ago
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?
(Reporter)

Comment 129

11 years ago
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.
(Assignee)

Comment 130

11 years ago
(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
Attachment #315901 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Created attachment 317412 [details] [diff] [review]
unbitrotted patch v2

Fix a minor merge conflict.
Attachment #315901 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.