User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5 http://news.opensuse.org/2007/10/04/announcing-opensuse-103-gm/ Browse above site. Notice extremely (!) slow scrolling and 100% CPU load. Now find the (inline) CSS style sheet that begins with ".comment". Remove the "overflow: auto" directive in ".comment .body .content". (I did this by copying the file to a local disk and reading it from there.) Notice normal fast scrolling. The slow scrolling does not happen under Windows XP using Firefox 3 beta 5. Thanks, Jens Reproducible: Always

Bug is confirmed on the latest Firefox 3 RC1 release running OS X 10.5.2

I tried looking at this in Shark and Sampler and didn't see anything that jumped out at me, but those aren't my forte. The page is much slower than say, http://caminobrowser.org/releases/1.6/complete.php (though that's also not nearly as long) and, as comment 0 notes, removing overflow:auto (I used Jesse's Edit Styles bookmarklet to do it live) helps a lot (still a bit slower, but not nearly as bad; no page scrolling long after I stopped scrolling). This bug sounds familiar, but I can't find whatever other bug I'm thinking of.

Another page that feels similar, with unbearably slow scrolling in Firefox 3 compared to 2, is the Reddit comment thread on the release of Firefox 3: http://www.reddit.com/info/6nrk3/comments/ OSX 10.5.3 here.

Confirming this based on the dupe; this really should have a (reduced) testcase, but it's trivial to finger the CSS rule based on comment 0 and comment 2. On some pages (such as http://www.djangoproject.com/documentation/db-api/ from the dupe), this is a regression from Gecko 1.8.1, though on others (like the URL for this bug, which is an extreme case), it doesn't appear to be.

A few other occurrences of this have been duped to bug 317991. In bug 358936 comment 2, Martijn speculates that bug 352093 could fix this sort of problems.

Oh, wow; I thought child widgets were already gone from Gecko 1.9. What's interesting, however, is that this bug was reported as Mac-only (comment 0), though I suppose it's possible that if we're still creating child widgets in overflow: auto cases, then we could still be having Gecko and Cocoa fighting over drawing (bug 310591 comment 1) and getting extra drawing that other platforms don't have? roc, is that plausible (and therefore confirming this as a dupe of the should-be-cross-platform bug 317991)?

Yeah, we still have child widgets in 1.9.

Note that on the djangoproject page linked in comment 5, scrolling with Fx 3 running on Ubuntu Linux is pretty much normal. What triggered my comment 6 was the fact that using 'overflow:hidden' instead of 'overflow:auto' [1] for the djangoproject page returns scrolling speed to normal on OS X. [1] pre, .literal-block {overflow:hidden !important;}

That is pretty atrocious scrolling. Over to Cocoa widgets; I don't think this is anything gfx related. A profile shows some pretty serious lock grabbing: 17.1% 17.1% libSystem.B.dylib pthread_mutex_lock 0.0% 6.1% HIToolbox RetainEvent 0.0% 6.0% HIToolbox EventIteratorNext 0.0% 6.0% HIToolbox FlushSpecificEventsFromQueue 0.0% 6.0% AppKit -[NSView(NSInternal) _uninstallTrackingArea:] 0.0% 0.0% HIToolbox _NotifyEventLoopObservers 0.0% 0.0% HIToolbox PostEventToQueueInternal 0.0% 0.0% HIToolbox CoalesceMouseEvent(OpaqueEventRef*) 0.0% 0.0% HIToolbox AcquireEventFromQueue 0.0% 5.3% HIToolbox ReleaseEvent 0.0% 5.2% HIToolbox EventIteratorNext 0.0% 0.1% HIToolbox _NotifyEventLoopObservers 0.0% 0.0% HIToolbox GetMainEventQueue 0.0% 0.0% XUL _cairo_quartz_surface_show_glyphs 0.0% 0.0% AppKit -[NSWindow _setTrackingRect:inside:owner:userData:useTrackingNum:install:] 0.0% 0.0% AppKit -[NSViewHierarchyLock unlock] 0.0% 0.0% HIToolbox EventIteratorDispose 0.0% 0.0% AppKit -[NSViewHierarchyLock _lockForWriting:handler:] 0.0% 0.0% HIToolbox PostEventToQueueInternal 0.0% 0.0% HIToolbox GetEventFromPool(__CFAllocator const*) 0.0% 0.0% HIToolbox EventIteratorCreate 0.0% 0.0% AppKit -[NSViewHierarchyLock lockForReadingWithExceptionHandler:] 0.0% 0.0% QD TTextLineLayout::PrepareForWriteUse() 0.0% 0.0% CarbonCore TSLockMutex 0.0% 0.0% libnspr4.dylib PR_Lock 0.0% 0.0% libCGATS.A.dylib get_units_per_em 0.0% 0.0% XUL _cairo_quartz_surface_intersect_clip_path 0.0% 0.0% XUL _cairo_quartz_surface_fill 0.0% 0.0% XUL _cairo_quartz_setup_source 0.0% 0.0% XUL _cairo_quartz_create_cgimage 14.4% 14.4% libSystem.B.dylib pthread_mutex_unlock 10.5% 10.5% HIToolbox _GetEventPlatformEventRecord 5.7% 5.7% XUL _cairo_quartz_surface_fill 4.6% 4.6% HIToolbox EventIteratorNext 1.8% 1.8% XUL _cairo_quartz_surface_show_glyphs 1.3% 1.3% XUL _cairo_quartz_surface_stroke 1.2% 1.2% libSystem.B.dylib szone_free Some library charging to callers: 44.2% 44.2% AppKit -[NSView(NSInternal) _uninstallTrackingArea:] 11.5% 11.5% AppKit compareTrackingAreas 5.8% 5.8% XUL _cairo_quartz_surface_fill 2.1% 2.1% XUL _cairo_quartz_surface_show_glyphs 1.7% 1.7% AppKit _NXSetTrackingRect 1.3% 1.3% XUL _cairo_quartz_surface_stroke That 44.2% looks like: 44.2% 44.2% AppKit -[NSView(NSInternal) _uninstallTrackingArea:] 0.0% 44.2% AppKit -[NSView(NSInternal) _uninstallRemovedTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSView(NSInternal) _updateTrackingAreas] 0.0% 44.2% AppKit -[NSWindow resetCursorRects] 0.0% 44.2% AppKit _handleInvalidCursorRectsNote 0.0% 44.2% AppKit _DPSNextEvent 0.0% 44.2% AppKit -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] 0.0% 44.2% AppKit -[NSApplication run] 0.0% 44.2% XUL nsAppShell::Run() The 11.5% looks related: 11.5% 11.5% AppKit compareTrackingAreas 0.0% 11.5% AppKit -[NSView(NSInternal) _uninstallTrackingArea:] 0.0% 11.5% AppKit -[NSView(NSInternal) _uninstallRemovedTrackingAreas] 0.0% 11.5% AppKit -[NSView(NSInternal) _updateTrackingAreas] A bit more charging of symbols to callers, and 56.8% is in [NSWindow resetCursorRects] -> _handleInvalidCursorRectsNote -> _DPSNextEvent -> [NSApplication nextEventMatchingMask....]

As I say at bug 440375 comment #3, this doesn't appear to be a Cocoa widgets bug (since it effects both FF2 and FF3). In fact it could be some kind of Apple bug (or design flaw). But if we can find a workaround, that'll probably go into Cocoa widgets code. (See bug 440375 for more info.)

Yeah, apple bug is probably the most likely. Wonder what we're doing to trigger it, though? I wonder if some of our scrollbar code is doing weird things.

I'm also seeing this on http://icanhascheezburger.com comment pages.

This is the simplest and ugliest solution I could think of, but it works.

won't that break something?

That's the problem. The patch messes with internal Apple stuff, so I don't think we have any of telling if it breaks something. If we really take this patch we'll have to make sure that it gets lots of testing (i.e. take it before the next Beta). I also think that this is caused by an Apple bug; I suspect it's triggered by creating lots of child widgets. So the proper way to fix it would be to avoid creating unnecessary widgets (i.e. Compositor). I guess the purpose of _updateTrackingAreas is to make sure that mouse cursor handling doesn't get messed up when NSViews are moved. But we're not using Cocoa to track the mouse and set cursors - we're doing all that cursor stuff ourselves. None of the NSView / NSWindow methods under "Managing Cursor Rectangles" (see Apple docs) are ever called in Gecko. So I think making _updateTrackingAreas a no-op won't hurt us. But of course we can't know that for sure.

Comment on attachment 344834 [details] [diff] [review] patch v1 I really don't like this -- it's like killing a fly with a hammer. But if I'm right about what _updateTrackingAreas does, the consequences of (in effect) commenting it out for ChildView objects should be very bad (and therefore easily noticeable). So if we don't see those consequences in our tests, I suppose this solution could be kept in mind as a last resort -- to be used if we can't come up with anything better and if we absolutely need to fix this bug right away. So I'm not r-ing this ... at least for the time being. I've played around a bit with tracking areas in another program I wrote (not the JEP, and not (unfortunately) open source). As I understand it, they're what (on receiving mouse-moved events) generate (if appropriate) mouse-entered and mouse-exited events. They're attached to NSWindow objects (I believe NSWindow's undocumented _cursorRects variable is a table of it's current tracking rects). Every time an NSWindow object changes its dimensions, all its old tracking rects become obsolete, and they all need to be deleted and replaced by new ones. If a given tracking rect isn't deleted in time, and the NSView object to which it corresponds has itself been deleted, you get crashes in [NSWindow sendEvent:] when OS code tries to send mouse-exited/mouse-entered events to the deleted NSView. [NSView _updateTrackingAreas] is (of course) an NSView method. I don't really know what it does, or how it fits into the picture I drew above. But if (say) it ensures that all "its" tracking rects get updated on time in "its" NSWindow object, commenting it out might lead to the NSWindow's list of tracking rects containing items that point to deleted NSView objects. I think we need to look for a solution that targets this problem more directly. That it uses (or takes advantage of) undocumented methods isn't necessarily a problem -- especially if we use them to work around an Apple bug. But it'd be better if we found out what exactly this Apple bug is, so we can make our workaround more precise.

(In reply to comment #18) > I also think that this is caused by an Apple bug; I suspect it's triggered by > creating lots of child widgets. So the proper way to fix it would be to avoid > creating unnecessary widgets (i.e. Compositor). As long as we're still creating child widgets for those <div>s, AppKit is still going to be telling us to redraw ones that are not visible and which we don't want to redraw; see comment 7 here, and especially bug 310591 comment 1 (for an alternative strategy to short-circuit the extra drawing).

(Following up comment #19) Another thing -- [NSView _updateTrackingAreas] doesn't exist in Tiger (only in Leopard, and perhaps also in SnowLeopard).

(Following up comment #19 and comment #21) Yet another thing (and very interesting) -- as best I can tell this bug is 10.5-only (it doesn't happen on Tiger). Can others confirm or deny this?

This fixes the pathological _updateTrackingAreas case.

The patch is based on (sort-of) the comment on http://developer.apple.com/documentation/Cocoa/Conceptual/EventOverview/MouseTrackingEvents/chapter_950_section_3.html#//apple_ref/doc/uid/10000060i-CH11-DontLinkElementID_27 "An NSView object’s cursor rectangles are automatically reset whenever: *Its frame or bounds rectangle changes, whether by a setFrame... or setBounds... message or by autoresizing." So I started to test some other way to move the view. Apparently setFrameOrigin doesn't change the rectangle in the way the text means. Sounds still like an Apple bug though.

(In reply to comment #24) > .Sounds still like an Apple bug though. ..because IMO .setFrame should detect when it is called without any changes to frame size. (And I'm still an OSX newbie, may easily make mistakes.)

A tryserver build: https://build.mozilla.org/tryserver-builds/2008-11-09_04:48-opettay@mozilla.com-1226234866/

Comment on attachment 347148 [details] [diff] [review] Use .setFrameOrigin when moving This sounds very promising -- it's much less violent than Markus' patch :-) But I'm also very surprised it makes any difference. It's hard to believe [NSView setFrameOrigin:] doesn't cause the cursor rectangles to be reset (in fact I suspect setFrameOrigin: calls setFrame:). But (if so) that just means some other explanation needs to be found (for why your patch works). I need to spend several hours digging around in Apple internals. I'd prefer to put that off until sometime next week (11-10 through 11-14) -- so this will miss beta2.

And yes, your tryserver build from comment #26 fixes this bug for me (testing on OS X 10.5.5 with this bug's URL, http://news.opensuse.org/2007/10/04/announcing-opensuse-103-gm/). The difference in scrolling speed from today's Minefield nightly is dramatic.

(In reply to comment #27) > It's hard to believe [NSView setFrameOrigin:] doesn't cause the cursor > rectangles to be reset (in fact I suspect setFrameOrigin: calls > setFrame:). Actually it *might* be other way round; setFrame calls setFrameOrigin. "cursor rectangles need to be reset often as a view’s size and graphic elements change" Maybe setFrameOrigin doesn't need to cause reset, because it moves view in its parent's coordinate space - doesn't really change the handling within the view. But still, no idea what is happening with TrackingAreas. Anyway, the patch helps here, and it is using only public API.

(In reply to comment #29) My main point is that (though I like your patch better than Markus') I won't feel comfortable with it until I've spent a few hours digging around in Apple internals. Do you really, really want this to get into beta2 ... or can it wait for a bit? (By the way, it's no particular advantage that your patch uses a documented API -- it's much more important that it's a much smaller change.)

> it's much more important that it's a much smaller change I should have said "it's much more important that it appears to be a much smaller change".

(In reply to comment #30) > Do you really, really want this to get into beta2 ... or can it wait > for a bit? It is always better to get as much testing as possible, especially for things which we can't be sure about. But personally, I can wait.

It's a cold, rainy day in Chicago. My new SnowLeopard install disk (the installer on it) just crashed as I was trying to run it. And what you and Markus have done (with your patches) has really piqued my curiosity I'll see what I accomplish for the rest of the afternoon :-)

OK, the time I thought I'd spend digging around in Apple internals was instead spent finding a much more precisely targeted workaround for this bug (which is almost certainly a 10.5-specific Apple bug). It's our calls to [NSView addTrackingRect:...] and [NSView removeTrackingRect:] (in [ChildView setFrame:]) that slow things down (on OS X 10.5). But it turns out they're completely unneeded! Adding a tracking rect to an NSView object (or NSView subclass object) makes the OS send mouseEntered and mouseExited events at the appropriate times to that object (to call mouseEntered: and mouseExited: on it). A subclass can implement mouseEntered: and mouseExited: handlers to change the cursor as it sees fit. http://developer.apple.com/documentation/Cocoa/Conceptual/EventOverview/MouseTrackingEvents/chapter_950_section_2.html But ChildView has no mouseEntered: or mouseExited: method -- so it never sees mouseEntered or mouseExited events, and never makes use of them. So we don't need to bother adding and removing tracking rects. An embedder might (somehow) hook ChildView and add its own mouseEntered: and mouseExited: handlers. But Camino (the only embedder of which I'm aware) doesn't do this. (Camino does use mouseEntered: and mouseExited: handlers in its own RolloverImageButton and TabButtonView objects. But these are Camino-specific, and Camino also (as it needs to) does its own adding and removing of tracking rects for these objects.) In any case I doubt we'd consider this (hooking the ChildView class) to be legitimate behavior -- that'd be far too much like the kind of tricks *I* get up to :-) 1.9-branch Camino also has this bug's problem. I suspect my patch will cure it, without adversely effecting its own use of tracking rects. I'll test this in the next hour or so. By the way, there's nothing wrong with the way nsChildView.mm adds and removes tracking rects. It mimicks very closely Apple's examples from http://developer.apple.com/documentation/Cocoa/Conceptual/EventOverview/MouseTrackingEvents/chapter_950_section_2.html. This (and the fact that this bug is 10.5-only) makes me pretty certain that it's an Apple bug.

Here's a tryserver build made with my patch (from comment #34): https://build.mozilla.org/tryserver-builds/2008-11-09_13:23-smichaud@pobox.com-1226265746/smichaud@pobox.com-1226265746-firefox-try-mac.dmg Try it out! Especially with this bug's URL (https://bugzilla.mozilla.org/attachment.cgi?id=347185), which makes an excellent test of changing the cursor as it moves over the document's "screenshots" (the cursor should be a hand over the screenshots, and an ordinary arrow elsewhere).

Oops, this bug's URL is: http://news.opensuse.org/2007/10/04/announcing-opensuse-103-gm/

Great work, Steven! I had a look at the patch for bug 150297. I suspect the calls to addTrackingRect: were added so that the "grab" cursor is set when pressing Cmd+Option and moving the mouse out of and back into the scroll view... but due to the missing mouseEntered: or mouseExited: methods, this has probably never worked.

The scrolling works _much_ better for me now (Macbook pro, 2.4 Ghz Intel Core 2 Duo, Mac OS X 10.5.5), both on the opensuse page and on other pages that has been sluggish. However the cursor doesn't change quite as I guess it should. If I move the cursor over one of the screenshots it does change into a hand. When I move it out it changes back to a cursor. However, if I scroll (using the standard two finger gesture) so that the cursor winds up inside a screenshot it does not change to a hand and won't do so until I invoke a new 'standard' mouse enter event by moving the mouse cursor out of the screenshot and then in again. If I move the cursor normally into a screenshot it changes to a hand, but if I scroll slightly, so that the cursor is still inside the screenshot, it changes to a cursor and I have to again trigger a mouse enter event to get it to become a hand again. This however is minor details and doesn't change the fact that the new snapshot is much better than the last release, so kudos =)

(In response to comment #38) > However, if I scroll (using the standard two finger gesture) so that > the cursor winds up inside a screenshot it does not change to a hand > and won't do so until I invoke a new 'standard' mouse enter event by > moving the mouse cursor out of the screenshot and then in again. I expect this also happens without the patch. Does it?

(In reply to comment #39) > I expect this also happens without the patch. Does it? For me it does.

(In reply to comment #37) > Great work, Steven! Thanks! :-) > I suspect the calls to addTrackingRect: were added so that the > "grab" cursor is set when pressing Cmd+Option and moving the mouse > out of and back into the scroll view... but due to the missing > mouseEntered: or mouseExited: methods, this has probably never > worked. Interestingly, ChildView on the 1.8 branch (where it was only used by Camino) does have mouseEntered: and mouseExited: methods. I wonder if they simply got dropped (by mistake) in the transition to 1.9-branch ChildView (and Cocoa widgets). It's really too bad we can't use them (because of Apple's bug) ... at least for now. If we *did* want to use them, we'd have to figure out more precisely what the Apple bug is, and apply a bandage that just covers the wound. But that's a lot of work, with no guarantee of success. It's probably too big a task to take on for the 1.9.0 or 1.9.1 branches.

(Following up comment #41) > If we *did* want to use them [addTrackingRect: and > removeTrackingRect:], we'd have to figure out more precisely what > the Apple bug is, and apply a bandage that just covers the wound. Or maybe we could figure out how to use addTrackingArea: and removeTrackingArea: on 10.5 and up (these are new with 10.5). I'll just bet these don't suffer from Apple's bug :-) But this'd still be a lot of work, and would probably now be too big a change for the 1.9.0 and 1.9.1 branches.

(In reply to comment #39) > I expect this also happens without the patch. Does it? Sort of, with one exception. It seems like the first scroll into a screenshot turns on the hand, but if I scroll more, in such a way that the cursor is kept within the screenshot, then the hand disappears. Doing just one scroll to get into a screenshot area is possible due to the lag. That is just a detail though.

How about filing a new bug for the issue mentioned in comment 38?

Comment on attachment 347185 [details] [diff] [review] Fix (same idea as Markus' and Smaug's patches, more precisely targeted) Looks fine to me, I removed mouseEntered and mouseExited on purpose because they were unreliable and causing us to get into a bad state. I replaced their functionality by calculating mouse enter/exit in mouseMoved:. If we want to do something like this again in the future we should probably use NSTrackingArea (10.5 only).

Comment on attachment 347185 [details] [diff] [review] Fix (same idea as Markus' and Smaug's patches, more precisely targeted) Code removal only?

> Code removal only? Yes. The code removed is unused and also triggers Apple's bug (on 10.5).

> The code removed is unused Or more precisely it has no effect.

>> The code removed is unused > > Or more precisely it has no effect. Best to say what I originally did -- it's unneeded. It serves no useful purpose, but triggers Apple's bug.

Comment on attachment 347185 [details] [diff] [review] Fix (same idea as Markus' and Smaug's patches, more precisely targeted) a & <3 = beltzner

> a & <3 What does this mean? "Approval" and what? :-)

It's a heart. Usually means "love".

Thanks. I'm impressed! :-)

Comment on attachment 347185 [details] [diff] [review] Fix (same idea as Markus' and Smaug's patches, more precisely targeted) Landed on mozilla-central (changeset f22c0016f923).

This is different from my issue. I get extremely laggy browsing on some pages, pages i've never had issues with before on FF2. I was told it was because I had too many add-ons, and it slows down my browser, but then why is that on extremely rare occasions it wants to behave? I only have like 7 anyway. Just as well, the same pages work just fine in Internet Explorer. I viewed the first sample link just fine, however, so I guess that means my issue has nothing to with overflow: auto

Verified Fixed on: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090303 Minefield/3.2a1pre ID:20090303020504

This landed on 1.9.1 and has the fixed keyword, but I don't see the checkin referenced, so here it is: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f22c0016f923,

verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090401 Shiretoko/3.5b4pre

Aakash: This bug is a Widget:Cocoa bug and as such, is only visible in Mac builds and must be verified with them.

Verifying on Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre testing on pages from Comment #5, Comment #3, Comment #2, Comment #0