395397 - Process Gecko events on demand in Cocoa widgets

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Attachment: Process Gecko events on demand using a CFRunLoopSource

At present, Minefield processes Gecko events in a tight loop (repeated
calls to NS_ProcessNextEvent() in nsBaseAppShell::Run()) on the main
thread.  But this tight loop can be "interrupted" by modal activity (a
"nested" event loop that temporarily does its own event processing).
And unless the nested event loop also processes Gecko events, Gecko
events will (temporarily) stop being processed.

Some of these nested event loops belong to the browser, so they
(presumably) can take responsibility for processing Gecko events as
needed.  But others belong to the OS, which knows nothing of Gecko
events.  Examples are "native" menus and context menus (only Camino
uses native context menus), plus the modal loop that runs while
someone is using a window's "resize region" (in the lower-right
corner) to resize it.  While these "native" event loops are running,
no Gecko events are processed.  (For more information see bug 356720.)

The current nsAppShell.mm contains code (in ProcessGeckoEvents() and
ScheduleNativeEventCallback()) that's supposed to get around this
problem by also processing Gecko events "on demand" (triggered from
nsBaseAppShell::OnDispatchedEvent()).  But (for obscure timing
reasons, about which more below) this code currently never processes
any Gecko events if we're also processing Gecko events in a tight loop
(in nsBaseAppShell::Run()).

My patch fixes this problem by no longer processing Gecko events in a
tight loop -- it _only_ processes Gecko events on demand.  As a
result, Gecko events get processed even during "native" modal event
loops (and bug 356720 stops happening).

Several other changes were needed to make processing Gecko events only
on demand via a CFRunLoopSource work properly -- they're noted in the
patch's comments.

Camino currently only processes Gecko events on demand (it doesn't
call nsAppShell::Run(), and doesn't process them in a tight loop).
But it uses a different method to "schedule" those events (not a
CFRunLoopSource), which still allows the processing of Gecko events to
stop during "native" modal event loops.  My patch's method fixes this
problem.

In principle, it'd be nice to be able to process Gecko events on
demand _and also_ in a tight loop.  But that would require changes to
cross-platform widget code (in nsBaseAppShell.cpp, particularly in
NativeEventCallback() and DoProcessNextNativeEvent()).

As far as I can tell, my patch doesn't cause any event starvation
(Gecko or native).

Note (quoting from a comment in my patch):

nsBaseAppShell::NativeEventCallback() doesn't actually process any
Gecko events if elsewhere we're also processing Gecko events in a
tight loop (as happens in nsBaseAppShell::Run()) -- in that case
ProcessGeckoEvents() is always called while ProcessNextNativeEvent()
is running (called from nsBaseAppShell::OnProcessNextEvent()) and
mProcessingNextNativeEvent is always true (which makes
NativeEventCallback() take an early out).

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I'm requesting that this block 1.9 because my patch fixes bug 356720,
which is already marked blocking1.9+.

Comment 2

[Josh Aas]

Comment on attachment 280078 [details] [diff] [review]
Process Gecko events on demand using a CFRunLoopSource

This doesn't compile for me.

mozilla/widget/src/cocoa/nsAppShell.mm: In static member function 'static void nsAppShell::ProcessGeckoEvents(void*)':
mozilla/widget/src/cocoa/nsAppShell.mm:228: error: expected primary-expression before '*' token
mozilla/widget/src/cocoa/nsAppShell.mm:228: error: expected primary-expression before ',' token
mozilla/widget/src/cocoa/nsAppShell.mm:228: error: 'NS_STATIC_CAST' was not declared in this scope

+  // No event loop is running yet (unless Camino is running).  Avoid

This is probably true for all embedding apps, so probably better to say "embedding apps" instead of Camino specifically.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Revised fix (change NS_STATIC_CAST to static_cast)

> This doesn't compile for me.

Odd, it compiled fine for me.

But I somehow let a deprecated NS_STATIC_CAST into my patch, which
might have been removed from all headers in the most recent code.
I've now replaced it with static_cast, which should work fine.

> This is probably true for all embedding apps, so probably better to
> say "embedding apps" instead of Camino specifically.

As far as I know, Camino's use of [NSApp run] is specific to Camino,
and that's what I was referring to.  So I'm not sure the event loop
would be running even in other OS X embedding apps.

But I will look into this further, and if need be change this comment.

Comment 4

[Josh Aas]

In addition to fixing 356720, this also fixes another major bug which if we don't have filed we should file. Without this patch, youtube videos pause when you open menus (from the native menu bar). With this patch, everything keeps working - the video does not stop.

In general this seems to be a big improvement for UI responsiveness, removing quite a few glitches.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> In addition to fixing 356720, this also fixes another major bug which if we
> don't have filed we should file. Without this patch, youtube videos pause when
> you open menus (from the native menu bar). With this patch, everything keeps
> working - the video does not stop.

Bug 391301 was filed about that problem, but it was duped to bug 356720.

Comment 6

[Josh Aas]

This fixes several major browser responsiveness regressions from Firefox 2, added keyword. I suggest we block on this but I want more input from other drivers before I mark blocking.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2 (possible performance tweak)

This revision adds a possible performance tweak to my patch.

I've been a little anxious about making ProcessNextNativeEvent()
always return PR_FALSE (which means that there (currently) aren't any
more native events to be processed) -- though I didn't see it in any
of my tests, there was the potential for this to cause some level of
native event starvation.

I've now changed ProcessNextNativeEvent() to return PR_FALSE if there
aren't any native events in the OS's event queue, and also if
ProcessNextNativeEvent() has previously returned PR_TRUE ten times in
a row.

In principle this makes native event starvation much less likely.  But
my tests don't tell me whether it makes any difference -- for better
or for worse.

Josh (or anyone else who's willing):  Please run your own performance
tests on this revised patch.  I'm inclined to keep it even if it
doesn't appear to make things any better.  But I should probably
revert to the previous version if if makes things worse.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Even with my patch, Flash videos still drop frames when resizing a
browser window in Minefield and Camino.

At first I thought I might be able to fix this by tweaking my patch.
But now I think it's either a problem with how Flash does video or a
separate problem with the browser (on OS X):

1) Flash animations (e.g. those at bug 356720's URL http://www.vg.no/)
   don't appear to be slowed down by resizing the browser window.

2) The Flash plugin (as I understand it) only receives events via the
   very old and primitive NPAPI (so for example it doesn't receive
   mouse-moved events except as "null events").  It's possible that
   the flow of NPAPI events to the Flash plugin is still
   slowed/interrupted while resizing the browser window.

Given sufficient time I might be able to come up with a solution to
this problem (which might or might not involve tweaking my patch yet
again).  But I suspect this won't be easy or quick.

Here's the Flash video I've been testing with:

http://www.thecuteproject.com/videos/2086/playing.meowzart/

I think this problem with Flash videos is relatively minor compared
with the other problems my patch does fix.  So even if I can't fix the
Flash videos problem right away (presuming that it's a browser bug), I
don't think it should block my patch landing.

Comment 9

[Josh Aas]

To clarify, previously (Gecko 1.8) Flash was totally blocked during window resizes, so though it might drop some frames during resize as outlined in comment #8, things are much better with this patch. It is not introducing a regression of any sort.

Comment 10

[Josh Aas]

I finished another round of testing today after fixing some methodology mistakes from yesterday (tip: don't test your own opt builds vs. nightly builds). With Steven's patch I see a 2-3% Tp improvement. This is awesome in addition to the UI responsiveness it adds.

Another interesting thing I noticed about the perf numbers with Steven's patch is that the results are much less erratic. Unpatched, the same Tp test run 10 times came up with results scattered within ~12% of each other (4600 vs. 5200 ms at the extremes). With Steven's patch results were within ~2.2% of each other (4600 vs. 4700 ms at the extremes).

Comment 11

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #7)

> I'm inclined to keep it even if it doesn't appear to make things any
> better.  But I should probably revert to the previous version if if
> makes things worse.

I've now used the same performance tests that Josh used (page load
tests originally written by Mark Mentovai) to compare my latest patch
(attachment 280473 [details] [diff] [review]) with my previous one (attachment 280281 [details] [diff] [review]).

These tests can be a bit erratic, and I had to remove a couple of
outliers.  But once that was done the results of my tests were within
a 2% range for both of my patches.  Furthermore, though my previous
patch did slightly better on my MacBook Pro, my latest patch did
better on my PowerPC Mac.  This difference is probably just noise, so
the performance of my two patches is (basically) the same on
Josh/Mark's tests.

So I'll stick with my current patch (attachment 280473 [details] [diff] [review]).

Comment 12

[Mark Mentovai]

Comment on attachment 280473 [details] [diff] [review]
Fix rev2 (possible performance tweak)

In constructor:

>+, mCFRunLoop(nsnull)
>+, mCFRunLoopSource(nsnull)

Should probably use NULL for system pointers (as opposed to Gecko ones).

In destructor:

>+  ::CFRunLoopRemoveSource(mCFRunLoop, mCFRunLoopSource,
>+                          kCFRunLoopCommonModes);

Check that mCFRunLoop and mCFRunLoopSource are not NULL.

In Init:

>+  mCFRunLoop = [[NSRunLoop currentRunLoop] getCFRunLoop];
>+  NS_ENSURE_STATE(mCFRunLoop);
>+  ::CFRetain(mCFRunLoop);

Who releases this?

>+  mCFRunLoopSource = ::CFRunLoopSourceCreate(kCFAllocatorDefault, 0, &context);

Who releases this?

In ProcessGeckoEvents:

>+  // Still needed to fix bug 343033.

Use a few words to describe the bug in addition to giving the bug number reference.

>+  NS_RELEASE(self);

Hmm...in ScheduleNativeEventCallback:

>+  NS_ADDREF_THIS();

The problem is that you can run through ScheduleNativeEventCallback multiple times, hitting the addref, but the CFRunLoopSource doesn't get any more signaled once it's been signaled once (until the run loop hits it and runs the perform target, ProcessGeckoEvents).  You might wind up addrefing the AppShell more times than you free it.

In WillTerminate:

>+  if (mTerminated)
>+    return;
>   mTerminated = PR_TRUE;
>+  // Ugly hack to stop _NSAutoreleaseNoPool errors on shutdown from Camino --
>+  // these seem to be triggered by our call here to NS_ProcessPendingEvents().
>+  [[NSAutoreleasePool alloc] init];
>+  // Calling [NSApp terminate:] causes (among other things) an
[...]

Could you put a blank line before each new comment section?  That makes it clear that the comment applies to the code that follows it.  The same applies elsewhere, such as in ScheduleNativeEventCallback.  (Also, in ScheduleNativeEventCallback, the comment should actually come before signalling the source and waking up the loop, not before the addref.)

RunningXULModal: could you move the definition of this function up and out of the way of the member methods?  Or move it down and give it a forward declaration at the top of the file.

>+    return PR_FALSE;
>+  nsCOMPtr<nsISimpleEnumerator> orderedWindowList;

Blank line between an early return and doing more work.  This pattern comes up elsewhere in this function, too.

>+  // Get an nsIWebBrowserChrome interface for the frontmost XUL window, then

Hmm, nsIWebBrowserChrome?  Is this function browser (Firefox)-specific?  Will it work with Thunderbird?  I'm hoping nsIWebBrowserChrome is just a misnomer, and it's really not browser-specific but common to XUL applications.

In ProcessNextNativeEvent:

>+#define HAD_MORE_EVENTS_COUNT_MAX 10

Why not make this |static const int kHadMoreEventsCountMax|?  Or, since you don't even need it outside of ProcessNextNativeEvent (and you even went to the trouble of #undefing it), why not just make it a const int inside the method?

>+  static PRUint32 hadMoreEventsCount = 0;

I'd prefer to see this as a member variable than a static variable.

>+    // just long enough to process it.  For some reason, using NSApp
>+    // nextEventMatchingMask to dequeue the event and NSApp sendEvent to

Put the Objective-C calls in [brackets] like you do elsewhere to make this clearer.  Same when you mention nextEventMatchingMask later in the comment.  Also, I'd favor using the entire method names with the colons and extra parameters.

>+    // bring up a context menu.)
>+    // Now that we're using [NSRunLoop acceptInputForMode:beforeDate:], it's

Break these paragraphs up with a blank // line.  :)

>+    PRBool runningModal = [NSApp _isRunningModal] || RunningXULModal();
>+    currentMode = [[NSRunLoop currentRunLoop] currentMode];
>+    if (!currentMode)
>+      currentMode = NSDefaultRunLoopMode;
>+    if (runningModal) {

Since you don't use runningModal until after you're done setting currentMode, don't declare or initialize runningModal until you've figured out currentMode.  You can also feel free to get rid of runningModal as a variable and stick the expression in your conditional, since you don't use its value elsewhere.

>+      if (aMayWait ||
>+          [NSApp nextEventMatchingMask:NSAnyEventMask
>+                             untilDate:nil
>+                                inMode:currentMode
>+                               dequeue:NO]) {
>+        [[NSRunLoop currentRunLoop] acceptInputForMode:currentMode
>+                                            beforeDate:waitUntil];
>+        eventProcessed = PR_TRUE;
>       }

Do we even need the outer conditional here?  If aMayWait is false, waitUntil is nil, so is it safe to always do acceptInputForMode as long as we're not running modal?

In Run:

>+  NS_ENSURE_STATE(!mStarted);
>+  if (mStarted)
>+    return NS_OK;
>+  mStarted = PR_TRUE;

Choose one or the other.  If mStarted is true, NS_ENSURE_STATE returns NS_ERROR_UNEXPECTED before the second check has a chance to return NS_OK.  The same comment applies in Exit.  I also saw that you used NS_ENSURE_STATE elsewhere, you should go back and make sure that it does what you intend.

Again, blank lines between early returns and doing more work, please.  Makes for a much easier read.

In Exit:

>+    return NS_OK;
>+  mTerminated = PR_TRUE;
>+  // Quoting from Apple's doc on the [NSApplication stop:] method (from their

And again, also, blank lines before a comment, so that the comment's application is clear.

>+  // So this code assumes that nsAppShell::Exit() will never be called from a
>+  // modal event loop.

Can you check that and at least add a debug-mode assertion?

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev3 (follow Mark's suggestions)

> Hmm...in ScheduleNativeEventCallback:
>
> >+  NS_ADDREF_THIS();
>
> The problem is that you can run through ScheduleNativeEventCallback
> multiple times [...]

Not so.  nsBaseAppShell.cpp uses the mNativeEventPending variable to
guarantee that ScheduleNativeEventCallback() is called (from
nsBaseAppShell::OnDispatchedEvent()) no more than once per call to
NativeEventCallback() (called from ProcessGeckoEvents()).

I've long wondered about the opposite case -- can ProcessGeckoEvents()
be called "spontaneously", without a call to
CFRunLoopSourceSignal(mCFRunLoopSource)?  I've had code to test for
this in the debugging version of my appshell patch for quite a long
time, and as best I can tell the answer is "no".  But Apple doesn't
document this anywhere (as far as I know).

Since only one nsIAppShell object exists at any one time, it might be
better to use a global weak variable (which could be AddRefed or
Released as needed).

What do you think?

> >+  // Get an nsIWebBrowserChrome interface for the frontmost XUL window, then
>
> Hmm, nsIWebBrowserChrome?  Is this function browser
> (Firefox)-specific?  Will it work with Thunderbird?

I'm pretty sure the nsIWebBrowserChrome interface isn't
Firefox-specific (or "browser"-specific).  nsXULWindow::GetInterface()
(in xpfe/appshell/src/nsXULWindow.cpp) lets you get an
nsIWebBrowserChrome interface on any nsXULWindow object.

So we're fine (I think) if Thunderbird uses XUL windows.  If it
doesn't, we may have a problem.

> >+      if (aMayWait ||
> >+          [NSApp nextEventMatchingMask:NSAnyEventMask
> >+                             untilDate:nil
> >+                                inMode:currentMode
> >+                               dequeue:NO]) {
> >+        [[NSRunLoop currentRunLoop] acceptInputForMode:currentMode
> >+                                            beforeDate:waitUntil];
> >+        eventProcessed = PR_TRUE;
> >       }
>
> Do we even need the outer conditional here?  If aMayWait is false,
> waitUntil is nil, so is it safe to always do acceptInputForMode as
> long as we're not running modal?

I'm not sure what you mean by "outer conditional" here (aMayWait or
the call to [NSApp nextEventMatchingMask:...]).  But in fact I do need
both of them -- in order to know how to set eventProcessed and to
avoid unnecessary (and possibly expensive) calls to [NSApp
nextEventMatchingMask:...] and/or acceptInputForMode.

If aMayWait is PR_TRUE, I can skip the (possibly expensive) call to
[NSApp nextEventMatchingMask:...], because I know acceptInputForMode
(when told to wait forever, as it is in this case) will always return
after having processed an event.

If aMayWait is PR_FALSE, I need the call to [NSApp
nextEventMatchingMask:...] for two purposes -- 1) to avoid a (possibly
expensive) call to acceptInputForMode if no event is waiting to be
processed, and 2) to know that acceptInputForMode will have processed
an event when it returns (instead of just timing out).

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Oops, I made a small change in the previous patch that goofed up "had
more events" counting in ProcessNextNativeEvent().  Another patch is
coming shortly.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev4 (fix "had more events" counting)

Comment 16

[Mark Mentovai]

Comment on attachment 280623 [details] [diff] [review]
Fix rev4 (fix "had more events" counting)

>Since only one nsIAppShell object exists at any one time, it might be
>better to use a global weak variable (which could be AddRefed or
>Released as needed).

Nah, I think it's better as-is.  I forgot that the atomic checks were in the base appshell, the addref/release on the appshell object itself is fine.

>+  static const int   kHadMoreEventsCountMax; // Used in ProcessNextNativeEvent()

This would have been OK as a file-scoped static const int or just a const int in the function that it's used.  There's nothing wrong with it here, but it doesn't really need this level of visibility.

If you do want to leave it here, it's OK to assign a value right here in the header.  C++ allows that for static const members of fundamental types.

Your call.

>+  // Quoting from Apple's doc on the [NSApplication stop:] method (from their
>+  // doc on the NSApplication class):  "If this method is invoked during a
>+  // modal event loop, it will break that loop but not the main event loop."
>+  // nsAppShell::Exit() shouldn't be called from a modal event loop.  So if
>+  // it is we complain about it (to users of debug builds) and call [NSApp
>+  // stop:] one extra time.

Can there be multiple nested modal loops?  If so, say that this will only send enough stops for one of them.  If not, ignore this comment.

Do you also want to assert/check that mStarted is PR_TRUE in Exit?

Looks really good, thanks for putting the work in on this.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev5 (follow Mark's further suggestions)

> >+  static const int   kHadMoreEventsCountMax; // Used in ProcessNextNativeEvent()
>
> This would have been OK as a file-scoped static const int or just a
> const int in the function that it's used.  There's nothing wrong
> with it here, but it doesn't really need this level of visibility.

Since mHadMoreEventsCount and kHadMoreEventsCountMax are used together
(in ProcessNextNativeEvent()), I wanted their definitions to be
together.  I've left them both in nsAppShell.h, but I followed your
suggestion to "initialize" (i.e. set) kHadMoreEventsCountMax where I
define it.

> Can there be multiple nested modal loops?

Apple's docs don't say (not surprising).  And the little other
evidence I could find is conflicting.  But just in case I now keep
calling [NSApp stop:] until [NSApp _isRunningModal] no longer returns
TRUE.

> Looks really good, thanks for putting the work in on this.

Thanks!  It _has_ been a lot of work, but it seems to have panned out.

Comment 18

[Mark Mentovai]

Comment on attachment 280748 [details] [diff] [review]
Fix rev5 (follow Mark's further suggestions)

>Apple's docs don't say (not surprising).  And the little other
>evidence I could find is conflicting.  But just in case I now keep
>calling [NSApp stop:] until [NSApp _isRunningModal] no longer returns
>TRUE.

OK, as long as you've tested this - just make sure that [NSApp _isRunningModal] doesn't return YES after you've issued [NSApp stop:nil] from inside a modal loop.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev6 (fix problem in nsAppShell::Exit())

> just make sure that [NSApp _isRunningModal] doesn't return YES after
> you've issued [NSApp stop:nil] from inside a modal loop

I hadn't tested it.  It was difficult to come up with a way to do so
... but it's a good thing that I did (and that you asked), because
[NSApp _isRunningModal] _does_ return YES just after a call to [NSApp
stop:] (even though it also does cause the modal loop to be broken).

So I've gone back to calling [NSApp stop:] one extra time if
nsAppShell::Exit() is called from a Cocoa-modal run loop.

If it's possible to nest Cocoa-modal run loops, and if someone calls
nsAppShell::Exit() from a multiply-nested Cocoa-modal run loop, bad
things will happen.  But whoever does this will deserve the bad
results, and there's no point going to great lengths to allow a really
bad programming mistake to have no bad consequences.

Comment 20

[Mark Mentovai]

Sounds good to me.  I guess the only other thing that could be done if we detect we're inside a modal event loop would be to send a stop and then queue up a new event that, when processed, does the same check and sends a stop to the parent event loop.  We shouldn't need to do that here unless proven otherwise.  This patch (including the comments) is completely satisfactory to me now.  Thanks again.

Comment 21

[Josh Aas]

I'm marking this bug as blocking1.9+ for now because it fixes bug 356720, which is a blocker. Should we reverse this decision we need to remove blocking status from 356720.

That said, I do think we should remove blocking status from either.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if ([NSApp _isRunningModal] || RunningXULModal()) {

Isn't there are more elegant way to do this? In particular, can't we test directly for whatever XUL-modal windows have done to event processing?

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> can't we test directly for whatever XUL-modal windows have done to
> event processing?

Are you asking if there's an OS-X-specific way to find out (in all
cases) whether we're running modal (including XUL-modal)?

If so, I'm quite sure the answer is "no".

I started testing only the results of [NSApp _isRunningModal], but I
found windows that were running XUL-modal for which this wasn't true
-- in particular the Quit dialog and the window that comes up when you
start Firefox/Minefield after it crashed.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Perhaps we should fix those latter cases, then?

> Are you asking if there's an OS-X-specific way to find out (in all
> cases) whether we're running modal (including XUL-modal)?

What I'm asking is that given XUL-modal windows are messing with event handling (requiring special casing here), is there way to detect that messing directly?

Comment 25

[Steven Michaud [:smichaud] (Retired)]

> is there way to detect that messing directly?

What do you mean by this?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know.

How about this: can you detect when we're running a nested Gecko event loop (i.e. not the thread's main event loop)? Wouldn't that be a better test here?

Comment 27

[Steven Michaud [:smichaud] (Retired)]

> can you detect when we're running a nested Gecko event loop
> (i.e. not the thread's main event loop)? Wouldn't that be a better
> test here?

There's no OS-specific way to do this.  And as far as I know there's
no Gecko-specific way either ... besides RunningXULModal().

And if there _is_ a Gecko-specific method to find out, it'd need to be
better than RunningXULModal().

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about adding a method to nsIThreadInternal?

I'm just worried that RunningXULModal looks really fragile.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #27)

Furthermore, even if I _had_ a way to detect event-loop nesting (as
opposed to running modal), I'm not sure it would fit the bill.

Since there isn't an OS-specific one, I haven't been able to test one.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

> I'm just worried that RunningXULModal looks really fragile.

It's true things would be better if there were a "published" API to
detect whether or not Gecko is running modal?  Do you think that could
be managed?

Comment 31

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #29)

> Furthermore, even if I _had_ a way to detect event-loop nesting (as
> opposed to running modal), I'm not sure it would fit the bill.
>
> Since there isn't an OS-specific one, I haven't been able to test
> one.

On reflection, I'm quite sure what I need is a way to detect whether
Gecko is running modal, _not_ a way to detect whether the Gecko event
loop is running nested.

The problems I had (the ones I'm using RunningXULModal() to avoid)
were always and only with modal windows.  And (in
nsAppShell::ProcessNextNativeEvent()) I need to avoid using [NSApp
nextEventMatchingMask:...] plus [NSApp sendEvent:] to process events
unless I absolutely have to -- otherwise I run the risk of triggering
the problems discussed in the first paragraph of my new comments (in
the do ... while();) loop).

The Gecko event loop will always be nested when RunningXULModal() is
TRUE, but not (necessarily) the converse.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #30)
> It's true things would be better if there were a "published" API to
> detect whether or not Gecko is running modal?  Do you think that could
> be managed?

Yes, I think so.

(In reply to comment #31)
> On reflection, I'm quite sure what I need is a way to detect whether
> Gecko is running modal, _not_ a way to detect whether the Gecko event
> loop is running nested.

OK.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

XUL modal windows call nsIWidget::SetModal, so you should be able to hook that here:
http://mxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsCocoaWindow.h#155

Comment 34

[Steven Michaud [:smichaud] (Retired)]

OK, thanks.

I'll revise my patch tomorrow.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Attachment: fix rev7 (better way to detect running XUL modal)

Here's a revised patch that uses the calls to nsIWidget::SetModal()
(from nsXULWindow::ShowModal()) to keep track of whether or not the
"browser" is running XUL modal.

It's too bad I didn't notice these calls before ... oh, well.

I also made one other minor change -- I made the types of
mHadMoreEventsCount and kHadMoreEventsCountMax match (both are now
PRUint32).  This avoids warnings about comparing a signed variable
with an unsigned one.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 281332 [details] [diff] [review]
fix rev7 (better way to detect running XUL modal)

great, that's much better!

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, one thing: when an nsCocoaWindow is destroyed (in the destructor), you should probably fix up gXULModalLevel if it was modal when it was destroyed. That should never happen, I guess, but just in case it does...

Comment 38

[Steven Michaud [:smichaud] (Retired)]

> when an nsCocoaWindow is destroyed (in the destructor), you should
> probably fix up gXULModalLevel if it was modal when it was
> destroyed.

I'd really prefer not to, because (as far as I can tell) doing so
would be very complicated.

I assume that modal XUL windows can be nested (that a modal XUL window
can be spawned while another modal XUL window is active).  If so, how
do I know (in the "first" modal window's destructor) whether or not
the "second" modal window has already closed?  If I assume it has been
but I'm wrong, my "fixing up" will throw gXULModalLevel out of whack.

With my gXULModalLevel code as it now stands, I'm doing exactly as
much as I need for my own purposes, and nothing more.  If at all
possible I'd like to keep things that way.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm worried about what might happen if someone destroys a window without calling SetModal(PR_FALSE) on it first. Then you'll think we're modal forever.

If you do what I suggested, then gXULModalLevel is just a count of the number of modal windows. When a window is destroyed, you just decrement gXULModalLevel if the window is still modal. I don't see what the difficulty is.

Comment 40

[Steven Michaud [:smichaud] (Retired)]

> When a window is destroyed, you just decrement gXULModalLevel if the
> window is still modal.

Now that I see what you're after, it isn't so bad.

I'd have to add per-widget "modality" tracking, but that's not a big
deal.  I'll do so and post another revision.

Comment 41

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev8 (deal with widget destroyed while running modal)

How's this?

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Excellent.

Comment 43

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk.

Comment 44

[Reed Loden [:reed]]

This seems to have turned "MacOSX Darwin 8.10.0 phlox Dep suiterunner" on http://tinderbox.mozilla.org/SeaMonkey-Ports/ orange.

Comment 45

[Robert Kaiser]

Reed: That box had a very tight timeout on the Ts test, it seems it did hit that somehow after this landed, so I increased the setting, but the actual Ts numbers aren't different from what it had before.

Comment 46

[Steven Michaud [:smichaud] (Retired)]

This patch caused or triggered a number of regressions.  I've now got
patches for all the ones that have ben reported (that I'm aware of).
But landing them separately would be a pain.  So (once they've been
reviewed) I plan to post a consolidated patch here, and land that
instead.  (Bug 396833's patch isn't mine, but I'll include it in my
list anyway.)

bug 396796
bug 396833
bug 396860
bug 397039

(Josh also wrote a patch for bug 396829 (another regression).  But
that's been marked security sensitive, so I won't consolidate it
here.)

Comment 47

[Steven Michaud [:smichaud] (Retired)]

While I'm at it I'll include the patch for bug 396710 in my
consolidated patch.  It's not a blocker (nor should it be).  But the
fix is very easy.

Comment 48

[Steven Michaud [:smichaud] (Retired)]

I've at least partially changed my mind about trying to consolidate
all these patches.

The patch for bug 397039 got reviewed and approved very quickly, so I
just landed it on the trunk.  (The patch for bug 396833 has also
already been landed separately, by someone else.)

Comment 49

[Steven Michaud [:smichaud] (Retired)]

I've given up on landing a consolidated patch.

I just landed a fix for bug 396860, and I've also landed a patch for
bug 396796 that partially reverses one of the performance regressions
reported there.

Comment 50

[Serge Gautherie (:sgautherie)]

FWIW, you might want to back out this patch (and maybe other related patch, if any),
as bug 389931 (which initially caused bug 356720) is now fixed.

Comment 51

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #50)

Noted.

But I suspect that the patch for bug 389931 actually _isn't_ a
comprehensive fix for bug 338225, and that this patch may still be
needed.  When I have time I'll look into this further.

In any case I don't think we need to worry about this until after the
Firefox 3 release ... though if you notice any problems you think are
caused by this patch, please comment here.

Comment 52

[Samuel Sidler (old account; do not CC)]

(In reply to comment #51)
> though if you notice any problems you think are
> caused by this patch, please comment here.

Erm. Please file new bugs and add dependencies *then* comment here. If there are regressions from patches, we should look at them individually so we can ensure they get fixed in future revisions of patches. Newly introduced bugs can get lost in comments and are much better tracked in new bugs.