Closed Bug 621117 Opened 14 years ago Closed 13 years ago

NSCursor class methods, SetThemeCursor() and SetCursor() not working for OOP plugins on mac

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(blocking2.0 .x+)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- .x+

People

(Reporter: duncantebbs, Assigned: smichaud)

References

Details

(Keywords: regression)

Attachments

(2 files, 8 obsolete files)

v 1.6 patch (follow Josh's suggestions)
62.29 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Final patch for someone else to land (from landing queue)
62.48 KB, patch
Details | Diff | Splinter Review 
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.
Component: Extension Compatibility → Plug-ins
Product: Firefox → Core
QA Contact: extension.compatibility → plugins
Version: unspecified → Trunk
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.
Assignee: nobody → joshmoz
Steven - can you look into this when you get a chance?
Assignee: joshmoz → smichaud
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yes.  But it'll be at least a week before I can do so.
(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.
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.
(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!
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
Keywords: regression
We don't need any more information here, we know what is going on. Thanks though.
blocking2.0: --- → .x+
Assignee: smichaud → joshmoz
Assignee: joshmoz → smichaud
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.
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!
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).
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/
> 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
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.
Hardware: x86_64 → x86
Attached patch v0.9 patch (work in progress) (obsolete) — — Splinter Review 
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.
Summary: [NSCursor hide] and [NSCursor unhide] not working for OOP plugins on mac → NSCursor class methods and SetThemeCursor() not working for OOP plugins on mac
(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.
Attached patch v0.91 patch (work in progress) (obsolete) — — Splinter Review 
Oops, the previous patch didn't build.
Attachment #525871 - Attachment is obsolete: true
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.
> 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).
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.
Attachment #525915 - Attachment is obsolete: true
Attachment #526834 - Flags: review?(joshmoz)
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.
Attachment #526834 - Flags: review?(jones.chris.g)
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.
Thanks Steven. We'll get started on testing this.
(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.
The DOM stuff was proposed to plugin-futures and implemented for FF5.
Oh duh, I even remember seeing that discussion go by.

Do we really want this patch then?
> 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?
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.
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 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.
Attachment #526834 - Flags: review?(joshmoz)
Attachment #526834 - Flags: review?(jones.chris.g)
Attached patch v1.1 patch (hook SetCursor()) (obsolete) — — Splinter Review 
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.
Attachment #526834 - Attachment is obsolete: true
Attachment #527871 - Flags: review?(joshmoz)
Attachment #527871 - Flags: review?(jones.chris.g)
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.
Summary: NSCursor class methods and SetThemeCursor() not working for OOP plugins on mac → NSCursor class methods, SetThemeCursor() and SetCursor() not working for OOP plugins on mac
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.
> 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.
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.)
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.
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
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?
(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.
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.
(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.
(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.
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.
(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.
Attachment #527871 - Flags: review?(joshmoz)
Attachment #527871 - Flags: review?(jones.chris.g)
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 #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.
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.
Steven, let me know if you have any issues with running the repro.
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?
> 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.
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.
> 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).
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.
(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.
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.
> 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.
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?
> 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.
(Following up comment #70)

And the context menu problem may be partly a Flash bug -- it doesn't happen with Silverlight.
Attached patch v 1.2 patch (more fixes) (obsolete) — — Splinter Review 
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.
> (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).
Attachment #527871 - Attachment is obsolete: true
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.
(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.
I tested with the release version as well and things are working as expected.
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.
(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.
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.
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.
Attachment #528896 - Attachment is obsolete: true
I tested your new nightly build with Flash Native mouse cursors and things all look good now.  Thanks Steve!
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.
Attachment #530351 - Attachment is obsolete: true
Attachment #531959 - Flags: review?(joshmoz)
Attachment #531959 - Flags: review?(jones.chris.g)
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 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.
Attachment #531959 - Flags: review?(jones.chris.g) → review-
(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.
(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.
(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.
(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?
(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).
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.
> 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.
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.
Attachment #531959 - Attachment is obsolete: true
Attachment #533838 - Flags: review?(joshmoz)
Attachment #533838 - Flags: review?(benjamin)
Attachment #531959 - Flags: review?(joshmoz)
(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.
> 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.
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 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.
Attachment #533838 - Flags: review?(joshmoz) → review+
> 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.
>>> +// 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".
And if GetTransparentCursor() became a static method, we'd leak the transparent cursor ... though this wouldn't be a large leak.

        Attachment #533838 -
        Attachment is obsolete: true

        Attachment #534156 -
        Flags: review?(benjamin)

        Attachment #533838 -
        Flags: review?(benjamin)

    
    

    
  
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.

        Attachment #534156 -
        Flags: review?(benjamin) → review+

    
    

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

    
    

    
  
landed -> will land
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
  
Depends on: 659817
No longer depends on: 659817

    
    

    
  
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.
See Also:  → 1575726

    
  
Product: Core → Core Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: