456473 - Mac Plug-ins that use frameworks cause crashes on Firefox Quit

Status: RESOLVED WONTFIX

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

Description

[Rudi Sherry]

Attachment: Added a notification when Cocoa Firefox exits

Our NSAPI plug-in needs a way of detecting when Firefox is about to Quit.

We have a mac NSAPI plug-in that loads frameworks.  On Mac OS X, frameworks cannot be unloaded except at process exit time (in which case their static destructor fire).  This means our plug-in must stay loaded until Firefox quits.

Unfortunately, Firefox calls NPP_Shutdown() for our plug-in when the document it's looking at is closed, and we don't get any more calls from Firefox; when it Quits, we crash.

Firefox does not call [NSApp terminate:], so we have no "hook" for finding when it quits.

Solution (for patch which I will attach when I figure out how to "create" a patch) is to add an explicit call to NSNotificationCenter that appWillTerminate (in the case that it wasn't already sent) when the app is about to exit.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

As best I can tell, Mark hasn't had time to work on Mozilla.org stuff
for quite a while.  It might be better to ask me to review this (since
now most Cocoa appshell work is being done by me).

It'll probably be a while before I can get to this, though.

Mark, if you _would_ like to work on this, you're more than welcome to
:-)

Comment 2

[Rudi Sherry]

Comment on attachment 339870 [details] [diff] [review]
Added a notification when Cocoa Firefox exits

Changed reviewer to Steven Michaud at his request.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 339870 [details] [diff] [review]
Added a notification when Cocoa Firefox exits

Sorry to take so long with this ... but I was very busy in the rush to
get beta2 out the door.

Also sorry that I have to reject this patch.  But (as you'll see)
sending an NSApplicationWillTerminateNotification isn't quite as
simple as it first appears.  And I think I have a workaround that will
deal with your problem.

Your patch sends an NSApplicationWillTerminateNotification (on tearing
down the app shell) if none has yet been sent (by the OS (as a result
of choosing Quit from the Dock menu) or by an embedder (like Camino)).
But NSApplicationWillTerminateNotification is part of a sequence of
notifications, preceeded by NSApplicationShouldTerminateNotification.
And registering for NSApplicationWillTerminateNotification allows you
to (optionally) stop the application from terminating at all.

So if we wanted to send NSApplicationWillTerminateNotification from
the browser, we'd also need to send
NSApplicationShouldTerminateNotification beforehand, and allow
whoever'd registered for the second notification a chance to stop the
browser from terminating.  Otherwise we'd risk confusing embedders,
plugins or extensions that wanted to use these notifications correctly
(as Apple has documented them).  Moreover we couldn't do this from
nsAppShell::Exit() -- by which time the browser's decision to quit is
irrevocable.

If we end up having to send a notification here, I think it should be
some kind of new, unique, notification -- in effect an extention to
the NPAPI.  Or maybe we could document NP_Shutdown() more precisely,
and make it illegal to call it until the plugin is actually about to
be unloaded (Safari seems to do this, though Firefox (going back at
least to FF 2) doesn't).

But I'm not sure any browser-side changes are required to resolve your
specific problem (knowing when to unload the frameworks that your
plugin loads).  For example, it's possible to create a class like the
following:

class loadHandler
{
public:
  loadHandler::loadHandler() { [do stuff while plugin loads] }
  loadHandler::~loadHandler() { [do stuff while plugin unloads] }
};

and invoke it as follows, in code outside any method:

loadHandler handler = loadHandler();

My testing plugin from bug 441880 (its latest version) has a working
example of this.

Please try out this kind of solution and let us know your results.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> And registering for NSApplicationWillTerminateNotification allows you
> to (optionally) stop the application from terminating at all.

Oops.  This should be:

And registering for NSApplicationShouldTerminateNotification allows you
to (optionally) stop the application from terminating at all.

Comment 5

[Rudi Sherry]

I understand your loadHandler solution, but I believe it's problematic for our situation. When a process is being exited, the framework/library unload order is nondeterministic, and our plug-in loads about thirty other frameworks and libraries.  Thus, if our plug-in is unloaded after some of the other libraries are unloaded, then ~loadHandler() will call into code that's already gone (we have seen this happen, and we've gone back and forth with Apple on this).

How about a firefox-specific notification, then?  I'll create on and submit the patch for your review.

Comment 6

[Rudi Sherry]

Attachment: Sends a firefox-specific termination notification when the nsAppShell runloop is about to stop.

As per Steve Michaud's comment, changed the notification to be app-specific named "GeckoAppShellWithTerminateNotification" so there aren't any pre-formed specifications about the ordering and/or existence of other notifications.

It would have been nice to do the notification via an automatically-called static-destructor but that doesn't work for the reason I've stated.

Comment 7

[Rudi Sherry]

Sorry, mistyped; that's "GeckoAppShellWillTerminate", not "GeckoAppShellWithTerminate".

Comment 8

[Wayne Mery (:wsmwk)]

rudi, is patch ready for review?

Comment 9

[Rudi Sherry]

Yes, it is ready, thanks.  Not sure if I'm supposed to set some review flag for this.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 363773 [details] [diff] [review]
Sends a firefox-specific termination notification when the nsAppShell runloop is about to stop.

I'm very unenthusastic about this kind of solution.

Host-plugin communication of this sort should really be part of a
standard API, like the NPAPI.

If an existing NPAPI call (like NPP_Shutdown) doesn't work according
to spec, then that's a bug and should be dealt with as such.  If
nothing in the NPAPI does what you need, then maybe the NPAPI should
be extended.

There's a public mailing list for the discussion of extensions and
changes to the NPAPI, which is currently quite active
(https://mail.mozilla.org/listinfo/plugin-futures).  I'd suggest
making a proposal there.

And by the way, sorry for the long delay.  I've been very busy with
other stuff, and this fell through the cracks.

Comment 11

[Rudi Sherry]

You make a good point and I will propose something (I do participate on that mailing list, BTW).

Comment 12

[Rudi Sherry]

Maybe we can extend the NPP stuff, but I want you to understand why I would do it the way I suggested.  To sum it up:

Part I

Our plug-in needs to stay loaded after the last document is closed because we have very common workflows that link from one doc (an FDF) to another (a PDF) and we save information with the FDF so it can be used in the PDF.  If we unloaded when the FDF was closed (which it is, by Firefox, as part of getting the PDF), we would lose that information.

This is often sensitive information that should not be saved to disk so we don't want to create a persistent store for it.

Thus we have to stay loaded after the NPP_Shutdown call.

Part II

Our plug-in uses a (potentially) large number of frameworks and has an orderly shutdown sequence assuming we make a certain termination call.  If the frameworks get unloaded without making this call, and the unload order is not just exactly right, the interdependencies of these frameworks will cause a crash.  Reader is so large and complex, re-architecting this just so we don't have to make the termination call is not feasible.

Part III

An idiosyncrasy of Mac OS X, Objective-C and Framework-loading means that terminating an application results in the frameworks being unloaded in essentially a random order.

...so we have to stay loaded past NPP_Shutdown, we have to know when we're going to unload, and we can't depend on application termination to do the right thing.

Thus we're asking for a specific notification.

There may be a possible alternative: having NP_Shutdown return false meaning "I'm not unloading", and add NP_Terminate.  Then Gecko will have to know that it had a rogue plug-in and call NP_Terminate when it's quitting.  That would work for us but I think you'd be even less enthusiastic about it.

Comment 13

[Benjamin Smedberg]

There is no function NPP_Shutdown, although there is a function NPP_Destroy. There is only NP_Shutdown.

I am very confused by this bug. We don't unload plugins at all, currently. We call NP_Shutdown when the browser is shutting down, and that's that. We have discussed potentially unloading out-of-process plugins after a period of inactivity (say, 5 minutes), but that is currently not implemented. 

As far as I can tell, this bug is completely invalid.

Comment 14

[Rudi Sherry]

It is indeed a confusing issue from the point of Firefox coding, since the problem is due  just to the plug-in architecture (which can't be changed) and idiosyncrasies in the OS ... but Firefox has to participate in the solution.

The plug-in is automatically unloaded by the OS when the Firefox exits... but without warning that the application is about to exit and the unloading will commence, the plug-in will crash Firefox due to the interdependency of frameworks and the OS/Objective-C constraints.  What we're asking for is some sort of notification -- allowing the plug-in to gracefully terminate itself -- just before Firefox is about to exit, so there is no crash.

Comment 15

[Benjamin Smedberg]

Are you saying that we don't ever fire NP_Shutdown? That would be a bug. But you should expect that the browser will be shutting down soon after you receive NP_Shutdown.

Comment 16

[Josh Aas]

We didn't always keep plugins loaded when their last instance was destroyed. I forget in what version of Firefox we changed that. Firefox 4 always keeps plugins loaded until shutdown though, I believe.