Last Comment Bug 621117 - NSCursor class methods, SetThemeCursor() and SetCursor() not working for OOP plugins on mac
: NSCursor class methods, SetThemeCursor() and SetCursor() not working for OOP ...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Mac OS X
: -- major with 4 votes (vote)
: ---
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
: 587943 593857 602018 633034 642192 643768 644864 646593 650211 651616 653388 676406 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-23 01:16 PST by duncantebbs
Modified: 2013-04-02 09:34 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
v0.9 patch (work in progress) (43.30 KB, patch)
2011-04-13 17:02 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Review
v0.91 patch (work in progress) (43.40 KB, patch)
2011-04-13 20:56 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Review
v1.0 patch (fixes Flash native cursor animation) (47.86 KB, patch)
2011-04-18 14:47 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Review
v1.1 patch (hook SetCursor()) (51.67 KB, patch)
2011-04-22 15:30 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Review
v 1.2 patch (more fixes) (55.84 KB, patch)
2011-04-28 10:30 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Review
v 1.3 patch (fix context menu problem) (57.35 KB, patch)
2011-05-05 09:50 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Review
v 1.4 patch (add prefs to partly/fully disable native cursor support for OOP plugins) (62.08 KB, patch)
2011-05-12 09:50 PDT, Steven Michaud [:smichaud] (Retired)
cjones.bugs: review-
Details | Diff | Review
v 1.5 patch (address cjones' review comments) (62.74 KB, patch)
2011-05-19 16:26 PDT, Steven Michaud [:smichaud] (Retired)
jaas: review+
Details | Diff | Review
v 1.6 patch (follow Josh's suggestions) (62.29 KB, patch)
2011-05-20 16:56 PDT, Steven Michaud [:smichaud] (Retired)
benjamin: review+
Details | Diff | Review
Final patch for someone else to land (from landing queue) (62.48 KB, patch)
2011-05-23 20:32 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Review

Description duncantebbs 2010-12-23 01:16:58 PST
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

For a 32 bit plugin running out-of-process, the NSCursor apis don't seem to work for hiding / unhiding the mouse pointer.


Reproducible: Always

Steps to Reproduce:
[NSCursor hide];

Actual Results:  
Mouse pointer should disappear

Expected Results:  
Mouse pointer does not disappear

Works for in-process plugins.
I'm not aware of a work-around, so marking this major.
Comment 1 jonas echterhoff 2011-01-21 07:08:59 PST
I can confirm this bug in the Unity plugin. You probably need to intercept [NSCursor hide] messages and pass them on to the main process for it to work (both Safari and Chrome seem to do something like that), as cursor hiding needs to be done from the foreground process.
Comment 2 Josh Aas 2011-02-10 07:05:25 PST
*** Bug 633034 has been marked as a duplicate of this bug. ***
Comment 3 Stuart Morgan 2011-02-16 06:29:30 PST
Yes, Chrome swizzles the core NSCursor methods to pass the commands to the browser process:
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/plugin/plugin_interpose_util_mac.mm&q=nscursor&exact_package=chromium&sa=N&cd=16&ct=rc
Comment 4 Josh Aas 2011-02-22 17:06:39 PST
Steven - can you look into this when you get a chance?
Comment 5 Steven Michaud [:smichaud] (Retired) 2011-02-22 17:52:46 PST
Yes.  But it'll be at least a week before I can do so.
Comment 6 Josh Aas 2011-02-22 18:21:53 PST
(In reply to comment #5)
> Yes.  But it'll be at least a week before I can do so.

No problem. This is not a Firefox 4 blocker but I would like to follow up with a fix soon after the release.
Comment 7 Josh Aas 2011-02-23 10:47:15 PST
We should look into recommending that plugins use NPRuntime and the DOM to set cursors. Maybe set "this.style.cursor" and use "data:" URIs for custom cursors.
Comment 8 Benjamin Smedberg [:bsmedberg] 2011-03-07 07:07:08 PST
*** Bug 602018 has been marked as a duplicate of this bug. ***
Comment 9 Josh Aas 2011-03-14 02:44:07 PDT
*** Bug 593857 has been marked as a duplicate of this bug. ***
Comment 10 Alex Butin 2011-03-14 03:16:58 PDT
(In reply to comment #6)
> No problem. This is not a Firefox 4 blocker but I would like to follow up with
> a fix soon after the release.

Hey guys, are you kidding us!? Of course it is FF4 blocker, because this is absolutely necessary to differentiate clickable object! The world strictly based on underlined strings is gone many years ago, wake up!
Comment 11 Ed Morley [:emorley] 2011-03-14 04:51:30 PDT
For reference, the duped bug 593857 comment 1, gives the regression range for this as:

Last good nightly: 2010-04-05 
First bad nightly: 2010-04-06

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c4df407008b5&tochange=523d0f8e0926
Comment 12 Josh Aas 2011-03-14 05:09:37 PDT
We don't need any more information here, we know what is going on. Thanks though.
Comment 13 Markus Stange [:mstange] 2011-03-16 14:01:43 PDT
*** Bug 642192 has been marked as a duplicate of this bug. ***
Comment 14 Ed Morley [:emorley] 2011-03-22 11:57:26 PDT
*** Bug 643768 has been marked as a duplicate of this bug. ***
Comment 15 Ed Morley [:emorley] 2011-03-24 19:54:50 PDT
*** Bug 644864 has been marked as a duplicate of this bug. ***
Comment 16 Steven Michaud [:smichaud] (Retired) 2011-04-01 10:02:38 PDT
I'm working on this bug, and have a patch that works perfectly in my
own OOPP tests (for which I added calls to -[NSCursor set], +[NSCursor
hide] and +[NSCursor unhide] to my DebugEventsPlugin from bug 441880).

But it doesn't work with Flash (notably with the testcase from bug
643678, http://kirken.no/daap/), and it looks like I'm going to have
to spend some time figuring out why.

I should have more to say about this early next week.
Comment 17 Steven Michaud [:smichaud] (Retired) 2011-04-01 15:39:07 PDT
Michelle, it would be extremely useful to have one or more Flash
"movies" that comprehensively test what current versions of Flash can
do with the cursor on OS X.

Thanks in advance for whatever links you can provide!
Comment 18 Steven Michaud [:smichaud] (Retired) 2011-04-01 15:57:02 PDT
The following document references "native custom mouse cursors", which
it calls a new feature in Flash 10.2.

http://www.adobe.com/products/flashplayer/features/

Is it these "native custom mouse cursors" (and perhaps only these
cursors) that are broken in Firefox 4 on OS X?

So far all the Flash cursor examples that I've been able to find on my
own work just fine in Firefox 4 (testing in the	default 64-bit mode on
OS X 10.6.7 -- which runs Flash	out-of-process).
Comment 19 Michelle Sintov 2011-04-01 16:00:45 PDT
I will work on compiling a list of URLs. For now, here is one that doesn't work with Mac Firefox 4 and the latest released Flash Player version 10,2,153,1:

http://blog.andrevenancio.com/2010/01/08/change-mouse-cursor-in-flash10/
Comment 20 Steven Michaud [:smichaud] (Retired) 2011-04-01 16:16:00 PDT
> http://blog.andrevenancio.com/2010/01/08/change-mouse-cursor-in-flash10/

Thanks!  This does indeed work in Safari but not in Firefox 4 (on OS X 10.6.X, where both browsers run Flash out of process).  The cursor-handling features used by this example seem to be new with Flash 10.0.

But here's another example, this time of the "native custom mouse cursors" introduced in Flash 10.2.  These also work in Safari but not in Firefox 4.  But the "non-native" cursors (which you get by toggling the Native Cursors/Fake Cursors button) work fine in Firefox 4.

http://corlan.org/downloads/examples/custom-cursors/CustomCursors.html
Comment 21 Michelle Sintov 2011-04-04 16:30:54 PDT
The corlan.org link is indeed our new native mouse cursor feature.

Those two links may sufficiently cover our cursor tests for this bug fix. Once you have a build, we can test it internally. Thanks so much.
Comment 22 Steven Michaud [:smichaud] (Retired) 2011-04-13 17:02:58 PDT
Created attachment 525871 [details] [diff] [review]
v0.9 patch (work in progress)

Yes, I'm still working on this bug.  But it's been a lot more trouble
than I anticipated, and I'm still not finished.

It turns out that the Flash plugin doesn't just use NSCursor
class methods to manipulate the native cursor.  It also uses the
old Appearance Manager's SetThemeCursor() method, and the even
older (pre-Carbon) QuickDraw SetCursor() method.

In my tests, I find that the SetCursor() method works fine even
when called from the plugin child process (running in the
background).  But this isn't true for the SetThemeCursor()
method.  So I've had to add code to hook calls to
SetThemeCursor() in the plugin child process, and forward them to
the browser process (running in the foreground), using something
called "dyld interposing".

Chrome does the same thing.  So my current patch now works as well as
Chrome does with the Flash examples I've been testing with
(http://kirken.no/daap/,
http://blog.andrevenancio.com/2010/01/08/change-mouse-cursor-in-flash10/
and
http://corlan.org/downloads/examples/custom-cursors/CustomCursors.html).

But the CustomCursors example uses some kind of cursor animation,
which *doesn't* work properly in Chrome or with my patch.  It does
work properly in Safari.  So I feel obliged to at least try to get it
working in Firefox.

A tryserver build made with my patch should be available by tomorrow
morning.
Comment 23 Stuart Morgan 2011-04-13 20:55:43 PDT
(In reply to comment #22)
> But the CustomCursors example uses some kind of cursor animation,
> which *doesn't* work properly in Chrome or with my patch.

That's http://crbug.com/73885 FYI. It may be as simple as just swizzling that method as well, I just haven't had time to look into it.
Comment 24 Steven Michaud [:smichaud] (Retired) 2011-04-13 20:56:40 PDT
Created attachment 525915 [details] [diff] [review]
v0.91 patch (work in progress)

Oops, the previous patch didn't build.
Comment 25 Stuart Morgan 2011-04-13 21:00:48 PDT
Your patch doesn't appear to scrub the interposing back out of the environment in the plugin process; I strongly recommend doing so since plugins can (and do) spawn processes.
Comment 26 Steven Michaud [:smichaud] (Retired) 2011-04-14 08:27:33 PDT
> Your patch doesn't appear to scrub the interposing back out of the
> environment in the plugin process; I strongly recommend doing so
> since plugins can (and do) spawn processes.

I actually considered doing this, and decided against it.  But I
hadn't fully thought through what would happen if the
MacPluginChildSetThemeCursor() hook got called from a different
process than the plugin child process.  As the code is now written, no
harm would be done:  OnSetThemeCursorPtr would be NULL (since another
copy of libplugin_child_interpose.dylib would have been loaded into
the plugin's subprocess), and dlsym() would fail to initialize it.  So
my hook would just call the system's SetThemeCursor().  But it
wouldn't do any good, either.

I'll fix this in my next revision -- presumably the same way Chrome
does (by removing ../libplugin_child_interpose.dylib from
DYLD_INSERT_LIBRARIES just after the plugin child process has
started).
Comment 27 Steven Michaud [:smichaud] (Retired) 2011-04-14 08:51:08 PDT
For what it's worth, here's a tryserver build made with my v0.91 patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-00f2d6dd9089/tryserver-macosx64/firefox-6.0a1.en-US.mac.dmg
Comment 28 Markus Stange [:mstange] 2011-04-15 10:32:15 PDT
*** Bug 650211 has been marked as a duplicate of this bug. ***
Comment 29 Steven Michaud [:smichaud] (Retired) 2011-04-18 14:47:07 PDT
Created attachment 526834 [details] [diff] [review]
v1.0 patch (fixes Flash native cursor animation)

Here's a patch that fixes the Flash native cursor animation problem.

A tryserver build should follow in a few hours, or perhaps by tomorrow
morning.

I really should use a define for the name of the library I use for
dyld interposing (libplugin_child_interpose.dylib), and possibly also
for DYLD_INSERT_LIBRARIES.  But I can't figure out a good place to put
them.  I'm open to suggestions.

>> But the CustomCursors example uses some kind of cursor animation,
>> which *doesn't* work properly in Chrome or with my patch.
>
> That's http://crbug.com/73885 FYI. It may be as simple as just
> swizzling that method as well, I just haven't had time to look into
> it.

Thanks.  The key to the problem did turn out to be +[NSCursor
currentCursor].

At least on OS X 10.6 (and in Cocoa even mode), the Flash plugin
implements what amounts to an idle timer.  (I suspect this is a
replacement for the "null events" that used to be sent by the browser
in Carbon event mode.)  It creates a CFRunLoopSource (by calling
CFRunLoopSourceCreate()) with an 'order' of INT32_MAX (presumably to
indicate that it should run with a very low priority), which it adds
to the main thread's CFRunLoop (using CFRunLoopAddSource()).  Then it
periodically signals this CFRunLoopSource (using
CFRunLoopSourceSignal() and CFRunLoopWakeUp()) from a secondary thread
created by the Flash plugin.

While the Flash plugin is doing cursor animation, this idle timer's
"perform" target calls +[NSCursor currentCursor] every time it runs.
When it's time to display the animation's next "cell", the idle timer
(or more precisely a cursor-animation method called from the idle
timer) creates a new NSCursor object for the cell, and calls
-[NSCursor set] on it.

This all works fine when the Flash plugin runs in the browser process.
But when it runs in a background process, +[NSCursor currentCursor]
always returns NULL.  When the idle timer sees this, it figures
something is wrong and stops calling the cursor-animation method.

Interestingly, the cursor-animation code can also be called from the
Flash plugin's handler for mouse-moved events.  So if (in Chome or in
the tryserver build made from my v0.91 patch) you move the mouse
around over the Flash "movie", you'll see the animation advance a few
steps.
Comment 30 Steven Michaud [:smichaud] (Retired) 2011-04-18 14:50:40 PDT
Comment on attachment 526834 [details] [diff] [review]
v1.0 patch (fixes Flash native cursor animation)

I figure this patch should also be reviewed by someone who knows the OOPP code well.
Comment 31 Steven Michaud [:smichaud] (Retired) 2011-04-19 08:01:18 PDT
Here's a tryserver build made with my v1.0 patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-320e9bb28fc2/tryserver-macosx64/firefox-6.0a1.en-US.mac.dmg

Please test it!

Michelle, please ask your team to test with it.

Let us know your results.
Comment 32 Michelle Sintov 2011-04-19 10:28:09 PDT
Thanks Steven. We'll get started on testing this.
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 14:38:04 PDT
(In reply to comment #7)
> We should look into recommending that plugins use NPRuntime and the DOM to set
> cursors. Maybe set "this.style.cursor" and use "data:" URIs for custom cursors.

What's going on with this plan?  I'd really hate to have to maintain these hooks forever.  TBH I'd be fine with custom cursors being broken on mac until plugin vendors could implement this recommendation.

Also, the demo at corlan.org behaves differently on my windows and gtk2 machines: I don't seem to be getting custom cursors with gtk2 ("Flash 10.3 d162").  If we have a similar problem with custom cursors on gtk2 that requires similar hooks, we'll have increased the maintenance burden.  If serviceable cross-platform web APIs already exist, that new burden sounds very unattractive.
Comment 34 Benjamin Smedberg [:bsmedberg] 2011-04-20 14:42:30 PDT
The DOM stuff was proposed to plugin-futures and implemented for FF5.
Comment 35 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-20 14:53:41 PDT
Oh duh, I even remember seeing that discussion go by.

Do we really want this patch then?
Comment 36 Matthias Versen [:Matti] 2011-04-20 14:58:44 PDT
*** Bug 651616 has been marked as a duplicate of this bug. ***
Comment 37 Steven Michaud [:smichaud] (Retired) 2011-04-20 15:04:11 PDT
> Do we really want this patch then?

A lot depends on how soon Adobe can implement the new approach.  Anyone from Adobe care to comment?
Comment 38 Josh Aas 2011-04-20 15:04:34 PDT
Yes, we want this patch, or at least part of it (I don't know yet exactly how much stuff smichaud hooked). The backwards compat issue is a pretty big problem and vendors really want it fixed. We can set a deadline for removal of the hooks based on a reasonable timeline for moving to DOM cursors.
Comment 39 Steven Michaud [:smichaud] (Retired) 2011-04-20 15:24:15 PDT
Looks like I've got more work to do on my patch -- it doesn't (yet) work with the testcase from bug 651616 (which was marked as a dup of this bug in comment #36).
Comment 40 Steven Michaud [:smichaud] (Retired) 2011-04-20 16:27:41 PDT
Comment on attachment 526834 [details] [diff] [review]
v1.0 patch (fixes Flash native cursor animation)

> In my tests, I find that the SetCursor() method works fine even when
> called from the plugin child process (running in the background).

This turns out to be not quite true.  So my patch will (like Chrome) also
need to hook SetCursor(), in order to make the testcase for bug 651616 work
properly.

The reason I didn't notice this earlier is that the problems with
SetCursor() are more subtle in the testcase I was previously using for it
(http://corlan.org/downloads/examples/custom-cursors/CustomCursors.html)
than they are in the testcase for bug 651616.
Comment 41 Steven Michaud [:smichaud] (Retired) 2011-04-22 15:30:05 PDT
Created attachment 527871 [details] [diff] [review]
v1.1 patch (hook SetCursor())

Here's a patch that adds a hook for SetCursor() (the QuickDraw call),
and seems to perform entirely correctly in the two testcases I have of
SetCursor()-generated cursors -- the "fake cursors" in
http://corlan.org/downloads/examples/custom-cursors/CustomCursors.html
and the test case for bug 651616
(http://www.playercore.com/Users/rkwon/Bugs/Mozilla/MouseHide.html).

For a while I thought that SetCursor() worked correctly from the
background process, and didn't need a hook.  But after seeing the
testcase for bug 651616, I examined the "fake cursors" from
CustomCursors.html more carefully, and noticed that the SetCursor()
cursor (set from the background process) gets superimposed over the
current NSCursor from the foreground process.  I fixed this by making
sure that whenever SetCusor() is called in the background process, we
call [NSCursor set] on a transparent cursor (of the same size and with
the same hot-spot) in the foreground process.

This seems to fix the problem in the testcases I have available.  But
I very much want others to test, too -- and to report any problems
here.

I've been waiting several hours for a tryserver build to finish, but
it looks like the tryservers have (temporarily) ground to a halt.  So
I probably won't be able to provide one before Monday.

Once it's available, I'll need the Adobe folks to start testing with
it.
Comment 42 Steven Michaud [:smichaud] (Retired) 2011-04-25 07:53:47 PDT
Here's a tryserver build made with my latest (v1.1) patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-52c6502daf0b/try-macosx64/firefox-6.0a1.en-US.mac.dmg

Please test with it!  Especially the folks from Adobe.
Comment 43 Steven Michaud [:smichaud] (Retired) 2011-04-25 08:48:56 PDT
*** Bug 646593 has been marked as a duplicate of this bug. ***
Comment 44 Benoit Girard (:BenWa) 2011-04-25 10:43:57 PDT
*** Bug 587943 has been marked as a duplicate of this bug. ***
Comment 45 Robert Kwon 2011-04-26 13:29:09 PDT
I am seeing this test case still failing in 64-bit mode using the build with the latest v1.1 patch applied.

http://www.playercore.com/Users/rkwon/Bugs/Mozilla/MouseHide.html

I'm still in the process of running our suite of Flash based mouse cursor tests.  I should be done by the end of day tomorrow.
Comment 46 Steven Michaud [:smichaud] (Retired) 2011-04-26 13:36:17 PDT
> I am seeing this test case still failing in 64-bit mode using the
> build with the latest v1.1 patch applied.
>
> http://www.playercore.com/Users/rkwon/Bugs/Mozilla/MouseHide.html

Please be very specific about how it fails.

That test seemed to work fine for me (with my v1.1 patch) when I tried
it.

Also let us know what version of Flash you're testing with.
Comment 47 Steven Michaud [:smichaud] (Retired) 2011-04-26 13:59:50 PDT
I just tested
http://www.playercore.com/Users/rkwon/Bugs/Mozilla/MouseHide.html
again with the tryserver build from comment #42, and saw no problems.

Please tell us *exactly* what went wrong in your tests.

Please also make sure you're testing with the right build :-)

(I tested in the default 64-bit mode on OS X 10.6.7.  I tested with
Flash 10.2.159.1, which just came out in the last few weeks.
Previously I'd been testing with Flash 10.2.153.1.)
Comment 48 Robert Kwon 2011-04-26 15:02:10 PDT
My testing confirms that the patched Firefox 4 build works with 10.2.159.1 and 10.2.153.1, but we don't have 64-bit Flash Player support in these versions of the player.

I was testing with our Flash Player "incubator" player found here: http://labs.adobe.com/technologies/flashplatformruntimes/incubator/

The test works in Safari 5, but not with this nightly FF4 build.
Comment 49 Steven Michaud [:smichaud] (Retired) 2011-04-26 15:11:22 PDT
For the record, I just tested my v1.1 patch on Apple's current OS X 7
(Lion) developer preview (Build 11A430e) with the following testcases,
and had no trouble (that I could see) with any of them.  I used Flash
version 10.2.159.1.

http://kirken.no/daap/
http://blog.andrevenancio.com/2010/01/08/change-mouse-cursor-in-flash10/
http://corlan.org/downloads/examples/custom-cursors/CustomCursors.html
http://www.playercore.com/Users/rkwon/Bugs/Mozilla/MouseHide.html
http://stockcharts.com/h-sc/ui?s=AAPL&p=D&b=5&g=0&id=0&r=8139&depth=24&listNum=&cmd=chartnotesflash,739|623
Comment 50 Sunil 2011-04-26 15:17:21 PDT
Steven, when we tried the nightly build mentioned in comment #42, the browser
forced us to launch the browser in 32-bit mode. In this mode Cursors worked.
The original bug https://bugzilla.mozilla.org/show_bug.cgi?id=633034, was
opened stating that Cursor changes weren't happening while running the
Silverlight plugin in 64-bit Firefox4. 

Can you share out a nightly build that doesn't force us to get in 32-bit mode?
Comment 51 Steven Michaud [:smichaud] (Retired) 2011-04-26 15:18:42 PDT
(In reply to comment #48)

Thanks for the info.

> I was testing with our Flash Player "incubator" player found here:
> http://labs.adobe.com/technologies/flashplatformruntimes/incubator/

I'll test with this.  But I already know that our hooking/swizzling
code won't work with a 64-bit plugin (when it's run out-of-process).

I assume a 64-bit plugin will only use NSCursor class methods to
manipulate the native cursor.  If so it shouldn't be hard to make my
patch work with it.

New patch coming up, probably sometime tomorrow.
Comment 52 Robert Kwon 2011-04-26 15:26:43 PDT
I tested the 64-bit Flash Player incubator build with OS X 10.6.7 and Safari 5.0.4.  I haven't tried with OS X Lion, but I wanted to limit testing currently to publicly released operating systems and browsers.
Comment 53 Steven Michaud [:smichaud] (Retired) 2011-04-26 15:29:20 PDT
(In reply to comment #50)

This is a Silverlight bug, which I can't fix.  But I do have a
workaround, which follows.

The problem is that Silverlight sniffs the browser's user-agent string
to find out whether or not it's running in Firefox, but gets confused
by the user-agent string used by recent trunk nightlies (and also by
my tryserver builds).  If Silverlight thinks its running in Firefox,
it uses the Cocoa event model; otherwise it uses the Carbon event
model.  When Silverlight runs in recent trunk nightlies (or my
tryserver builds), it no longer thinks it's running in Firefox, and
uses the Carbon event model.  If Silverlight uses the Carbon event
model, Firefox 4.0 and up will force it to run in 32-bit mode.

The workaround is to make my tryserver builds use Firefox 4.0's user
agent string.  One way to do this is described at bug 619217 comment
#12.  Get the Firefox 4 user-agent string by running FF 4 and going to
a link such as http://browserspy.dk/browser.php.
Comment 54 Steven Michaud [:smichaud] (Retired) 2011-04-26 16:04:52 PDT
(Following up comment #51)

>> I was testing with our Flash Player "incubator" player found here:
>> http://labs.adobe.com/technologies/flashplatformruntimes/incubator/
>
> I'll test with this.  But I already know that our hooking/swizzling
> code won't work with a 64-bit plugin (when it's run out-of-process).

I've now tested with it, and seen that the
http://www.playercore.com/Users/rkwon/Bugs/Mozilla/MouseHide.html test
does fail.  The "fake cursor" tests in
http://corlan.org/downloads/examples/custom-cursors/CustomCursors.html
also fail, exactly the same way my v1.0 patch failed -- the "fake
cursor" gets superimposed over the (previously) current NSCursor.

I was wrong about the reason, though.  I'll have more to say tomorrow.

The other tests work fine, by the way.
Comment 55 Sunil 2011-04-26 16:24:18 PDT
Thanks Steven. I hadn't realized that. Switching the UA string got the plugin to load in 64 bit browser build. Now we are seeing the default cursors working whereas the custom NSCursors that we create ourselves and pass onto Firefox doesn't render in 64-bit mode. The Cursors that do not render are Wait, None, Stylus, Eraser, SizeNWSE, SizeNESE. The repro files for bug 633034 show all the cursors that should work. If you run the repro in 32-bit mode, you can see that all the cursors work.
Comment 56 Steven Michaud [:smichaud] (Retired) 2011-04-26 17:09:20 PDT
(In reply to comment #55)

You're telling me that some of the examples from bug 633034 don't work
even with my v1.1 patch (after playing the FF4-user-agent-string
trick)?

Sigh.

But at least now I'm starting to get a reasonable number of testcases,
and to know what I'm up against.
Comment 57 Josh Aas 2011-04-26 17:10:22 PDT
I don't think it is worth adding support for non-OS cursors (custom cursors). Firefox 5 will allow that via the DOM. This backwards compat fix may not even ship for Firefox 4.

https://wiki.mozilla.org/NPAPI:DOMCursors
Comment 58 Sunil 2011-04-26 17:14:30 PDT
comment #56. Steven, that is correct. While your changes fix so that the default cursors show up but the custom cursors do not show up with your fix.
Comment 59 Steven Michaud [:smichaud] (Retired) 2011-04-26 17:21:52 PDT
It'd be silly to decide now what we should fix and what we shouldn't,
before we really know the scope of the problem, and what kinds of
fixes might be required.

I should have more to say tomorrow, once I've had a chance to work
with Sunil's examples, which are completely new to me.
Comment 60 Sunil 2011-04-26 17:30:07 PDT
Steven, let me know if you have any issues with running the repro.
Comment 61 Steven Michaud [:smichaud] (Retired) 2011-04-26 18:11:21 PDT
Here's a very important question for anyone and everyone on this bug:

As of this afternoon, we know that two popular plugins (Flash and
Silverlight) manipulate the "native" cursor on OS X in ways that don't
work properly when called from a background process.  Furthermore we
know that a not-yet-released version of Flash does this in ways
substantially different from what the release versions do.

Are there any more surprises?

Is there, for example, a non-yet-released version of the Silverlight
plugin that manipulates OS X native cursors in significantly different
ways?

Are there other reasonably popular plugins that manipulate the native
cursor on OS X?
Comment 62 Sunil 2011-04-26 18:22:24 PDT
> Is there, for example, a non-yet-released version of the Silverlight
plugin that manipulates OS X native cursors in significantly different
ways?

Steven, while I cannot comment on future major releases, to my knowledge we are waiting for fixes from Mozilla for fixes in this area and there are no changes planned in this area in future service updates of Silverlight 4. 

bug #633034, was duped to this bug which I originally opened and have traced this bug as a result of the dupe. Unfortunately looking at the bug template, I don't see if there is a way of determining all the bugs duplicated to this bug. I apologize if I should have brought this up earlier but I didn't realize that you weren't aware of the Silverlight related bug.
Comment 63 Josh Aas 2011-04-26 22:25:27 PDT
I think the top damage-control priority here is to make the little hand work on links in Flash and Silverlight, and to allow the cursor to return to the standard cursor when not over a link. The next priority is to allow them to hide the cursor. The rest is bonus points at the moment. Shipping custom cursor image data over IPC to be displayed from the browser process is probably going too far. That can wait for a plugin update in which they accomplish it via the DOM.

I don't want to delay a fix for links, standard cursor, and hiding for the sake of anything else, and I don't want to ship cursor image data over IPC at all.
Comment 64 Steven Michaud [:smichaud] (Retired) 2011-04-27 08:08:04 PDT
> I don't want to ship cursor image data over IPC at all

Why not?  DOM cursors will presumably also need to have cursor image
data shipped over IPC (using data: URLs or otherwise).
Comment 65 Robert Kwon 2011-04-27 11:48:25 PDT
I did some more testing on the Flash Player side and here is what I found:

32-bit plugin (10.2.159.1) on OS X 10.6.7

Native Mouse cursor issue is fixed and mouse hide/unhide works properly as well.  The cursor also properly switches between AUTO/BUTTON/HAND/IBEAM, etc.

64-bit plugin (Flash Player incubator) on OS X 10.6.7

Native Mouse cursor works but there is a side effect when you right-click to bring up a context menu.  In FF3 and various other browsers, the cursor will go back to the system OS cursor from the custom native cursor.  However, in your nightly patch build, the custom cursor will remain even over the context menu.  I never exactly knew what the expected behavior should be, but I went with consistency and most of the browsers seem to kick the custom cursor back to the OS cursor when over context menus.

http://corlan.org/downloads/examples/custom-cursors/CustomCursors.html

In addition, when you do mouse hiding with custom cursors, instead of hiding the mouse, the custom cursor will go away, but the OS cursor will remain showing.  I assume this is because of the earlier comments I saw where it stated the system cursor is always hiding behind the custom cursor.  Is this how this is supposed to work?  Will the OS cursor always be hiding behind the custom one?  I've seen the custom cursor tests that use large cursors, but what if the cursor is smaller?  Won't the cursor behind the custom one be showing?

Mouse hide/unhide with basic cursors isn't properly working with the following test

http://www.playercore.com/Users/rkwon/Bugs/Mozilla/MouseHide.html

Let me know if you need any other info.
Comment 66 Josh Aas 2011-04-27 14:28:52 PDT
(In reply to comment #64)

> Why not?  DOM cursors will presumably also need to have cursor image
> data shipped over IPC (using data: URLs or otherwise).

Priorities. I don't think it is worth the time and the risk to add code for doing it via another mechanism. We'd also have to pay special attention to performance implications. DOM cursors already work. If the current patch here works for the regression priorities I mentioned before then I think we should go with it ASAP.
Comment 67 Steven Michaud [:smichaud] (Retired) 2011-04-27 16:47:34 PDT
I've now got a patch that fixes all reported problems except the
problem with the context menu reported in comment #33 (which I suspect
will be rather difficult to fix).

I'll do a tryserver build overnight, and post it tomorrow.

Tomorrow I'll also post variously crippled versions of my patch, with
accompanying tryserver builds.  People can test with these, and see
what they'd be willing to live with.
Comment 68 Steven Michaud [:smichaud] (Retired) 2011-04-27 16:59:09 PDT
> I've now got a patch that fixes all reported problems except the
> problem with the context menu reported in comment #33 (which I suspect
> will be rather difficult to fix).

The context menu problem was reported in comment #65.
Comment 69 Robert Kwon 2011-04-27 17:16:28 PDT
The context menu problem we might be able to live with.  FF3 doesn't do this, so I assume this is the wrong behavior, but it's not like a user can't select menu items still.  Clicking still works even though the custom mouse icon appears even while hovering over context menus.

Is the problem really difficult to fix that it would have to wait until the next dot release of Firefox?
Comment 70 Steven Michaud [:smichaud] (Retired) 2011-04-27 17:23:22 PDT
> Is the [context menu] problem really difficult to fix that it would
> have to wait until the next dot release of Firefox?

I don't even know how difficult it would be to fix ... which is a bad
sign.

And given that the powers-that-be don't even like many parts of my
patch that *do* work, I expect the only way the context menu problem
will ever get fixed is by plugin vendors moving to the DOM cursors
model (as partially described at
https://wiki.mozilla.org/NPAPI:DOMCursors).  In other words, only if
the context menu problem persists with DOM cursors are "we" likely to
spend any time fixing it.
Comment 71 Steven Michaud [:smichaud] (Retired) 2011-04-27 17:29:11 PDT
(Following up comment #70)

And the context menu problem may be partly a Flash bug -- it doesn't happen with Silverlight.
Comment 72 Thomas Ahlblom 2011-04-28 06:23:51 PDT
*** Bug 653388 has been marked as a duplicate of this bug. ***
Comment 73 Steven Michaud [:smichaud] (Retired) 2011-04-28 10:30:12 PDT
Created attachment 528896 [details] [diff] [review]
v 1.2 patch (more fixes)

Here's the new, full version of my patch I promised in comment
#67.

And here's a tryserver build made from it:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-367de46d2323/try-macosx64/firefox-6.0a1.en-US.mac.dmg

Please test it as soon as possible -- especially the Adobe and
Silverlight folks.  And please let us know (in detail) whatever
problems you find.  Some I may be able to fix quickly -- those
can fall within the scope of this bug.  Others (like the context
menu bug mentioned by Robert Kwon in comment #65) I won't be able
to fix quickly, and will need to have other bugs opened on
them (presuming they still exist in whatever part of my patch
does finally get landed).

Thanks in part to Robert Kwon's having told me about the 64-bit
incubator version of Flash, I discovered that I was wrong about
how Flash (release and incubator versions) does non-native custom
cursors (for example the "fake cursors" in the
http://corlan.org/downloads/examples/custom-cursors/CustomCursors.html
testcase).  Apparently the cursor graphic (e.g. the Christmas
tree/arrow combination used in CustomCursors) isn't a cursor at
all, but an overlay that follows the mouse around.  When using
non-native cursors, the only native-cursor manipulation performed
by the Flash plugin is to make the native cursor transparent --
using SetCursor() in the 32-bit release versions, and in the
64-bit incubator version using a call to [NSCursor set] on an
NSCursor object whose "image" has no "representations".  Some of
the changes in my new patch are due to my having discovered this.

Flash uses [NSCursor set] to implement native custom
cursors.  (Silverlight uses [NSCursor set] to implement custom
cursors of any kind.)

I also discovered that Silverlight uses CGDisplayHideCursor() and
CGDisplayShowCursor() (documented Quartz methods) to hide and
show the native cursor.  So I needed to hook these to get
hiding/showing the cursor to work in Silverlight.
Comment 74 Steven Michaud [:smichaud] (Retired) 2011-04-28 10:39:55 PDT
> (Silverlight uses [NSCursor set] to implement custom cursors of
> any kind.)

Oops, this is incorrect.  All the custom cursors in the testcase
from bug 633034 use SetCursor() (the QuickDraw method).
Comment 75 Robert Kwon 2011-04-28 11:50:35 PDT
The latest version of the patch looks good based on testing from the Adobe side.  Basic cursor hiding and native mouse cursor hide/unhide works with the Flash Player incubator build.  

It looks like the only issue is with context menus.  Even with non-native flash based cursors, if you right click and bring up the context menu, the cursor will disappear.  This is different than native cursors, where the custom cursor will remain while hovering over the context menu.

I know this is a difficult issue that may not be able to be fixed soon and is not within the scope of this bug.  Should I open a new bug for this?  I'm also opening up an internal Adobe bug to have a dev look into this on our side to confirm this isn't our bug for sure.
Comment 76 Steven Michaud [:smichaud] (Retired) 2011-04-28 12:30:42 PDT
(In reply to comment #75)

> The latest version of the patch looks good based on testing from the
> Adobe side.  Basic cursor hiding and native mouse cursor hide/unhide
> works with the Flash Player incubator build.

I'm glad to hear it.  But please be sure to also test with the release
version of the Flash plugin -- that's actually much more important for
our purposes.

> Even with non-native flash based cursors, if you right click and
> bring up the context menu, the cursor will disappear.  This is
> different than native cursors, where the custom cursor will remain
> while hovering over the context menu.

I also noticed this, and find that it happens with both the incubator
build and the release version.  Having the cursor disappear (as it
does on right-click with non-native custom cursors) is a much worse
problem than having it not turn back into an arrow cursor (as it does
with native custom cursors).  It may be worth it to desupport custom
cursors (partially or completely) to make this problem go away (if
that's possible).  This is one of the things I'll be working on.

> Should I open a new bug for this?

Only if the problem still happens with whatever part of my patch gets
landed.

> I'm also opening up an internal Adobe bug to have a dev look into
> this on our side to confirm this isn't our bug for sure.

Thanks, and glad to hear it.
Comment 77 Robert Kwon 2011-04-28 12:44:11 PDT
I tested with the release version as well and things are working as expected.
Comment 78 Sunil 2011-04-29 11:53:11 PDT
Steven, I'm glad to say that the latest patch, fixes the custom cursor issue while also retaining the fix for original part of this bug.
Comment 79 Robert Kwon 2011-05-03 14:59:03 PDT
(In reply to comment #76)
> (In reply to comment #75)
>
> > Even with non-native flash based cursors, if you right click and
> > bring up the context menu, the cursor will disappear.  This is
> > different than native cursors, where the custom cursor will remain
> > while hovering over the context menu.
> 
> I also noticed this, and find that it happens with both the incubator
> build and the release version.  Having the cursor disappear (as it
> does on right-click with non-native custom cursors) is a much worse
> problem than having it not turn back into an arrow cursor (as it does
> with native custom cursors).  It may be worth it to desupport custom
> cursors (partially or completely) to make this problem go away (if
> that's possible).  This is one of the things I'll be working on.
> 
> > Should I open a new bug for this?
> 
> Only if the problem still happens with whatever part of my patch gets
> landed.
> 
> > I'm also opening up an internal Adobe bug to have a dev look into
> > this on our side to confirm this isn't our bug for sure.
> 
> Thanks, and glad to hear it.

I did have an Adobe dev look at this and for Flash Native mouse cursors, we will now put in a fix that will convert the custom native cursor back to an arrow cursor when the context menu is triggered.  The dev informed me that Safari and Chrome do this through the browser, so will Mozilla consider changing behavior to become consistent with Chrome and Safari?

This only fixes Flash Native mouse cursors and context menus and it doesn't fix the issue with the cursor disappearing for NON-Native flash based cursors when activating the context menu.
Comment 80 Steven Michaud [:smichaud] (Retired) 2011-05-03 16:35:17 PDT
I've found a potential browser-side fix for the context-menu bug.
I've also found out why it doesn't happen in Chrome.  I'll have more
to say about this tomorrow.  (I've taken the last couple of days off,
to run various kinds of errands.  So I haven't done any work on this
bug since last week.)

As for Safari, I suspect it somehow manages to run the plugin process
in the foreground whenever this is needed, and for that reason doesn't
suffer from any of the problems reported here.  I don't think this is
a viable option for either Firefox or Chrome ... at least not in the
short or medium term.
Comment 81 Steven Michaud [:smichaud] (Retired) 2011-05-05 09:50:47 PDT
Created attachment 530351 [details] [diff] [review]
v 1.3 patch (fix context menu problem)

As promised, here's a patch that fixes the context menu problem on the
browser side.  And here's a tryserver build made from it:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-eb30280d23cb/try-macosx64/firefox-6.0a1.en-US.mac.dmg

Turns out the context menu bug only happens if the plugin uses
NPN_PopUpContextMenu(), which Flash does and Silverlight doesn't
(Silverlight's context menu is displayed using ContextualMenuSelect(),
from the pre-Carbon Menu Manager API).  It also only happens if the
plugin process is the background process while the context menu is
displayed.  This is why it happens in Firefox but not in Chrome (which
foregrounds the plugin process while the context menu is displayed) --
whenever a process is moved to the foreground, an (undocumented)
NSEvent gets sent, and when this event is processed (by the OS)
[[NSCursor arrowCursor] set] gets called.

Adobe and Silverlight folks, please test this new build.  It's the
same as the v1.2 build except for the fix/workaround for the context
menu problem, so it probably doesn't need a full round of tests.
Comment 82 Robert Kwon 2011-05-05 13:57:53 PDT
I tested your new nightly build with Flash Native mouse cursors and things all look good now.  Thanks Steve!
Comment 83 Steven Michaud [:smichaud] (Retired) 2011-05-12 09:50:43 PDT
Created attachment 531959 [details] [diff] [review]
v 1.4 patch (add prefs to partly/fully disable native cursor support for OOP plugins)

OK, here's what I hope is my final patch ... or very close to it.  It
adds a dom.ipc.plugins.nativeCursorSupportLevel pref with three
possible values:

'0' means no support (the current situation)
'1' means support for preset cursors, and for hiding/showing the cursor
'2' means full support (including for custom cursors)

Only when dom.ipc.plugins.nativeCursorSupportLevel is '2' do we
serialize any graphical images over IPC.

I very much want the default for this setting to be '2', but that
might not be how things turn out (more about this below).

Here's a tryserver build made with my patch:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-550d6626ca13/try-macosx64/firefox-6.0a1.en-US.mac.dmg

Please test this patch, with particular attention to whether the
different support levels match the documentation for them that I've
given above.  (They do in my tests; I just want to be sure.)

There are a couple of problems with what I'm trying to do here (with
my patches), which makes this bug very difficult to deal with:

1) There are many different OS calls to manage the cursor.  Unless we
   bring the plugin process to the foreground whenever a plugin might
   call them (not feasible anytime soon), we need to hook them and
   pass information over IPC from the plugin process to the browser
   process.

   But this inevitably makes my patches complex, both techically and
   "politically".  I might have tried to hook *all* OS calls that
   might be used to manage native cursors.  But I'm not sure I can
   find them all, and in any case it's difficult to test hooks for
   calls not currently used by any known plugin.  So instead I've
   chosen to hook only calls used by known plugins (currently Flash
   and Silverlight), and test those hooks as thorougly as possible.

   This means my patch is very ad-hoc:  I know it works well with
   Flash and Silverlight (at least with the testcases others have
   given me, and which I've been able to find on my own).  But there's
   no guarantee it'll work with some other plugin (as yet unknown to
   me) that uses OS calls to manipulate native cursors.

2) There's a lot of resistance to what I'm trying to do among what
   I've been calling the "powers that be".  I don't believe this
   resistance is justified -- in other words, I believe my patches'
   benefits to users outweigh their problems.  But these problems are
   undeniable, and the solution I prefer may get rejected.

   The biggest problem is (I suppose) my patches' complexity.

   Another is that (as I've pointed out above) I can't guarantee that
   my patches' are a complete solution -- even on OS X.  And (as Chris
   pointed out in comment #33), if my patches land on OS X, we might
   feel obliged to come up with something equally complex (and ad-hoc)
   for GTK2.

   Still another is that for my hooks to support custom cursors, they
   need to serialize graphical images over IPC.  As I've already
   pointed out, I don't think this objection has much merit -- even
   supporting DOM cursors will require us to send image data over IPC.
   But using my hooks to do it does add some (small) amount of extra
   complexity.

Finally, my patches (even if they're accepted) are only a temporary
solution.  DOM cursors are much better -- both for their simplicity
and for their availability on all platforms.  We'll still want to drop
my patches as soon as this is feasible -- technically and politically.
Comment 84 Steven Michaud [:smichaud] (Retired) 2011-05-13 12:27:41 PDT
Flash 10.3.181.14 was just released for OS X.  I tested my most recent
tryserver build with it (the one with my v1.4 patch from comment #83),
and everything seems to be fine.
Comment 85 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-15 16:07:50 PDT
Comment on attachment 531959 [details] [diff] [review]
v 1.4 patch (add prefs to partly/fully disable native cursor support for OOP plugins)

>+// This pref governs whether (and to what extent) we attempt to work around
>+// problems caused by plugins using OS calls to manipulate the cursor while
>+// running out-of-process.  These workarounds all involve intercepting
>+// (hooking) certain OS calls in the plugin process, then arranging to make
>+// certain OS calls in the browser process.  Eventually plugins will be
>+// required to use the NPAPI to manipulate the cursor, and these workarounds
>+// will be removed.  See bug 621117.

I guess the pref here is the "exit strategy" for this code, wrt
release schedule?  The tristate is fine, but do you foresee a
situation in which we would drop 2->1 before releasing?  (This is
something for Josh to review, really.)

Please file a followup on removing this code and mention it here.

>+  // OS X Specific calls to allow the plugin to manage the cursor.
>+  async PluginSetCursor(NSCursorInfo cursorInfo);
>+  async PluginShowCursor(bool show);
>+  async PluginPushCursor(NSCursorInfo cursorInfo);
>+  async PluginPopCursor();

Nit: drop the "Plugin" prefix from these, it's clear from the message
direction (plugin->browser).

>diff --git a/dom/plugins/ipc/PluginInterposeOSX.h b/dom/plugins/ipc/PluginInterposeOSX.h

(I'm not able to review this code.)

>+    // Trigger "dyld interposing" for the dylib that contains
>+    // plugin_child_interpose.mm.  This allows us to hook OS calls in the
>+    // plugin process (ones that don't work correctly in a background
>+    // process).  Don't break any other "dyld interposing" that has already
>+    // been set up by whatever may have launched the browser.
>+    const char* prevInterpose = PR_GetEnv("DYLD_INSERT_LIBRARIES");
>+    nsCString interpose;
>+    if (prevInterpose) {
>+      interpose.Assign(prevInterpose);
>+      interpose.AppendLiteral(":");
>+    }
>+    interpose.Append(path.get());
>+    interpose.AppendLiteral("/libplugin_child_interpose.dylib");
>+    newEnvVars["DYLD_INSERT_LIBRARIES"] = interpose.get();

This is pretty bad, because it's going to force all subprocess types
(content, jetpack, etc.) to pay a penalty for the plugin hooks.  We've
lost the process type by the time we reach this code, so there's not
really a better way to do this atm, but can you please file a followup
on limiting this to plugin subprocesses?  I fear the patch will
require significant refactoring so it definitely shouldn't block this.

>+
>+    // Plugin child processes have no access to the pref service.  So pass
>+    // the current value of the native cursor support level setting (needed in
>+    // PluginInterposeOSX.mm) in an environment variable.
>+    PRInt32 cursorSupportLevel = 0;
>+    nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+    if (prefs) {
>+      if (NS_FAILED(prefs->GetIntPref("dom.ipc.plugins.nativeCursorSupportLevel",
>+          &cursorSupportLevel))) {
>+        cursorSupportLevel = 0;
>+      }
>+    }
>+    char cursorSupportLevelBuf[3];
>+    PR_snprintf(cursorSupportLevelBuf, sizeof(cursorSupportLevelBuf),
>+                "%d", cursorSupportLevel);
>+    newEnvVars["MOZ_IPC_PLUGINS_NATIVECURSORSUPPORTLEVEL"] = cursorSupportLevelBuf;

Yuck, but thankfully there's a better way to do this: grab the value
on-demand through a 'sync' PPluginModule message, say
PPluginModule:GetNativeCursorSupportLevel, that has the browser read
the value of this pref.  That will limit the scope of this hack to
PPluginModule.

r- for last comment above.  Please request rereview from bsmedberg;
I'm going on vacation tomorrow, and he's a better reviewer for the
build stuff to boot.
Comment 86 Josh Aas 2011-05-15 18:17:46 PDT
(In reply to comment #85)

> I guess the pref here is the "exit strategy" for this code, wrt
> release schedule?  The tristate is fine, but do you foresee a
> situation in which we would drop 2->1 before releasing?  (This is
> something for Josh to review, really.)

I think we should do support level 1 with no prefs (no custom image support). All code is a liability, and in this case I don't think the custom cursor code is worth it. In particular it seems like it has the potential to contribute to performance issues depending on how often plugins set custom cursors.
Comment 87 Steven Michaud [:smichaud] (Retired) 2011-05-16 09:39:11 PDT
(In reply to comment #86)

> All code is a liability, and in this case I don't think the custom
> cursor code is worth it. In particular it seems like it has the
> potential to contribute to performance issues depending on how often
> plugins set custom cursors.

Yes, all code is a liability.  But this is true for *every* patch, and
*every* change.  In order to make a decision that's not simply
arbitrary, you need to give your reasons.  You *have* given one reason
-- you're worried about sending custom cursor data over IPC, and
particularly about the possible impact on performance of doing this.
But, as I've already said, using DOM cursors will also involve sending
custom cursor data over IPC, so this objection has no merit.
Comment 88 Steven Michaud [:smichaud] (Retired) 2011-05-16 10:01:29 PDT
(In partial reply to comment #85)

> I guess the pref here is the "exit strategy" for this code, wrt
> release schedule?

Yes.

> The tristate is fine, but do you foresee a situation in which we
> would drop 2->1 before releasing?

Personally I don't.  If Josh hadn't been making a fuss about custom
cursors, my flag would have had two states -- 'on' and 'off'.  Still,
though, I suppose there *is* a (small) possibility that the code (in
my patch) that supports custom cursors might have bugs.  So it may be
safer to be able to quickly and easily turn off just that part of my
patch.

> Nit: drop the "Plugin" prefix from these, it's clear from the
> message direction (plugin->browser).

Will do.
Comment 89 Steven Michaud [:smichaud] (Retired) 2011-05-16 10:11:46 PDT
(In partial reply to comment #85)

> This is pretty bad, because it's going to force all subprocess types
> (content, jetpack, etc.) to pay a penalty for the plugin hooks.
> We've lost the process type by the time we reach this code, so
> there's not really a better way to do this atm, but can you please
> file a followup on limiting this to plugin subprocesses?  I fear the
> patch will require significant refactoring so it definitely
> shouldn't block this.

The hooks don't impose any *performance* penalty.  Calls to methods
implemented in other libraries go through a call table maintained by
dyld.  Initially, many entries in this table point to code in dyld
that (when first called) sets the entry to its final value.  So there
*is* a performance hit the first time any of these (externally linked)
methods is called.  But you also get a performance hit if you don't
use "dyld interposing" (i.e. if you don't hook these methods).  And
there's strong evidence that this performance hit is no larger with
hooks than without them (my patch for bug 533001, which does a
different kind of dyld interposing (on a lower level), and contains
code that's called hundreds of times on startup, caused no regression
in TS startup -- in fact it may have caused a slight improvement).

That said, we don't want or need to add these hooks to subprocesses
other that plugin subprocesses (or extension subprocesses, if we ever
have them).

It'd be possible to set DYLD_INSERT_LIBRARIES in code that calls down
to GeckoChildProcessHost::PerformAsyncLaunchInternal(), then unset it
immediately afterwards.  This would be functionally equivalent to what
my patch does now, and might allow us to easily limit my hooks to
plugin subprocesses.

Is this something that could be done in current code?  What do you
think, bsmedberg?
Comment 90 Steven Michaud [:smichaud] (Retired) 2011-05-16 10:51:04 PDT
(In partial reply to comment #85)

> Yuck,

Really?

> but thankfully there's a better way to do this: grab the value
> on-demand through a 'sync' PPluginModule message, say
> PPluginModule:GetNativeCursorSupportLevel, that has the browser read
> the value of this pref.  That will limit the scope of this hack to
> PPluginModule.

And is this alternative really better?

I, too, first thought of using an IPC call to get the current value of
the "nativeCursorSupportLevel" setting.  But then I realized this
would have to be a synchronous call, which would (at least in
principle) increase the chances of contention and hangs.

There's other code in the tree that uses environment variables to pass
values to subprocesses -- for example
http://hg.mozilla.org/mozilla-central/file/60cfdbaf76fb/toolkit/xre/nsUpdateDriver.cpp#l476.
Though I admit this isn't done often.

Passing variables in environment variables does have one disadvantage
I can think of -- a subprocess's subprocesses will inherit them.
Could we instead pass the value as a command-line parameter for the
plugin subprocess?

Ultimately, though, if people in the know think there's no significant
danger of the added synchronous IPC calls causing contention or hangs,
I'm happy to make the change -- it does have the advantage of being
simpler to code (and later on to remove).
Comment 91 Benjamin Smedberg [:bsmedberg] 2011-05-16 11:09:54 PDT
Any environment munging that happens should happen only after we've fork()ed but before we've exec()ed the new process. Which means that PerformAsyncLaunchInternal needs to at least be aware of the changes that need to be made: they should not be made on the main thread, because there are timing attacks which can cause security bugs related to unwanted environment variables such as this.

We make sync calls to plugins all the time, so I really doubt it will hurt as long as it's not super-frequent.
Comment 92 Steven Michaud [:smichaud] (Retired) 2011-05-16 12:05:59 PDT
> Any environment munging that happens should happen only after we've
> fork()ed but before we've exec()ed the new process. Which means that
> PerformAsyncLaunchInternal needs to at least be aware of the changes
> that need to be made: they should not be made on the main thread,
> because there are timing attacks which can cause security bugs
> related to unwanted environment variables such as this.

OK, I didn't realize this.  Thanks for letting me know.

> We make sync calls to plugins all the time, so I really doubt it
> will hurt as long as it's not super-frequent.

OK, I won't worry about it.
Comment 93 Steven Michaud [:smichaud] (Retired) 2011-05-19 16:26:30 PDT
Created attachment 533838 [details] [diff] [review]
v 1.5 patch (address cjones' review comments)

Here's a patch that addresses cjones' review comments.

It still has support for custom cursors, for the following reasons:

1) No valid objections have been made to the idea of including custom
   cursor support, or to anything in my implementation of them.

2) No problems have been reported in any tests of the last several
   versions of my patch.

3) Removing support for custom cursors wouldn't significantly reduce
   my patch's complexity.

4) Removing custom cursor support would severely limit my patch's
   functionality, to the point that it's no longer worth the
   complexity it adds to Mozilla code.  As far as I'm concerned, it's
   better to leave things as they are (and not land any part of my
   patch) than to land a badly crippled version of it.

A tryserver build should be available by tomorrow morning.
Comment 94 Josh Aas 2011-05-20 09:32:47 PDT
(In reply to comment #87)

> Yes, all code is a liability.  But this is true for *every* patch, and
> *every* change.  In order to make a decision that's not simply
> arbitrary, you need to give your reasons.  You *have* given one reason
> -- you're worried about sending custom cursor data over IPC, and
> particularly about the possible impact on performance of doing this.
> But, as I've already said, using DOM cursors will also involve sending
> custom cursor data over IPC, so this objection has no merit.

You have to re-evaluate the risk for this different context. The results of risk assessment for DOM cursors don't translate necessarily to this situation. DOM cursors provide more value over time because they'll be around for a long time. The custom cursor code in this patch will provide value only for a matter of months. Higher value over time makes risk (and effort) easier to swallow. Also, DOM cursors use standard NPRuntime to pass the image data, which has been used heavily for a while. Not a single line of code was added to facilitate the passing of cursor image data for DOM cursors. The code to pass image data here is new and untested. That makes it riskier.

I'm not convinced of your later argument that the custom cursor support is such a critical part of this patch that without it the rest of the patch isn't worth taking. The Chrome team apparently isn't convinced either. IIRC, they support show/hide and some other things your patch does, but not custom cursors.

That said, I don't feel strongly enough about this to argue any more and you've already done most/all of the work. I've already reviewed much of it too. I'll drop my objection. I appreciate your desire to get as much as possible working, even though I disagree about the risks. Hopefully I can finish the review today.
Comment 95 Steven Michaud [:smichaud] (Retired) 2011-05-20 10:08:48 PDT
> The Chrome team apparently isn't convinced either. IIRC, they
> support show/hide and some other things your patch does, but not
> custom cursors.

Chrome *does* support custom cursors -- just not cursor animation.

> That said, I don't feel strongly enough about this to argue any more
> and you've already done most/all of the work. I've already reviewed
> much of it too. I'll drop my objection. I appreciate your desire to
> get as much as possible working, even though I disagree about the
> risks. Hopefully I can finish the review today.

Thank you!  I'm quite sure the additional risks of supporting custom
cursors are small, and that they're outweighed by the annoyance to
users that would be caused by not including support for them.

Let's hope, though, that Silverlight and Flash quickly acquire support
for DOM cursors, so that we can drop my patch in the not-too-distant
future.
Comment 96 Steven Michaud [:smichaud] (Retired) 2011-05-20 12:58:20 PDT
Note:  I'm going to be out all day Monday (2011-05-23), and unable to
work on this bug.  But I realize that a deadline is approaching for
getting into Firefox 6, so I'm willing to work on this over the
weekend.
Comment 97 Josh Aas 2011-05-20 14:08:00 PDT
Comment on attachment 533838 [details] [diff] [review]
v 1.5 patch (address cjones' review comments)

Review of attachment 533838 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure about some of the dyld muckery here, hopefully bsmedberg will look more closely at that.

::: browser/app/profile/firefox.js
@@ +909,5 @@
> +#ifdef XP_MACOSX
> +// '0' means no support
> +// '1' means support for preset cursors (like the arrow or open-hand cursors)
> +//     and for hiding/showing cursors.
> +// '2' means full support (including for custom cursors)

In light of our decision to include full support, please just make this a boolean pref.

::: dom/plugins/ipc/PluginInterposeOSX.h
@@ +37,5 @@
> +#ifndef __OBJC__
> +class NSCursor;
> +#else
> +#import <Cocoa/Cocoa.h>
> +#endif

Why not just use the "class" version all the time and skip the ifdef?

::: dom/plugins/ipc/PluginInterposeOSX.mm
@@ +374,5 @@
> +}
> +
> +// Get a transparent cursor with the appropriate hot spot.  We need one if
> +// (for example) we have a custom cursor with no image data.
> +NSCursor* NSCursorInfo::GetTransparentCursor() const

Can we cache at least some of the work done in this method? Do we need to rebuild this 16x16 transparent image every time?

@@ +386,5 @@
> +
> +  uint8_t* data = (uint8_t*) moz_xmalloc(dataSize);
> +  for (int y = 0; y < height; ++y) {
> +    for (int x = 0; x < width; ++x) {
> +      int offset = y * rowBytes + x * bytesPerPixel;

Make order of operations more clear here using parens.

@@ +403,5 @@
> +                                               isPlanar:NO
> +                                         colorSpaceName:NSCalibratedWhiteColorSpace
> +                                            bytesPerRow:rowBytes
> +                                           bitsPerPixel:16]
> +     autorelease];

Are you sure this code can only execute within an event loop with a proper autorelease pool? If not then this and other autoreleased memory should be managed manually.

@@ +483,5 @@
> +  }
> +  return "TypeUnknown";
> +}
> +
> +nsPoint NSCursorInfo::GetHotSpot() const

Can you document somewhere what the coordinate system in use here is?

@@ +700,5 @@
> +void NotifyBrowserOfSetCursor(NSCursorInfo& aCursorInfo)
> +{
> +  AssertPluginThread();
> +  PluginModuleChild *pmc = PluginModuleChild::current();
> +  if (pmc)

I prefer always having brackets on if statements.

::: dom/plugins/ipc/interpose/plugin_child_interpose.mm
@@ +93,5 @@
> +    OnShowCursorPtr = (BOOL(*)())
> +      dlsym(RTLD_DEFAULT, "mac_plugin_interposing_child_OnShowCursor");
> +  }
> +  return ((OnSetCursorPtr != NULL) && (OnSetThemeCursorPtr != NULL) &&
> +    (OnHideCursorPtr != NULL) && (OnShowCursorPtr != NULL));

I'd rather we not have the "!= NULL" components here, unnecessary.
Comment 98 Steven Michaud [:smichaud] (Retired) 2011-05-20 14:33:50 PDT
> I'm not sure about some of the dyld muckery here, hopefully
> bsmedberg will look more closely at that.

I more-or-less copied this from Chrome, which seems to have used it
successfully for quite a while.  And of course by now I've tested it
quite thoroughly myself.

>> +// '0' means no support
>> +// '1' means support for preset cursors (like the arrow or open-hand
>> +//     cursors) and for hiding/showing cursors.
>> +// '2' means full support (including for custom cursors)
>
> In light of our decision to include full support, please just make
> this a boolean pref.

OK.

>> +#ifndef __OBJC__
>> +class NSCursor;
>> +#else
>> +#import <Cocoa/Cocoa.h>
>> +#endif
>
> Why not just use the "class" version all the time and skip the
> ifdef?

Because PluginInterposeOSX.h needs to be includable from C++ code.

>> +// Get a transparent cursor with the appropriate hot spot.  We need one if
>> +// (for example) we have a custom cursor with no image data.
>> +NSCursor* NSCursorInfo::GetTransparentCursor() const
>
> Can we cache at least some of the work done in this method? Do we
> need to rebuild this 16x16 transparent image every time?

Probably, but I need to check to be sure.

>> +  uint8_t* data = (uint8_t*) moz_xmalloc(dataSize);
>> +  for (int y = 0; y < height; ++y) {
>> +    for (int x = 0; x < width; ++x) {
>> +      int offset = y * rowBytes + x * bytesPerPixel;
>
> Make order of operations more clear here using parens.

I assume you want me to change the last line to:
int offset = (y * rowBytes) + (x * bytesPerPixel);

>> +                                               isPlanar:NO
>> +                                         colorSpaceName:NSCalibratedWhiteColorSpace
>> +                                            bytesPerRow:rowBytes
>> +                                           bitsPerPixel:16]
>> +     autorelease];
>
> Are you sure this code can only execute within an event loop with a
> proper autorelease pool? If not then this and other autoreleased
> memory should be managed manually.

I'm pretty sure, but I'll double check.  In any case, if there were no
autorelease pool, that'd be a bug, which we'd need to fix.

>> +  }
>> +  return "TypeUnknown";
>> +}
>> +
>> +nsPoint NSCursorInfo::GetHotSpot() const
>
> Can you document somewhere what the coordinate system in use here
> is?

OK.

>> +void NotifyBrowserOfSetCursor(NSCursorInfo& aCursorInfo)
>> +{
>> +  AssertPluginThread();
>> +  PluginModuleChild *pmc = PluginModuleChild::current();
>> +  if (pmc)
>
> I prefer always having brackets on if statements.

OK, but I won't add brackets to existing code.

>> +    OnShowCursorPtr = (BOOL(*)())
>> +      dlsym(RTLD_DEFAULT, "mac_plugin_interposing_child_OnShowCursor");
>> +  }
>> +  return ((OnSetCursorPtr != NULL) && (OnSetThemeCursorPtr != NULL) &&
>> +    (OnHideCursorPtr != NULL) && (OnShowCursorPtr != NULL));
>
> I'd rather we not have the "!= NULL" components here, unnecessary.

OK.
Comment 99 Steven Michaud [:smichaud] (Retired) 2011-05-20 16:15:12 PDT
>>> +// Get a transparent cursor with the appropriate hot spot.  We need one if
>>> +// (for example) we have a custom cursor with no image data.
>>> +NSCursor* NSCursorInfo::GetTransparentCursor() const
>>
>> Can we cache at least some of the work done in this method? Do we
>> need to rebuild this 16x16 transparent image every time?
>
> Probably, but I need to check to be sure.

I've looked into this further, and have found that it isn't necessary
-- GetTransparentCursor() is called at most once on an NSCursorInfo
object before that object is destroyed:

Every time an IPC call serializes an NSCursorInfo object from the
plugin process to the browser process, a new NSCursorInfo object is
deserialized in the browser process.  This object is used just once --
for example to set the native cursor.  When this happens,
GetNSCursor() is called once on the object, and GetTransparentCursor()
may be called from GetNSCursor().  But the deserialized NSCursorInfo
object gets destroyed after this single "use".
Comment 100 Steven Michaud [:smichaud] (Retired) 2011-05-20 16:19:49 PDT
And if GetTransparentCursor() became a static method, we'd leak the transparent cursor ... though this wouldn't be a large leak.
Comment 101 Steven Michaud [:smichaud] (Retired) 2011-05-20 16:56:16 PDT
Created attachment 534156 [details] [diff] [review]
v 1.6 patch (follow Josh's suggestions)
Comment 102 Benjamin Smedberg [:bsmedberg] 2011-05-23 07:04:39 PDT
Comment on attachment 534156 [details] [diff] [review]
v 1.6 patch (follow Josh's suggestions)

PluginInterposeOSX.h is only used within dom/plugins, please don't export the header, just include it directly.
Comment 103 Steven Michaud [:smichaud] (Retired) 2011-05-23 20:32:29 PDT
Created attachment 534662 [details] [diff] [review]
Final patch for someone else to land (from landing queue)
Comment 104 Steven Michaud [:smichaud] (Retired) 2011-05-23 21:01:03 PDT
> PluginInterposeOSX.h is only used within dom/plugins, please don't
> export the header, just include it directly.

As best I can tell, the fact that I need to include it from
PluginMessageUtils.h means that I have to export it.

But I just came back from a long day elsewhere, and didn't have time
to check this out thoroughly.  So I landed the patch as is, to make
the midnight deadline.
Comment 105 Steven Michaud [:smichaud] (Retired) 2011-05-23 21:01:43 PDT
landed -> will land
Comment 106 Steven Michaud [:smichaud] (Retired) 2011-05-24 00:01:44 PDT
Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/6bf3ffd66eed
Comment 107 Steven Michaud [:smichaud] (Retired) 2011-08-04 07:54:11 PDT
*** Bug 676406 has been marked as a duplicate of this bug. ***
Comment 108 Steven Michaud [:smichaud] (Retired) 2013-04-02 09:34:02 PDT
It's come up recently (at bug 855303) that recent Silverlight versions may no longer need the workarounds landed by the patch for this bug.  (Note that I said "may":  It will take some time to get an answer from Microsoft, or failing that to find the answer ourselves by reverse engineering.  We are currently pursuing this matter at bug 855303.)

I wonder if the same is true of Adobe's products.

Note You need to log in before you can comment on or make changes to this bug.