474491 - NPP_SetWindow()'s window->clipRect.top off by 22 pixels in CoreGraphics mode (OS X)

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Steven Michaud [:smichaud] (Retired)]

Whenever the browser makes a call to NPP_SetWindow() in an NPAPI
plugin that uses the CoreGraphics drawing model, window->x is off by
22 pixels (the height of the browser window's titlebar).

You can confirm this by testing with the DebugEventsPlugin at bug
441880.  After copying the plugin to /Library/Internet Plug-Ins/, load
the plugin distro's test.html, and observe the calls to
NPP_SetWindow() that it logs to the system console.

Here's an example (made using the CoreGraphics drawing model, which is
the DebugEventsPlugin's default on browsers that support it):

DebugEventsPlugin: NPP_SetWindow(): instance 0x1CA2EC48,
    window 0x1E9DF470, x 8 y 114 width 400 height 400,
    clipRect (top 92 left 8 bottom 492 right 408)

Note that, while clipRect->top is 92, window->y is 114 (exactly 22
pixels larger).

Now load the DebugEventsPlugin again, using an altered EMBED tag like
the following (which makes the plugin use the old QuickDraw drawing
model):

<embed width="400" height="400" type="test/x-netscape-debug-events-plugin"
    supportCoreGraphics=no></embed>

Now you'll see logs of calls to NPP_SetWindow() like this:

DebugEventsPlugin: NPP_SetWindow(): instance 0x1F840228,
    window 0x1F04A3B0, x 8 y 92 width 400 height 400,
    clipRect (top 92 left 8 bottom 492 right 408)

Note that clipRect->top and window->y are both 92.

Both values should be the same.  And the results in QuickDraw drawing
mode show that '92' is (presumably) the correct value.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

This bug started happening with the 2007-06-03-04-trunk nightly --
which implicates the patch for bug 382765.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Turning various toolbars off and on, the following is apparent:

In CoreGraphics mode, window->y measures from the very top of the
browser window (the top of its titlebar), but window->clipRect.top
measures from the bottom of the titlebar.

In QuickDraw mode, both window->y and window->clipRect.top measure
from the bottom of the titlebar.

I'm no longer so sure that '92' (in comment #0) is the correct value
for window->y and window->clipRect.top.  The correct value may
actually be '114'.  I'll be looking further into this.

But in any case both window->y and window->clipRect.top should have
the same value.

Comment 3

[Kenneth Russell]

Note that when the page is scrolled so that the plugin's area is clipped by the top of the window, it is not and should not be the case that window->y and window->clipRect.top are the same.

Please compare to Safari on the Mac to see what we have found to be the correct behavior.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> Note that when the page is scrolled so that the plugin's area is
> clipped by the top of the window, it is not and should not be the
> case that window->y and window->clipRect.top are the same.

Correct.  I was referring to cases where there is no clipping.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

In Safari, both window->y and window->clipRect.top are measured from
the top of the titlebar in CoreGraphics mode but from the bottom of
the titlebar in QuickDraw mode.

I suppose that, whatever the abstract merits of the case, we're now
stuck with this.  What do you think, Josh?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

By the way, when I say "titlebar", I'm always referring to a window's top 22 pixels.  I'm *not* including any additional toolbars (or whatever) below it.

Comment 7

[Kenneth Russell]

It's unfortunate that there are compatibility constraints in this area but it would be best to have consistent behavior between browsers. From the point of view of the new Java Plug-In, we currently depend on the location and clip coming in from NPP_SetWindow as well as the result of a call to GetWindowBounds() with a region code of kWindowStructureRgn in order to properly place the applet content on the web page.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I don't have a problem (other than a mildly offended sense of
neatness) with measuring window->y and window->clipRect.top from the
top of the window in CoreGraphics mode, and from the bottom of the
(22-pixel high) titlebar in QuickDraw mode.

But we really should document this somewhere, prominently, to stop
people getting confused by it.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a patch that (in my tests) fixes this problem --
NPP_SetWindow() now measures window->y and window->clipRect.top from
the top of the window in CoreGraphics mode, and from the bottom of the
(22-pixel high) titlebar in QuickDraw mode.

And here's a tryserver build made with my patch:

https://build.mozilla.org/tryserver-builds/2009-01-21_10:[e-mail]-1232562856/[e-mail]

Please try it, Kenneth, and let us know your results.

> This bug started happening with the 2007-06-03-04-trunk nightly --
> which implicates the patch for bug 382765.

The patch for bug 382765 didn't cause this bug -- it just uncovered
it.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 358004 [details] [diff] [review]
Fix

I had high hopes this would resolve bug 436945.

No such luck ... but keep testing.

Comment 11

[Kenneth Russell]

I've tested this build with the new Java Plug-In and it looks good. This fix will eventually let us get rid of a Firefox-specific hack in our code.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 358004 [details] [diff] [review]
Fix

Josh says this needs to get into 1.9.1.

I'm currently unable to land this on the trunk, because it's
restricted to 1.9.1 blockers only (for the beta3 code freeze).

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/53f4c4890ffc

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Landed on the 1.9.1 branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9074662447ae

Comment 15

[(not currently active) Ted Mielczarek]

Any way we can test this with the new test plugin?

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Not with an automated test.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Backed out on 1.9.1 branch because this patch triggered bug 477077:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0f71219d1610

I'm waiting for mozilla-central to open, then I'll also back it out there.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Bug 477077 shows that the Flash plugin (both version 10 from Adobe and
version 9 (r151) from Apple) has Mozilla.org-specific code to work
around this bug.  Fixing this bug apparently breaks that code.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Backed out on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/a3db57c18fb4

Comment 20

[Steven Michaud [:smichaud] (Retired)]

At Josh's request, I landed patches adding an explanatory comment to nsChildView::GetPluginClipRect():

http://hg.mozilla.org/mozilla-central/rev/fe61a13e0960
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1aefd6ff4180

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Steven, is this fixed now? If not we should remove the fixed1.9.1 keyword, right?

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Since this has been reopened (and the patch backed out everywhere it was landed), it's no longer "fixed1.9.1".

I simply forgot to do this earlier.

Comment 23

[Damon Sicore (:damons)]

Steven, Ideas on what is wrong?  ETA for a new patch?

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> Ideas on what is wrong?

I'm pretty sure comment #18 is the correct explanation.

> ETA for a new patch?

If so, we can't fix this bug until we come to some kind of arrangement
with Adobe and Sun/Apple (plus other vendors who have plugins in the
works that use the CoreGraphics drawing mode).  For example we might
do as described in but 477077 comment #16.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

bug 477077 comment #16

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Michelle, is comment #18 correct?

Comment 27

[Josh Aas]

We can't block on this given our situation, I'm working on a long-term solution.

Comment 28

[Josh Aas]

Note that while there is a specification mismatch between Gecko and WebKit, there is no actual broken behavior in the wild at this time, for any plugin. I'm working on a way to avoid breaking anyone when we resolve the mismatch.

Comment 29

[Kenneth Russell]

FYI, in the Java Plug-In we dynamically detect whether the +22 offset is needed based on the incoming clip regions, so it should be possible for other plugins to do the same without needing to rely on version detection.

Comment 30

[Kevin Shepherd]

I have done some work on what I believe to be the same bug as reported here.  Namely a vertical offset in some NPP_SetWindow calls (Mac OS X only).  I submit my results here in the hope that it will help with a fix.
Working:
Safari v4.0.2
SeaMonkey v1.1.16
Opera v9.64
OmniWeb v5.9.2
iCab v4.5.0
Firefox v2.0.0.20

NPP_SetWindow initially providing wrong window port position:
Camino v1.6.7
Firefox v3.0.11
Firefox v3.5.0
SeaMonkey v2.0b1

NPP_SetWindow initially provides the wrong port co-ordinates, but gives the
correct ones later, after a window re-size.  This bug is shared by Camino
v1.6.x, Firefox v3.x and SeaMonkey v2.x
This bug is not just Gecko version dependent but is at least partially browser specific ( Camino with Gecko v1.8.1.21 has it,
but SeaMonkey with the same Gecko version does not ).

and additionally, I quote from Smokey Ardisson:
This indicates that this particular bug has been latent in Cocoa widgets (which
Camino has always used, but which Firefox and SeaMonkey only started using with
Fx3 and Sm2).

These comments are split off from the related bug 505135

Comment 31

[Kevin Shepherd]

In the hopes that this helps others who are trying to get around this bug, I have found the following test to be fairly reliable.  This is an excerpt from my code called within the updateEvt.  The bufferRect defines my plugin's actual area (in this case for AGL, though they seem to be QD correct also)
  Rect winRect;
  GetWindowPortBounds(mWindow, & winRect);
  Rect structRect;
  GetWindowBounds(mWindow, kWindowStructureRgn, & structRect);
  int win_height = winRect.bottom - winRect.top;
  int win_width = winRect.right - winRect.left;
  int struct_height = structRect.bottom - structRect.top;
  int bug_474491_offset = 0;
// Fix Firefox v3.x, SeaMonkey v2.x, Camino bug bugzilla 474491:
  if (struct_height == win_height)
    {
    bug_474491_offset = -22;
    }
  GLint bufferRect[4];
  /* mX, mY, mWidth, mHeight come from NPP_SetWindow(NPP instance, NPWindow* aWindow):
     NP_Port * npport = (NP_Port * ) aWindow->window;
     mX = -npport->portx;
     mY = -npport->porty;
     mWidth = aWindow->width;
     mHeight = aWindow->height;
  */
  bufferRect[0] = mX; // 0 = left edge
  bufferRect[1] = (win_height - ( mY + mHeight)) + bug_474491_offset; // 0 = bottom edge
  bufferRect[2] = mWidth; // width of buffer rect
  bufferRect[3] = mHeight; // height of buffer rect
  
  aglSetInteger (aglContext, AGL_BUFFER_RECT, bufferRect);

Thank you to Kenneth for the hint.

Comment 32

[Michelle Sintov]

I am working on the fix for the Flash Player. Could you tell me how I can reproduce this bug with already built binary (e.g., does it happen only in Mac Firefox nightlies or in Camino)?

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Test with the STR from comment 0 :-)

Comment 34

[Michelle Sintov]

What does STR mean?

Comment 35

[Aakash Desai [:aakashd]]

steps to reproduce, michelle

Comment 36

[Michelle Sintov]

Josh Aas and I looked at this bug together. Josh came up with a solution for how a browser plug-in should work around this bug (thanks to Ken for doing so much work on this already). After plug-ins have implemented this workaround, Mozilla is free to change the clipRect to match Safari behavior.

Using DebugEventsPlugin, after plug-in startup I see 4 NPP_SetWindow calls with window settings like the following:

NPP_SetWindow #1: 9/8/09 10:44:45 AM firefox-bin[451] DebugEventsPlugin: NPP_SetWindow(): x 8 y 8 width 400 height 400, clipRect (top 0 left 0 bottom 0 right 0) 

NPP_SetWindow #2: 9/8/09 10:44:45 AM firefox-bin[451] DebugEventsPlugin: NPP_SetWindow(): x 8 y 117 width 400 height 400, clipRect (top 95 left 8 bottom 95 right 8) 

NPP_SetWindow #3: 9/8/09 10:44:45 AM firefox-bin[451] DebugEventsPlugin: NPP_SetWindow(): x 8 y 8 width 400 height 400, clipRect (top 95 left 8 bottom 95 right 8) 

NPP_SetWindow #4: 9/8/09 10:44:45 AM firefox-bin[451] DebugEventsPlugin: NPP_SetWindow(): x 8 y 117 width 400 height 400, clipRect (top 95 left 8 bottom 495 right 408) 


Note that you can generally ignore any SetWindow call that has a clipRect size of 0,0. The fact that the clipRect has no area tells you something (don't draw), but aside from that it doesn't matter what the numbers are. The clip rects are of size 0,0 for the first 3 SetWindow above:
(0,0,0,0) = size 0,0
(95,8,95,8) = size 0,0
(95,8,95,8) = size 0,0
These aren't bogus, the 3 calls are essentially duplicate at worst.

Given the above SetWindow calls, here is the proper plug-in behavior:

- SetWindow #1 comes in. 0-8 is -8, so from now on assume the current browser has the 22-pixel offset bug; set a flag to remember that this browser has this behavior. This is the only time you set this flag to true and you should never change it back to false for this plug-in session. Other than setting the flag, you can ignore this NPP_SetWindow call because the clipRect is size 0,0.

- SetWindow #2 comes in. You may ignore it because the cliprect is size 0,0.

- SetWindow #3 comes in. You may ignore it because the cliprect is size 0,0.

- SetWindow #4 comes in. window=(x 8 y 117 width 400 height 400) and cliprect=(top 95 left 8 bottom 495 right 408). Clip rect is not of size 0,0 so don't ignore this. Since you set the flag to true in SetWindow #1, you know you need to shift the whole clip rect down by 22 pixels, which means adding 22 to cliprect.top and cliprect.bottom so that the resolved clipRect is (top 117 left 8 bottom 517 right 408). This clipRect makes a lot of sense - it is the full area of the plugin but in another representation.

The Flash Player is following this process and it seems to be working. I will reply back to this bug when the Flash Player is available publicly with this workaround.

Comment 37

[Benjamin Smedberg]

smichaud, are we ever going to fix this bug, or is the risk to great? Please resolve WONTFIX if appropriate.

Comment 38

[Steven Michaud [:smichaud] (Retired)]

> are we ever going to fix this bug?

Probably not.  I've never heard anything more about it since comment #36, and fixing it would require a lot of work coordinating with plugin vendors.

So I'm happy to mark it WONTFIX.  We can reevaluate if the issue ever comes up again.