332579 - Improve Mac event handling for 1.8.1

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Mark Mentovai]

I've made some changes to Mac event handling on the THREADS_20060307_BRANCH that totally eliminate WaitNextEvent, which has been the source of much suckage on the Mac.  It would be nice to take a subset of these changes on the MOZILLA_1_8_BRANCH after the threads branch is merged to the trunk and has gotten some testing exposure and we've had an opportunity to assess the risk.

In what I've done, the EventRecords returned by WaitNextEvent are instead created by a transitional application-scope Carbon event handler.  The main run loop can then become a Carbon run loop.

Comment 1

[Mark Mentovai]

Attachment: WaitNextEvent-free AppShell

There's currenty a bug with this AppShell, so I'm not asking for review yet: if you open a browser window and a preferences window, and then press command-W to close the preferences window, the browser window will also close.

Comment 2

[Mark Mentovai]

Attachment: MacMessagePump cleanup

Changing event handling allows a lot of junk to be ripped out of nsMacMessagePump.  There may be more cleanup possible.

Comment 3

[Mark Mentovai]

Attachment: Checkpoint

The bugs are fixed in this one, but I'm not finished with cleanup yet.

Comment 4

[Mark Mentovai]

The cleanup gets rid of:
 - already-dead code, either #ifdefed or otherwise unreachable
 - code that is becoming dead as a result of the new event loop impl
 - code that should have been dead or #ifdefed but erroneously never was
   (some of which was last useful only to Classic or CFM users)

To exercise some code of the third class, hold down the command key while starting to resize a window by grabbing the grow box in its lower-right corner.  You'll hit some code designed for live resize in the classic OS that was never properly disabled.  It interacts poorly with the current Carbon event-based live resize code.

I think the most recent "checkpoint" is the bulk of the cleanup.  I'm going to try to do a little bit more before posting a reviewable patch.  I'd like to see these changes on the trunk too.  Even though the appshell will change with the THREADS_20060307_BRANCH landing, the new appshell will be able to take advantage of this same cleanup (with an API change or two).

Note to self for the threads branch: ReceiveNextEvent (used here) is probably better than AcquireFirstMatchingEventInQueue/RemoveEventFromQueue (used there).

After this lands, there's some additional work that can be done to reduce the number of times the transitional WNE event handler is hit.  For example, right now, we have a kEventWindowUpdate handler on each nsMacWindow, but it doesn't actually properly handle the event (nor does it inform anyone that the event was handled).  The transitional event handler gets it at the application scope and has to figure out which window should deal with it.  This amounts to busywork in the path for one of the more common event types.

Some handlers for the other common event types can probably also be easily moved into the appropriate class (usually nsMacWindow), eliminating the overhead of the transitional handler.

Comment 5

[Mark Mentovai]

Attachment: v3

Comment 6

[Mark Mentovai]

Attachment: v3, applies to branches

Comment 7

[Mark Mentovai]

Comment on attachment 219641 [details] [diff] [review]
v3

Sorry, Josh.  This is mostly a lot of stuff that's being removed.  The interesting things are happening in nsAppShell, the diff to that file is bigger than the patched file, so it might be easier to just patch it and read it.  There's also interesting stuff happening in nsMacMessagePump: that's where the transitional event handler lives.  You might recognize it from the threads branch.

Comment 8

[Mark Mentovai]

Comment on attachment 219642 [details] [diff] [review]
v3, applies to branches

The branch patch is different from the trunk patch in that it removes nsWatchTask.  This was already removed on the trunk.

Comment 9

[Mark Mentovai]

Attachment: v4 (branch) fixes 336195

Fixes bug 336195:

Changed the timeout in nsAppShell::GetNativeEvent to not block forever when someone's manually running the event loop.

Doesn't over-free the nsMacTSMMessagePump singleton.

Comment 10

[Mark Mentovai]

Attachment: v5 (branch) also fixes 336197

When I was developing this patch on the trunk, opening a browser window and a preferences window and then pressing command-W would close both windows, so I worked around it by eating more events than I really should have.  I'm not seeing the same problem on the branch, and am able to fix bug 336197 by not swallowing unprocessed events.

Comment 11

[Mark Mentovai]

Attachment: v6 (branch) fixes 336365

Comment 12

[Mark Mentovai]

Attachment: v7 (branch) fixes 336365 for non-debug builds too

This one whacks all regressions reported so far.

Test build:

http://jackassofalltrades.com/tmp/firefox-1503-332579v7.dmg

Comment 13

[Mark Mentovai]

Comment on attachment 220634 [details] [diff] [review]
v7 (branch) fixes 336365 for non-debug builds too

Josh, if you want, I can summarize the changes I made since v3.  It was mostly just moving things around and enabling and disabling stuff to more closely track the way things worked with the WaitNextEvent loop.

Comment 14

[Bill McGonigle (not currently reading bugmail; please contact directly)]

(In reply to comment #12)
> http://jackassofalltrades.com/tmp/firefox-1503-332579v7.dmg

Mark, would you expect this build to provide significant performance benefits over a standard build without the event handling changes?  That appears to be the case here in nearly all aspects of use, at least on my admittedly slower hardware, and so far no whammies.  Excellent work.

Comment 15

[Mark Mentovai]

Absolutely.  Current "standard" builds are based on an event loop that's basically spinning when it doesn't need to be.  The key architectural change here is that the loop goes to sleep until something actually happens.  There are also side benefits in the elimination of a bunch of dead code paths that would execute frequently.  These paths were useful on OS 9 but are dead ends on OS X.

Remaining known regressions are bug 336365 comment 2.  I've fixed the second one (but the fix isn't in v7) but haven't had a chance to look at the first one yet.

(A benefit of slower hardware is that it exhibits performance improvements and detriments more readily - good for testing!)

Comment 16

[Mark Mentovai]

Attachment: v8 (branch) fixes other 336365 regressions

Double-processing of certain key events was being caused by weird and ugly reentrancy problems.  I avoided the reentrancy by not handling key events while a key events is already being processed.  This is not obviously safe, but it doesn't seem to harm anything yet.

Unicode keyboard breakage after running a modal sheet was fixed by making the nsMacMessagePump a singleton.  (I'd actually like better memory management here than working with static data, but none of this stuff is COMatose.)

A universal test build with these fixes is available at:

http://jackassofalltrades.com/tmp/firefox-1503-332579v8.dmg

Comment 17

[Jo Hermans]

(In reply to comment #16)
> A universal test build with these fixes is available at:
> 
> http://jackassofalltrades.com/tmp/firefox-1503-332579v8.dmg
> 

Works like a charm under Mac OS X 10.2.8, on a dead-slow 300 MHz iBook. Big improvement. For instance, when typing in a textfield like this one, the cpu-usage used to be 30-40%, but it now dropped to 20-30%.

The only defect that I found in 10 minutes of usage, was that a cursor movement in a textfield like this one, was always skipping by 2 characters. But it was an old branch I noticed (it still had the click-and-hold context menu), so I don;'t know if this is significant.

Comment 18

[Mark Mentovai]

Jo, the test builds are equivalent to Firefox 1.5.0.3 plus the patches attached to this bug.  This gives a pretty stable base for testing, although the patch itself isn't targeted at the 1.8.0 branch.  That branch still does have the click-and-hold contextual menus.

I don't see anything skipping two characters in text fields.  Do you mean navigating with the arrow keys?  If you do find a bug that occurs in the v8 test build but not in Firefox 1.5.0.3, file a new bug report and mark it as blocking this bug.  If it's a keypress bug, be sure to specify which keyboard layout you're using (in the International system preference pane).  Thanks to everyone for the testing and feedback.

Comment 19

[Jo Hermans]

(In reply to comment #18)
> I don't see anything skipping two characters in text fields.  Do you mean
> navigating with the arrow keys?  If you do find a bug that occurs in the v8
> test build but not in Firefox 1.5.0.3, file a new bug report and mark it as
> blocking this bug.  If it's a keypress bug, be sure to specify which keyboard
> layout you're using (in the International system preference pane).  Thanks to
> everyone for the testing and feedback.
> 

It was with the arrow keys, using the Dutch keyboard layout.

I'm not able to repeat it right now ; it happened after several dozen webpages, testing with Flash and QuickTime, etc ... I'll keep an eye on it if I see it again.

Comment 20

[Mark Mentovai]

Comment on attachment 220791 [details] [diff] [review]
v8 (branch) fixes other 336365 regressions

Josh, can you sign off on the changes since v3?  Here's a summary of the changes:

- change nsAppShell::GetNativeEvent to not block forever (when events are being processed outside of nsAppShell::Run) (fixes parts of 336195).
- manage nsTSMMessagePump and nsMacMessagePump better, both are singletons now (fixes 336365 comment 2 part 2).
- nsMacMessagePump::WNETransitionEventHandler now returns eventNotHandledErr if an event was not handled, to allow system handlers of last resort to take care of things like command-` (fixes 336197).
- nsMacMessagePump::DispatchEvent will only dispatch when nsAppShell::Run or nsAppShell::ProcessNativeEvent is on the stack, avoiding crashes when the handlers are still installed but the app is shutting down (fixes 336195).
- install TSM AE handlers after transitional CE handlers to give them first crack at handling keystrokes.  Fixes inability to type when using Unicode keyboard layouts (336365 comment 0).
- Avoid reentrancy in nsMacMessagePump::DoKey (fixes 336365 comment 2 part 1) - there may be a better solution that can be applied here.

I've been running v8 for a couple days and so have a couple of testers - it's definitely stable enough for trunk/a2, and if there are regressions left, it'll be easier to handle them once this has all landed.

Comment 21

[Isaac]

(In reply to comment #20)

> I've been running v8 for a couple days and so have a couple of testers

I've been running it too (10.3.9, lots of FX extensions, doing heavy js/css web development so likely to stress it in weird ways) and v8 has been performing great.  The speed improvement is amazing, particularly when interacting with things like js drag and drop widgets.

Comment 22

[jhp (no longer active)]

Using build from comment #16, and I saw an issue that might be related to this patch.  In Gmail, I clicked on the "Download" link in order to download the attachment.  This does two this: (1) sets the focus on the "Download" link, and (2) brings up (at least in my case) a dialog asking whether to save the file or open with an app.  'Save' was already selected, so I just hit Enter, but then I saw the dialog immediately reappear.

It looks like the Enter key was being counted twice, first to select the default button in the dialog, and secondly to reselect the "Download" link.

Comment 23

[Mark Mentovai]

Javier, that's probably related - these changes caused behavior like that before (bug 336365 comment 2 part 1).

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 220791 [details] [diff] [review]
v8 (branch) fixes other 336365 regressions

sr=shaver (curse needing to log into bugzilla!)

Comment 25

[Josh Aas]

Please land this on the Firefox 2 branch ASAP. We are approved by drivers via email. We need to have this in by Monday morning.

Comment 26

[Mark Mentovai]

will land tonight when I have a chance to tree-sit

Comment 27

[Mark Mentovai]

Not landing yet - tree is closed.

Comment 28

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This needs to land today before about 2:30pm to make a2 -- branch tree is open, I think we should go straight there rather than wait for the trunk logjam to clear.

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Approved for landing through tree-closure, in Fx2a2 triage meeting.  Godspeed!

Comment 30

[Mark Mentovai]

v8 (branch) checked in on MOZILLA_1_8_BRANCH for 1.8a2.

Comment 31

[Mark Mentovai]

That should be 1.8.1a2.

Comment 32

[Mark Mentovai]

Key event regressions are covered in in bug 337199 patch v1 (attachment 221541 [details] [diff] [review]).

A new test build of Firefox 1.5.0.3 + 332579v8 + 337199v1 is available here:
http://jackassofalltrades.com/tmp/firefox-1503-332579v9.dmg

Comment 33

[Mike Schroepfer]

Should this be marked Resolved?

Comment 34

[Mark Mentovai]

Yeah, this is fixed.  Keyboard regressions are dependencies and are being mostly handled in bug 337199.  Frontporting this patch to the trunk is bug 338153.

Comment 35

[Mark Mentovai]

I've got a new test build available, equivalent to 1.5.0.4rc3 plus 332579v8 (this patch) and further key regression fixes from 337199v4.  This build is available at:

http://jackassofalltrades.com/tmp/firefox-1504rc3-332579v8-337199v4.dmg

I've prepared this build even though the above patches are now all checked in on the 1.8.1 branch because that branch is only alpha-quality at this point, and I'd like testers who have the time to be able to bang on the event improvements in isolation on a more stable codebase.  So, testers, if you have the time, have at it!