Bugzilla

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Assignee

Comment 11

•

16 years ago

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.)

Assignee: joshmoz → smichaud

Priority: -- → P3

Comment 12

•

16 years ago

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.

Justin Dolske [:Dolske]

Comment 15

•

16 years ago

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

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

Comment 16

•

16 years ago

Attached patch patch v1 (obsolete) — Details — Splinter Review

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

Attachment #344834 - Flags: review?(smichaud)

Comment 17

•

16 years ago

won't that break something?

Comment 18

•

16 years ago

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.

Smokey Ardisson (offline for a while; not following bugs - do not email)

Assignee

Comment 19

•

16 years ago

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.

Comment 20

•

16 years ago

(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).

Assignee

Comment 21

•

16 years ago

(Following up comment #19)

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

Assignee

Comment 22

•

16 years ago

(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?

Updated

•

16 years ago

Flags: blocking1.9.1?

Comment 23

•

16 years ago

Attached patch Use .setFrameOrigin when moving (obsolete) — Details — Splinter Review

This fixes the pathological _updateTrackingAreas case.

Attachment #347148 - Flags: review?(smichaud)

Comment 24

•

16 years ago

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.

Comment 25

•

16 years ago

(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.)

Comment 26

•

16 years ago

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

Assignee

Comment 27

•

16 years ago

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.

Assignee

Comment 28

•

16 years ago

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.

Comment 29

•

16 years ago

(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.

Assignee

Comment 30

•

16 years ago

(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.)

Assignee

Comment 31

•

16 years ago

> 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".

Comment 32

•

16 years ago

(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.

Assignee

Comment 33

•

16 years ago

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 :-)

Assignee

Comment 34

•

16 years ago

Attached patch Fix (same idea as Markus' and Smaug's patches, more precisely targeted) — Details — Splinter Review

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.

Attachment #344834 - Attachment is obsolete: true

Attachment #347148 - Attachment is obsolete: true

Attachment #347185 - Flags: review?(joshmoz)

Attachment #344834 - Flags: review?(smichaud)

Attachment #347148 - Flags: review?(smichaud)

Assignee

Comment 35

•

16 years ago

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).

Assignee

Comment 36

•

16 years ago

Oops, this bug's URL is:

http://news.opensuse.org/2007/10/04/announcing-opensuse-103-gm/

Comment 37

•

16 years ago

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.

Fredrik Berg Kjølstad

Comment 38

•

16 years ago

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 =)

Assignee

Comment 39

•

16 years ago

(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?

Comment 40

•

16 years ago

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

For me it does.

Assignee

Comment 41

•

16 years ago

(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.

Assignee

Comment 42

•

16 years ago

(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.

Fredrik Berg Kjølstad

Comment 43

•

16 years ago

(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.

José Jeria

Comment 44

•

16 years ago

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

Josh Aas

Comment 45

•

16 years ago

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).

Attachment #347185 - Flags: review?(joshmoz) → review+

Assignee

Updated

•

16 years ago

Attachment #347185 - Flags: superreview?(roc)

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

Assignee

Updated

•

16 years ago

Attachment #347185 - Flags: superreview?(roc) → superreview?(vladimir)

Updated

•

16 years ago

Attachment #347185 - Flags: superreview?(vladimir) → superreview+

Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)

Comment 46

•

16 years ago

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

Code removal only?

Assignee

Comment 47

•

16 years ago

> Code removal only?

Yes.  The code removed is unused and also triggers Apple's bug (on 10.5).

Assignee

Comment 48

•

16 years ago

> The code removed is unused

Or more precisely it has no effect.

Assignee

Comment 49

•

16 years ago

>> 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.

Mike Beltzner [:beltzner, not reading bugmail]

Assignee

Updated

•

16 years ago

Attachment #347185 - Flags: approval1.9.1b2?

Comment 50

•

16 years ago

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

a & <3 = beltzner

Attachment #347185 - Flags: approval1.9.1b2? → approval1.9.1b2+

Boris Zbarsky [:bzbarsky]

Assignee

Comment 51

•

16 years ago

> a & <3

What does this mean?

"Approval" and what? :-)

Comment 52

•

16 years ago

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

Assignee

Comment 53

•

16 years ago

Thanks.  I'm impressed! :-)

Updated

•

16 years ago

Blocks: 418351

Assignee

Comment 54

•

16 years ago

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).