510919 - Cursor can fail to change shape loading a new page (OS X)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

This bug is spun off from bug 504450, particularly from bug 504450
comment #20 and bug 504450 comment #32.  As best I can tell this bug
isn't a focus bug, and is unrelated to bug 504450 (see the STR in bug
504450 comment #0).

Furthermore, this bug (unlike bug 504450) is at least partially
cross-platform (I've seen something like it on both OS X and Windows,
though with different regression ranges).  It happens on
mozilla-central and the 1.9.2 branch, but not elsewhere.

STR:

1) Click on a link to this bug's URL
   (http://www.coteindustries.com/articles/html_articles/tabindex.html).

   Be careful not to move the mouse as you click on the link.  If you
   move it enough to change it to a pointer before the new page has
   finished loading, the bug doesn't happen.

2) A new page will load, but the mouse cursor will fail to change from
   a hand cursor back to a pointer.  This happens on both OS X and
   Windows.

   On Windows the mouse cursor changes to a pointer as soon as you
   move the mouse.  But on OS X the mouse cursor stays a hand cursor
   until you click somewhere in the page (this last may be caused by
   an additional, OS-X-specific bug).

The regression range for this bug is as follows on OS X:

firefox-2009-07-21-03-mozilla-central
firefox-2009-07-22-03-mozilla-central

And as follows on Windows:

firefox-2009-03-05-04-mozilla-central
firefox-2009-03-06-04-mozilla-central

After lunch I'll test on Linux.  For now I can't make anything of
these two regression ranges.

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

i can confirm the range (for windows) mentioned in comment 0:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e81fbdd55e4&tochange=3270fd0db0f0
=>  Bug 481359?

Comment 2

[Steven Michaud [:smichaud] (Retired)]

> =>  Bug 481359?

I assume you're suggesting this (on Windows) may be a regression
caused by (or triggered by) Markus' patch for bug 481359.  Sounds
reasonable to me.

Markus, what do you think?

From looking at your patch, I'd guess it just uncovered this bug (at
least on Windows), and didn't cause it.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

As best I can tell this bug doesn't happen on Linux.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #0)

> [This bug] happens on mozilla-central and the 1.9.2 branch, but not
> elsewhere.

On Windows in also happens on the 1.9.1 branch (in FF 3.5.X).

Comment 5

[Markus Stange [:mstange]]

I think bug 445567 is the most likely cause (that's me again).

Comment 6

[Markus Stange [:mstange]]

Steven, does removing the code that was added in bug 325558 fix this bug?

Comment 7

[Markus Stange [:mstange]]

Attachment: v1

Does this fix it? And maybe bug 496601, too, without regressing bug 325558?

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 394905 [details] [diff] [review]
v1

Er, wait, I need to make this compile first

Comment 9

[Markus Stange [:mstange]]

Attachment: v1

Comment 10

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #5)

> I think bug 445567 is the most likely cause (that's me again).

Yes, it is -- backing it out fixes this bug (on OS X 10.5.8 at least).

Which astonishes me.  I didn't think we were using cursor rects for
*anything*.  Some kind of Apple wierdness, I suppose.

It's entirely possible, though, that the patch for bug 445567 (in
addition to some later change during the regression window) only
*triggered* this bug.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #6)

> Steven, does removing the code that was added in bug 325558 fix this bug?

Nope.  Makes no difference (at least on OS X 10.5.8).

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 394907 [details] [diff] [review]
v1

This patch doesn't fix the bug (at least on OS X 10.5.8).

Comment 13

[Markus Stange [:mstange]]

Does it fix bug 496601?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

On OS X, this bug doesn't happen on OS X 10.4.11 -- only on OS X
10.5.X.  That, plus comment #10, strongly suggests that this bug (or
at least the trigger for this bug) is an Apple bug (having to do with
the different effects of [NSWindow disableCursorRects] on OS X 10.4.X
and 10.5.X).

Am I right, Markus, that your development system runs OS X 10.4.11
only (not 10.5.8)?  If so, maybe I should take this bug.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

> If so, maybe I should take this bug.

Well, the OS X side of it.

I now suspect we're dealing with two different bugs (one on Windows and another on OS X), that are at best tangentially related.

Comment 16

[Markus Stange [:mstange]]

Right, my development system only runs 10.4. It would be great if you could take this bug.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 394907 [details] [diff] [review]
v1

> Does it fix bug 496601?

Yes it does.

But I had to test on the 1.9.1 branch, since bug 496601 no longer
happens on the trunk or the 1.9.2 branch.

Comment 18

[Markus Stange [:mstange]]

Thanks for testing! And of course bug 496601 is not reproducible on trunk, how could I miss that...
I'll move the patch there.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix for OS X bug

Here's a patch that fixes this bug (the OS X bug) without regressing
bug 445567.  It's *much* simpler than I feared it would be.

A tryserver build will follow in a few hours.

Once I understand the Windows-specific bug better, I'll spin that off
into a new bug.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

I've spun the Windows-specific bug off into bug 511151.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch from comment #19:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla510919/bugzilla510919-macosx.dmg

Please try it out, Stephen! :-)

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #21)
> Here's a tryserver build made with my patch from comment #19:
> https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla510919/bugzilla510919-macosx.dmg
> 
> Please try it out, Stephen! :-)

I can confirm that it fixes that bug (woot!); one thing that seems to have regressed, though, is that command+w fails to close that tab _without_ an explicit click on the page.

Comment 23

[Markus Stange [:mstange]]

Nice Steven, the fix makes sense. But I'd like to understand what exactly is
going on - when the cursor update fails, is -[nsCursorManager setCursor:]
called at all? Does the call to -[NSCursor set] in -[nsCocoaCursor setFrame:]
just fail silently? If that's the case, what happens when you replace the
performSelectorOnMainThread: invocation with a direct call to set, like I do in
the (second) patch in bug 496601?

Comment 24

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #22)

> I can confirm that it fixes that bug (woot!);

Glad to hear it!

> one thing that seems to have regressed, though, is that command+w
> fails to close that tab _without_ an explicit click on the page.

I don't see this (either with or without my patch).  Try it in safe
mode (without extensions).

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

The second part of comment 22 only happens when I have two tabs open.  In the screencast that follows, opening up a new window is fine, but a new tab requires that explicit page click before command+W works.

http://screencast.com/t/MuUG2OvVWqca

I'll see if I can reproduce with Namoroka.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #23)

I'm afraid I didn't look into it that far.  I was satisfied when I
found that (on OS X 10.5 and above) I could substitute calling [NSApp
disableCursorRects] once for calling [NSWindow disableCursorRects] on
every browser window.

Clearly this means that [NSApplication disableCursorRects] does
something more (where it's available) than just calling [NSWindow
disableCursorRects] on each window -- otherwise your patch for bug
445567 wouldn't have caused this regression.  I don't know what the
"something more" is, nor do I feel we need to know:  [NSApplication
disableCursorRects] seems to do what's needed, and it makes sense that
this is what an NSApplication method with that name would do.  It's
just too bad that Apple hasn't (yet) documented [NSApplication
disableCursorRects] -- it seems to be much more suitable to our
purposes (globally disabling cursor rects) than [NSWindow
disableCursorRects].

Comment 27

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #25)

Please give me step-by-step STR.  I can't figure out your webcast.

I also tested with two tabs.

Comment 28

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #27)
> (In reply to comment #25)
> 
> Please give me step-by-step STR.  I can't figure out your webcast.
> 
> I also tested with two tabs.

I reproduced this in Namoroka, so it's not a problem with your patch, but here are the STR:

1. Load https://bugzilla.mozilla.org/show_bug.cgi?id=510919
2. With the default tab prefs, click on the URL in bugzilla (http://www.coteindustries.com/articles/html_articles/tabindex.html), and then after it loads try command+w; without a subsequent, explicit click, you can't close that newly opened tab.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #28)

Now I see it -- your were clicking on the URL link at the top of the
bug (which opens the site in a new tab and triggers the bug), while I
was clicking on the URL in comment #0 (which doesn't).

You don't (initially) need to have a second tab to see this bug.  It
happens in both Minefield and Namoroka (with or without my patch).
And it's a focus bug -- cmd-n and cmd-t also don't work.

Thanks, Stephen, for the STR.  I'll spin this off into a new bug.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

I've spun off the STR from comment #28 into bug 511449.

Comment 31

[Josh Aas]

Comment on attachment 395083 [details] [diff] [review]
Fix for OS X bug

review -> mstange, he's been looking into related things recently

Comment 32

[Markus Stange [:mstange]]

Steven, does this patch still work for you on 10.5? It doesn't fix the bug for me on 10.6.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

When I tested my patch (from comment #19), it fixed the bug on both 10.5 and 10.6.

I'll test again tomorrow.  Possibly something else has changed in the meantime.

Comment 34

[Markus Stange [:mstange]]

Comment on attachment 395083 [details] [diff] [review]
Fix for OS X bug

Canceling review for now - I don't see any evidence that this patch has any effect, neither on 10.5 nor on 10.6.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 395083 [details] [diff] [review]
Fix for OS X bug

> I don't see any evidence that this patch has any effect, neither on
> 10.5 nor on 10.6.

My patch still works fine on the 1.9.2 branch, on both OS X 10.5.8 and
10.6.1 (using today's code).  But it doesn't work (it appears to make
no difference) on the trunk (mozilla-central).

Back when it was first posted (2009-08-18), my patch worked fine even
on the trunk (on both 10.5 and 10.6).  Markus, can you think which
change (on the trunk) might have made this patch stop working?

Comment 36

[Steven Michaud [:smichaud] (Retired)]

> Markus, can you think which change (on the trunk) might have made
> this patch stop working?

My bet is on one of the patches for bug 496601.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

> My bet is on one of the patches for bug 496601.

I was wrong.  Next most likely is the patch for bug 443178.

Comment 38

[Markus Stange [:mstange]]

(In reply to comment #35)
> Markus, can you think which
> change (on the trunk) might have made this patch stop working?

Offhand, no. But I'd rather search for patches related to focus than related to mouse events or cursers.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

> Next most likely is the patch for bug 443178.

It wasn't that patch, either.

Comment 40

[Markus Stange [:mstange]]

If I recall correctly, this bug was fixed by bug 511449.