339051 - Recent Carbon trunk builds post kEventAppQuit after loading Java applet

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Starting with the 2005-05-11 nightly, trunk builds of Firefox for OS X post a
Carbon event of class kEventClassApplication and kind kEventAppQuit
immediately after having loaded a Java applet.  All recent SeaMonkey trunk
nightlies also do this (they only started being available as of 2006-05-16).

As a result, the Java Embedding Plugin thinks that the browser is about to
quit, and unloads all its event handlers (in addition to destroying all
existing Java applets).  But the browser doesn't quit.  And because all of the
JEP's handlers have been unloaded, browser windows into which at least one
applet has been loaded no longer accept any mouse or keyboard events -- making
it seem that the browser is hung.  (In fact it isn't hung, because you can
still use its main menu.)

(Browser windows not accepting key or mouse events is due to a design flaw in
Apple's Cocoa-Carbon interface -- after Apple's Carbon event handlers for this
interface have been loaded on the "window" and "user focus" targets, they
swallow all events for those targets.  The Java Embedding Plugin's event
handlers normally work around this, allowing windows that contain Java applets
(or once contained them) to continue receiving mouse and keyboard events.  But
once the JEP's event handlers have been unloaded, they can no longer perform
this service.)

This bug (a regression) cooincides with the fix for bug 326273.  So I assume
it was caused (perhaps only indirectly) by one of the changes made there.

This bug was originally reported at bug 338685 (another regression caused by
the fix for bug 326273).  But this bug is a separate issue, so I'm now
reporting it separately.

I will attach logs of that show this bug in action.  I generated them by
adding a "label" (an empty routine) to a debugging copy of the Java Embedding
Plugin, running the browser (plus the JEP) inside gdb, "break"ing on my label,
and then entering "thread apply all bt" and "info sharedlibrary".  I did this
on both OS X 10.3.9 and 10.4.6, with recent trunk nightlies of both Firefox
and SeaMonkey.

It's possible that the kEventAppQuit event is also being posted after having
loaded other kinds of plugins.  I notice that my favorite Flash example
(http://www.rogerdean.com) doesn't display in recent Carbon-based trunk
nightlies.  But I haven't tried to test for this.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: appQuit bug in Minefield on OS X 10.4.6

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: appQuit bug in SeaMonkey on OS X 10.4.6

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: appQuit bug in Minefield on OS X 10.3.9

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Attachment: appQuit bug in SeaMonkey on OS X 10.3.9

Comment 5

[Mark Mentovai]

This is due to my use of QuitApplicationEventLoop, which I've already decided to stop using.

Comment 6

[Mark Mentovai]

Attachment: Patch v1

This is the Carbon version of the Cocoa patch in bug 338249.  Here's the story:

Because we need to support embedding and plugins and all sorts of other things that expect the world to behave a certain way, we really can't get away with ::RunApplicationEventLoop()/::QuitApplicationEventLoop() and [NSApp run]/[NSApp stop:] as I had initially hoped.  This means that there's a chance of being bitten in the butt if we don't run our event loop the same way the system would, or if the system implementations of the event loops start doing something that we don't do in the future.  That's a risk I was hoping to avoid, but it's one we're going to have to take.

For Carbon at least, there are some well-documented things that you need to provide for when switching from ::RunApplicationEventLoop() to a self-run ::ReceiveNextEvent loop, documented at http://developer.apple.com/documentation/Carbon/Conceptual/Carbon_Event_Manager/Tasks/chapter_3_section_12.html .  For one-time setup that RAEL would do, such as installing certain handlers, Apple suggests running the entire app on an event dispatched by RAEL, but because we just migrated from the WNE world, these things are already accounted for in our code.  We just need to hope that there aren't any surprises.

As for Cocoa, the loop's internals aren't really documented, but I've done what I can to match what the system does based on what I know of it and based on the GNUstep implementation.

Comment 7

[Mark Mentovai]

P.S. You'll note that although it's no longer necessary when running the loop ourselves, I've still got ProcessNextNativeEvent running in its own tight loop when aMayWait is PR_TRUE.  I've done this because I've heard that we still might not be leaving the native loop to process Gecko events in all cases where we should (bug 337550 comment 24), and I'd like to be able to catch those before making PNNE behave like other platforms (with the side-effect of masking many cases of dysfunctional ScheduleNativeEventCallback behavior).

Comment 8

[Darin Fisher]

Comment on attachment 223444 [details] [diff] [review]
Patch v1

>Index: mozilla/widget/src/mac/nsAppShell.cpp

> PRBool
> nsAppShell::ProcessNextNativeEvent(PRBool aMayWait)
> {
>+  PRBool wasRunningEventLoop = mRunningEventLoop;
> 
>+  mRunningEventLoop = aMayWait;
>+  EventTimeout waitUntil = kEventDurationNoWait;
>+  if (aMayWait)
>+    waitUntil = kEventDurationForever;
>+
>+  OSStatus err;
>+  EventRef carbonEvent;
>+  do {
>+    err = ::ReceiveNextEvent(0, nsnull, waitUntil, PR_TRUE, &carbonEvent);
>+    if (err == noErr) {
>       ::SendEventToEventTarget(carbonEvent, ::GetEventDispatcherTarget());
>       ::ReleaseEvent(carbonEvent);
>     }
>+  } while (mRunningEventLoop);
> 
>+  mRunningEventLoop = wasRunningEventLoop;
> 
>+  PRBool eventsInQueue = PR_FALSE;
>+  err = ::ReceiveNextEvent(0, nsnull, kEventDurationNoWait, PR_FALSE,
>+                           &carbonEvent);
>+  if (err == noErr)
>+    eventsInQueue = PR_TRUE;
>+
>+  return eventsInQueue;
> }

I don't believe this is implementing the ProcessNextNativeEvent
API correctly.  It should return once it has processed the next
native event, irregardless of whether or not aMayWait is true.
Looping on mRunningEventLoop seems wrong to me.

Also, it should just return true if an event was processed and
false otherwise.  I don't understand why it chooses to return
true only if another event is in the queue.

Do you need to call ReleaseEvent on the carbonEvent returned
from the last ReceiveNextEvent call?


> void
> nsAppShell::ProcessGeckoEvents(void* aInfo)
> {
>   nsAppShell* self = NS_STATIC_CAST(nsAppShell*, aInfo);
> 
>   if (self->mRunningEventLoop) {
>+    self->mRunningEventLoop = PR_FALSE;

If you break out of the event loop whenever you process a single event,
then you do not need to set this to false.  In fact, I think you should
be able to remove this member variable.

Comment 9

[Mark Mentovai]

(In reply to comment #8)
> I don't believe this is implementing the ProcessNextNativeEvent
> API correctly.  It should return once it has processed the next
> native event, irregardless of whether or not aMayWait is true.
> Looping on mRunningEventLoop seems wrong to me.

See comment 7.  I don't intend to leave it this way, and will file a bug on myself to remove it once I'm satisfied that ScheduleNativeEventCallback is always doing the right thing.

> Also, it should just return true if an event was processed and
> false otherwise.  I don't understand why it chooses to return
> true only if another event is in the queue.

Because I went back to an earlier rev - I'll fix that.

> Do you need to call ReleaseEvent on the carbonEvent returned
> from the last ReceiveNextEvent call?

No, because I'm not pulling that event from the queue and am not assuming ownership of it.

> If you break out of the event loop whenever you process a single event,
> then you do not need to set this to false.  In fact, I think you should
> be able to remove this member variable.

Right.  I plan for it to go away, at most, in a couple of weeks.

Same comments apply to 338249.  Would you like to see a new patch with the fixed-up return values?

Comment 10

[Darin Fisher]

Comment on attachment 223444 [details] [diff] [review]
Patch v1

OK, sorry... I should have read your previous comment.  I don't need to see another patch.

Comment 11

[Mark Mentovai]

Checked in with comment addressed.

Comment 12

[Mark Mentovai]

Followup bug is bug 339353.