Closed Bug 68759 Opened 24 years ago Closed 23 years ago

Call SetWindow() with NULL correctly for ALL plugins

Categories

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

Product:
Core Graveyard
Component:
Plug-ins
Platform:
PowerPC
All
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED INVALID
Milestone:
mozilla0.8.1

People

(Reporter: grandma, Assigned: peterlubczynski-bugs)

References

(
URL
)

Details

(Keywords: crash, shockwave)

Attachments

(3 files)

patch to let NULL windows through in SetWindow()
539 bytes, patch
Details | Diff | Splinter Review
better patch, excludes UNIX because of bug 37477
664 bytes, patch
Details | Diff | Splinter Review
set window->window to null before call to SetWindow()
2.90 KB, patch
Details | Diff | Splinter Review 
This has happened with every shockwave movie I have tried it from.

My first theory would be that you are invoking methods on the plugin after 
unloading it, but I haven't substantiated that yet.
Shockwave cache's the last window from NPP_SetWindow(), and it, amoung other 
things, may use SetPalette() on the window. Many browsers call NPP_SetWindow
(instance, NULL) before NPP_Destroy(), and during NPP_SetWindow(instance, NULL) 
we dispose of any data we associated with the window. If Shockwave receive 
NPP_Destroy() without first getting NPP_SetWindow(instance, NULL), then 
Shockwave will dispose of associated data as a pre-cursor to destroying the 
shockwave instance. One part of this is to restore the palette of the window 
with SetPalette(window, NULL, TRUE); (mac toolbox call).

I would speculate that NS6.01 is disposing of the macintosh window BEFORE 
invoking NPP_Destroy(), so our toolbox calls using the cached WindowPtr are 
crashing.

Peter, is this a dup of what you are currently working on?
I discovered this while trying to look at Tank Wars crashes when loading, but 
this is a crash when you quit the application with shockwave playing, so no, it 
is unrelated. I couldn't find other crash when quitting bugs, but if you do, 
don't close this one as it exactly describes what netscape has done to cause 
Shockwave to crash (disposed of the window without first calling NPP_SetWindow
() will NULL to clear Shockwave's reference to it).

Andrei,

No, this is a crash which has been lurking for some time I think. It happens 
with other plugins on other platforms as well but it's difficult to reproduce. 
I'll take a closer look.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: crash
-->peterl
Assignee: av → peterlubczynski
Status: ASSIGNED → NEW
God, I feel really dumb. We were returning from SetWindow() if we were passed a 
NULL window; but of course we can't do that, we must send that to the plugin.

It's so simple and I think it may fix other bugs as well. I need to do some 
regression testing first though.
Status: NEW → ASSIGNED
Peter, there was a reason for doing that. If I remember correctly some plugins 
didn't like it (crash). But if not doing it also causes problems AND it is not 
verboten by the spec, we should probably do it.
CVS Blame points to pavlov making that change in version 1.30. cc:ing him. The 
log says it was for UNIX plugins for bug 37477 for crashing on flash. I'll 
#IFDEF it out for UNIX to be safe. That should do it? New patch on it's way.

So far, it has passed my tests on Mac and Windows. Even prevents the crash in 
bug 58128. I recall someone at Real mentioning a similar problem. cc:ing nhart
Priority: -- → P1
Target Milestone: --- → mozilla0.9
No, this is not pavlov. I just pulled version 1.1 and it's already there:

NS_IMETHODIMP ns4xPluginInstance::SetWindow(nsPluginWindow* window)
{
    // XXX 4.x plugins don't want a SetWindow(NULL).

    if (window == NULL)
        return NS_OK;

michaelp wrote the initial version, and this happened before I joined the group. 
By the way, his comment is still there. I don't know what we should do about it 
but originally this was definitely not Unix related.
I don't know then, what should we do?

http://devedge.netscape.com/docs/manuals/communicator/plugin/pgfns.htm#1079816
Quoting the old 4.x Plugin API Guide for NPP_SetWindow():

Communicator calls NPP_SetWindow after creating the instance to allow drawing to 
begin. Subsequent calls to
NPP_SetWindow indicate changes in size or position; these calls pass the same 
NPWindow object each time, but with
different values. If the window handle is set to null, the window is destroyed. 
In this case, the plug-in must not perform
any additional graphics operations on the window and should free any associated 
resources. 

Seems to me that the API says it should be this way, and for all platforms.
Isn't 'window handle' in this qiote meaning NPWindow->window rather than 
NPWindow?
Blocks: 62788
This is very ambiguous.

Eric, can you clear up?
removing pavlov from the cc list

The word 'handle' is used in comments in npapi.h which has been publicly 
distributed with the Plugin SDK and refers to NPWindow->window. I think this 
could've become a source of confusion: some people took 'If the window handle is 
set to null' in the doc as NPWindow while others -- as NPWindow->window.

Given that the spec does not specifically forbid the structure pointer be null, 
we can do the following:

1. Remove this check if this really helps us to solve problems with 'big' 
plugins AND does not break any of them.

2. Decide what 'handle' in the spec means. (I incline to NPWindow->window, and 
my further comment assume that.)

3. Make sure in all _our_ sample code we do this check in implementation of 
NPP_SetWindow or nsIPluginInstance::SetWindow

4. Make sure in all _our_ sample code we use 
NPWindow(_or_nsPluginWindow)->window for null purposes. This is important 
because plugin developers use our sample code as a starting point, so nobody has 
a temptation to use the null pointer to the struct itself as a sign of what the 
spec says the null handler is a sign of.

5. Make adjustment to the spec wording so it does not confuse anyone
FYI, the Flash plugin for windows and mac returns NPERR_GENERIC_ERROR is window 
(the NPWindow*) is NULL.

Entry code for the Windows Shockwave plugin dereferences window, so passing 
NULL would cause it to crash. The Macintosh Shockwave plugin treats window == 
NULL and window->window == NULL pretty much equivalently.

Seems like window->window == NULL might be the way to go.
So Shockwave (does it mean Director?) is going to crash with the proposed patch. 
Peter, would you please summarize what is going to be fixed with this patch, so 
we couls weigh the options to make a decision?
Peter,

I'm confused. In your comments above, I think I understand that you say 
Shockwave needs a NPP_SetWindow(instance, NULL) (in this case NPWindow* being 
NULL) so it can dispose of data before the destroy call. But now it's really 
window->window that needs to be NULL? I confused.

So, to be clear, what should be acceptable to pass to Shockwave and othe 
plugins. Do plugin vendors except a NULL NPWindow* or a NULL window->window? 
Which one should NOT be passed to the plugin? Which one should? It's an easy 
enough fix either way.
In answer to av@netscape.com, the Macintosh browser crashes if you quit the 
browser while shockwave is playing, because you are destroying the window 
without first telling us to remove all references to it (by calling 
NPP_SetWindow() with a NULL window handle).

In answer to Peter Lubczynski, I was trying to say you should pass in a valid 
NPWindow *window, but have window->window = NULL (for bonus points, I'd assign 
the window extents to be zero as well). This will work with Flash and 
Shockwave, for both Windows and Macintosh. NPWindow *window = NULL will cause 
Windows Shockwave to crash, and it will cause Windows and Macintosh Flash to 
bail early, without actually doing anything to remove it's references to the 
native window.
This happens in RealPlayer too.  To prevent the crash we have taken the highly 
ugly approach of trapping DestroyWindow() to detect when our window goes away-- 
at which point we clean our controls up.

The whole problem is that the window is destroyed before the plugin is 
destroyed-- this totally wreaks havoc on RealPlayer and apparently Shockwave 
too.


Okay, what about this solution:

Everywhere in the code where we call SetWindow(NULL) (and I think it's done a 
few times), replace that by:

GetWindow(window);
window->window=NULL;
SetWindow(window);

And perhpas get some bonus points with Macromedia by checking for this NULL 
window->window in SetWindow() and setting the window extents to be zero as well.

Would that please everyone? Is there anything else that needs to be done with 
this issue?
Hmmm... I'm not sure that would help with the case where DisposeWindow() is 
called before the plugin is destroyed.  That is what is causing Real to crash.  
Perhaps if you were to call SetWindow(NULL) before DisposeWindow...
Forgive me for being ignorant, but where is DisposeWindow()? Do you mean 
NPP_Destroy()?
When I say DisposeWindow I am referring to the native MacOS call.  It seems 
that if you navigate to a new page, then our plugin is destroyed fine.  
However, if you close the page, then DisposeWindow() is called, which destroys 
all of our plugins' controls-- and this happens *before* our plugin is 
destroyed.  So, we never have a chance to clean up and then crash when we try 
to access our invalid controls that were destroyed by the browser.

I had assumed that this is the same problem Shockwave is experiencing.  
Basically, in order for plugins to work like they did in NS 4.7, they'll have 
to be destroyed before the browser destroys the window in which they are 
embedded.


Oh, ok, I understand. I think the SetWindow() call is usually called 
before/after Stop() in most places. Correct me if I'm wrong, but but I think 
DisposeWindow() is called much later after Stop() [somewhere in the widget 
system] so the proposed solution should work for you and fix bug 62788 as well?
Unfortunately that is not the case.  If it were then we could cleanup our 
windows in Stop().  I believe that the nsMacWindow widget is getting destroyed 
before Stop() or Destroy() is called.  (I tried this by calling our 
cleanup/destruction code in our instance's Stop() and it still didn't work.)
Is this only happening upon closing the browser but works okay for navigation to 
another page? If so, the culprit may be a missing inst->Destroy() call in 
nsObjectFrame::Destroy(). If not, I can't figure out who's destroying the window 
 as the widget associated with it is destroyed with the object frame. Got a 
stack trace?
I am adding Ed and Sean as they were involved in shutdown/destruction 
discussions earlier and might be interested too.

Guys, what do you think?
Ok, I found all the SetWindow() calls and instead of passing in a null NPWindow, 
I set NPWindow->window to NULL then pass that. 

Results: Windows, all is good <figures>
However, looks like it works for Shockwave but crashes Flash on the Mac.

Time to go home, will investigate more later.
The patch looks like things should really go. But I still want to hear from our 
Java plugin folks. They might expect window == null.
Don't know that I can add much to the discussion.  Our 4x plugin was built to 
handle either a NULL NPWindow or a NULL window member of NPWindow.  Our XPCOM 
plugin is the same.  It looks like this problem specifically affects 4x plugins 
running in mozilla since 4x plugins are currently handled differently than 
xpcom plugins.  The last patch looks like the right thing to do.

But the window->window=NULL patch crashes Flash on the Mac??? 

The window=NULL doesn't crash anything, even Shockwave. So I'm confused.

Peter/Sean/Nick, can you provide some insight?
If it's mac only, window=NULL should be fine. Windows Shockwave will crash for 
window=NULL. Mac Shockwave will remove it's reference to the window. Mac Flash 
will return an error code without doing anything, but given they aren't 
crashing with NPP_Destroy() called after the window has been disposed of, I 
think they are fine either way.
Brian, do you know how 4.x did this?
Going back to the SDK for a minute...
If you look at the "Simple" plugin sample you will note that the comment above 
the NPP_SetWindow implementation says "...If either window or window->window are 
NULL, the plug-in must not perform..." thus it is implied that either can be 
null.
If you look at the "PlatformSetWindow" implementations you will notice that 
1) UNIX will crash if either pointer is NULL,
2) the PC will work if either pointer is NULL, and 
3) the Mac will work if the NPWindow* is NULL, but will crash if window->window 
is NULL.

All in all, not a stellar example of sample source code. What seems to be saving 
us, as far as I can tell, is that in 4.x NPP_SetWindow never gets called with 
either parameter set to NULL.

Scattering some breakpoints in 4.x appears to show that NPP_Destroy is called 
without NPP_SetWindow ever being called, either before or after the destruction. 
It seems to me that if you call NPP_SetWindow with either pointer set to NULL you 
are probably going to break some existing legacy plugin(s).

Oh...well now I see what all the confusion is about. There is no XP way to 
implement this. I'll re-work the patch to follow the guidelines of 4.x if that 
is ok with everyone?
Do you think we should continue to contradict to our own spec? Maybe so. Not 
breaking existing plugins is a strong point, but I would really like to hear 
from OJI people, as they developed their plugin based on the old API but at the 
same time on the new codebase. The code in the destruction of object frame was 
written with this Java plugin in mind.
Perhaps a different direction is in order? Call NPP_Destroy() before disposing 
of the window, so if the plugin uses the window during NPP_Destroy() handling, 
it doesn't crash.
Peter, is it possible to fix this in such a way that NPP_SetWindow never gets
called with NULL on a legacy plugin while still passing it to an XPCOM plugin?

Others, if this is possible, is it desirable?

I agree with Andrei that we need to set a rule, fix the samples, and stick with
it. The thorny issue is, of course, trying not to break all existing plugins.

Regarding the OJI plugins. I don't know what assumptions are made on the PC &
Unix plugins. I looked at the MRJ source and it follows the assumptions I
spelled out above for the Mac, i.e. it will work with the NPWindow*
(nsPluginWindow* in this case) set to NULL, but will crash if window->window is
NULL.
There needs to be some consensus on this issue.

I think we can tell the difference between legacy and XPCOM, Andrei?

Changing summary from: Browser crashes if you quit while shockwave movie is 
playing
Keywords: 4xp, flash, shockwave
Summary: Browser crashes if you quit while shockwave movie is playing
Whiteboard: Call SetWindow() with NULL correctly for ALL plugins
I second Peter Grandmaison's comments:

"Call NPP_Destroy() before disposing 
of the window, so if the plugin uses the window during NPP_Destroy() handling, 
it doesn't crash."

This would be a BIG help for RealNetworks.  Currently our plugin expects 
NPP_Destroy() to happen before MacOS' DisposeWindow() is called.  This is how 
it happens in NS4.x and we would like it to continue to behave this way in NS6.

The failure, as I see it, in the API is that calling DisposeWindow() before 
destroying the plugin violates its "sovereignty" if you will pardon the 
expression.  DisposeWindow() destroys all the controls in the window-- 
including ones created by our plugin.  I believe that if a plugin creates 
controls it should be the one to destroy them.

Oops... I should clarify my previous comment.  We expect NPP_Destroy() *or* 
nsIPluginInstance::Destroy() before DisposeWindow().

SPAM: oops
Summary: Call SetWindow() with NULL correctly for ALL plugins
Whiteboard: Call SetWindow() with NULL correctly for ALL plugins
Peter: there is a flag in nsPluginTag NS_PLUGIN_FLAG_OLDSCHOOL. Is it of help?
Beatnik expects that SetWindow will not be called on a destroyed plugin 
instance (Mac or Win).  To ensure this, in our implementation of 
nsIPluginInstance::GetValue(), we return false for the 
nsPluginInstanceVariable_CallSetWindowAfterDestroyBool variable.
Isn't bug 62788 really about the Destroy() isssue?

To fix this bug, we should perform as Brian says, neither should be null, but 
only in the 4.x case:
1) UNIX will crash if either pointer is NULL,
2) the PC will work if either pointer is NULL, and 
3) the Mac will work if the NPWindow* is NULL, but will crash if window->window 
is NULL.

We should also obey nsIPluginInstance::GetValue() as sean says.
Ok, trying to regroup here. It appears that the origin of this bug stems from
the fact that MacroMedia was expecting all plugins (legacy and XPCOM) to get a
SetWindow(NULL) call during the termination process. Their reasoning being that
that is what the documentation says happens.

Legacy plugins are not receiving this call. This is due to the fact that
ns4xPluginInstance::SetWindow() does not pass on NULL window pointers because,
as the comment says, "4.x plugins don't want a SetWindow(NULL)". This is
probably due to the fact that someone at some point figured out that this caused
crashes in 4.x plugins.

Currently we are passing NULL to XPCOM plugins via
nsIPluginInstance::SetWindow() and overriding that functionality in
ns4xPluginInstance::SetWindow(). It would appear that, in order to preserve
backward functionality, we must continue to do this. Thus it seems to me that no
patch is required here.

It seems to me that this has become more of an issue regarding the calling of
Destroy() which, as Peter noted, is a different bug.


Most of the problem seems to be in nsObjectFrame::Destroy():

        if (doCallSetWindowAfterDestroy) {
          inst->Stop();
          inst->Destroy();
          inst->SetWindow(nsnull);
        }
        else {
          inst->SetWindow(nsnull);
          inst->Stop();
          inst->Destroy();
        }
      }
      else {
        inst->SetWindow(nsnull);
        inst->Stop();

Inside Stop() is where NPP_Destroy is called. Is there a particular reason 
(perhpas Java?) why these are they way they are? It's like this in a few other 
places as well.
Yes, this was added because of problems with the Java plugin, which for some 
reason requires different sequence of calls. That's why I constantly assert on 
hearing from those guys. Ed?
The code I pasted is irrelevant anyway because in current code, remember we 
return from SetWindow(nsnull) right away so Destroy() SHOULD always be being 
called.

In what case is it not? What's the stack?
Hi AV, we brought this up at our meeting yesterday.  I want to give Stanley 
some time to reply, since he's the authority on the impact of SetWindow() on 
the Java Plugin.
Ok, so what needs to be done to fix this bug? Should I replicate the behavior 
found by Brian in 4.x plugins and leave XPCOM the way it is? What do the Java 
folks say?

Incidently, the orriginal problem of this bug was that Shockwave was crashing on 
exit on the Mac while playing. This is not happening to me anymore. There IS a 
type 12 error, but that's caused by an ASSERT holding on to the ServiceManager. 
Running in the debugger doesn't crash. Will try an optimized build.
> "Call NPP_Destroy() before disposing of the window, so if the plugin 
> uses the window during NPP_Destroy() handling, it doesn't crash."
> 
> This would be a BIG help for RealNetworks.  Currently our plugin expects 
> NPP_Destroy() to happen before MacOS' DisposeWindow() is called.  This is 
> how it happens in NS4.x and we would like it to continue to behave this way
> in NS6.
>
> The failure, as I see it, in the API is that calling DisposeWindow() before
> destroying the plugin violates its "sovereignty" if you will pardon the 
> expression.  DisposeWindow() destroys all the controls in the window-- 
> including ones created by our plugin.  I believe that if a plugin creates 
> controls it should be the one to destroy them.

     We are having the same problem in Java Plug-in as well. Apparently, the 
native window is destroyed before NPP_Destroy() or nsIPluginInstance::Destroy() 
is called, and it may crash the plug-in. The solution we had is to hack the 
window message loop to do cleanup if the window is disposed before NPP_Destroy
(), but that also had some drawbacks. It will be nice if this bug get fixed.

> if (doCallSetWindowAfterDestroy) {
>          inst->Stop();
>          inst->Destroy();
>          inst->SetWindow(nsnull);
>        }
>        else {
>          inst->SetWindow(nsnull);
>          inst->Stop();
>          inst->Destroy();
>        }
>      }
>      else {
>        inst->SetWindow(nsnull);
>        inst->Stop();

     Please don't change the calling sequence because it will break the Java 
Plug-in. However, you may change the SetWindow(nsnull) call to SetWindow
(window) where window->window is NULL.

So after setting some breakpoints and trying to reproduce this I'm somewhat
confused. An lxr search for DisposeWindow shows that the only place we actually
call DisposeWindow is in the nsMacWindow destructor. This is only called when
the application quits... long after any calls to SetWindow() and Destroy() have
been done.

I have tried loading pages with plug-ins and then loading a different page in
place of it. I have also tried loading pages with plug-ins and the quitting the
application. I did these tests with both legacy and XPCOM plug-ins. In no case
was DisposeWindow ever called before Destroy().

Can somebody who is experiencing this crash post a stack crawl so I can better
understand what is going on here?
Are we talking about the same thing?  This behavior is in NS6.0 & NS6.01.  Are 
you not seeing this in Mozilla?  Perhaps it has been fixed there, but not in 
Netscape.

This behavior should be pretty apparent if you just stick a plugin on a page, 
set a breakpoint in Destroy(), SetWindow(), check to see if the window is valid 
in either of those functions and then close the browser window.

In our plugin we use the patch manager to trap calls to DisposeWindow.  When we 
see our plugin's window being destroyed we internally call Destroy() before 
letting the original DisposeWindow() do its processing.  This works for now, 
but it has the following problems:

1) it can interfere with anyone else that patches DisposeWindow
2) we can't safely remove the patch (ie: if someone patches it after we do and 
we restore the original DisposeWindow then their patch is gone & vice versa.)
3) it won't work on OSX.


Can anyone still experiencing this bug and bug 62788 please try with a latest 
nighly from mozilla.org. I've also looked through the trunk, and like Brian I 
see his results. Netscape 6 and 6.01 come from a very old branch. 

If it is still happening, please post a stack trace to DisposeWindow. Thanks!
I can not reproduce on either the Mozilla or Netscape 6 using a current build.
Apparently whatever is causing this problem has changed since the 6.0/6.01
branch. This is good news going forward, but doesn't help with what is currently
available to the user.
Moving to mozilla0.8
Target Milestone: mozilla0.9 → mozilla0.8.1
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → INVALID
I can not reproduce this and it looks like it's good the way it is. I say mark 
INVALID unless any objections?
spam : marking invalid bugs 'verif'. reopen if u disagree, reporter!
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: