Xt plugins cannot be resized larger in FF17

RESOLVED FIXED in Firefox 19

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Leszek Matok, Assigned: karlt)

Tracking

({regression})

17 Branch
mozilla20
x86
Linux
regression
Points:
---

Firefox Tracking Flags

(firefox18 wontfix, firefox19+ verified, firefox-esr17- wontfix)

Details

(URL)

Attachments

(6 attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; U; Linux i686; rv:1.9.1.19) Gecko/20110429 Fedora/2.0.14-1.fc14 SeaMonkey/2.0.14
Build ID: 20110429142030

Steps to reproduce:

Tried entering the game.


Actual results:

If I set my config to start full screen, that works. But in window mode (which embeds QL in quakelive.com web page), it's not updating. By "not updating" I mean: the embedded window shows whatever was there, like windows from other desktops when I switch.


Expected results:

It should work like it works in all other browsers and all Firefox versions up to 16.0.2.
(Reporter)

Updated

5 years ago
Could you please help us and find the regression range ?
We have a tool that should help with that task:
- http://mozilla.github.com/mozregression/

per default it will use trunk builds and you have to use an earlier start date.
e.g. "mozregression --good=2012-04-01"
(Reporter)

Comment 2

5 years ago
Really useful script. Something with it is broken because none of the Nightlies accepted keyboard input, but mouse copy/paste saved the day :)

So after just a few bisections and only 16 minutes of testing it found the following:

Last good nightly: 2012-08-01
First bad nightly: 2012-08-02

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=582d4c67b3a7&tochange=588424024294


It proposed to start building stuff on my computer, but I didn't take the offer. I might try over the weekend if there's no other way.
Thank you for finding the regression range !
could this be a regression from bug 544088 ?
Component: Untriaged → Plug-ins
Keywords: regression
Product: Firefox → Core
(Reporter)

Comment 4

5 years ago
I don't know all the dirty details, but the Quake Live plugin needs Xt and doesn't use xembed (we know this because that's preventing QL from working in Chrome and Opera). You may be right.

Would it be simple to rebuild FF17 with those patches reverted? I'd be willing to try, but have no idea where to start.
Blocks: 544088
(In reply to Leszek Matok from comment #4)
> Would it be simple to rebuild FF17 with those patches reverted? I'd be
> willing to try, but have no idea where to start.

I haven't tried it myself, but mozcommitbuilder should make this pretty easy:
https://github.com/mozilla/mozcommitbuilder
(Assignee)

Comment 6

5 years ago
I'm surprised this worked in FF16.  Bug 544088 was meant to make it work in 17.
Did you have any dom.ipc.plugins.* preferences set to non-default values in about:config?
(Reporter)

Comment 7

5 years ago
I've tried mozcommitbuilder -g 101144 -b 101147, but it didn't finish compilation after 3,5 hours and I had to ^C it.

I don't have anything non-default in dom.ipc.plugins.*.

The plugin has always worked as OOPP since FF 3.6 times and nobody reported problems with FF up to version 16.0.2.
(Assignee)

Comment 8

5 years ago
(In reply to Leszek Matok from comment #7)
> The plugin has always worked as OOPP since FF 3.6 times

OOPP support was introduced on / backported to 3.6 and that was when Xt plugins became unsupported.  Do you know whether the plugin worked with earlier versions (which did have Xt support)?
(Reporter)

Comment 9

5 years ago
Yes, it still works in my old Seamonkey I've posted this bug with (equivalent to FF 3.5, IIRC). In 3.6 the default was also not to use OOPP (but we were testing it in anticipation of 4.0, so I can assure you it worked in 3.6's OOPP as well as in-process plugin).

It's not really surprising that it worked till FF 16 as QL doesn't need much of the Xt stuff, it's an OpenGL game that doesn't need any widgets, it only needs to embed the OpenGL "window" in the page. But I don't know how it does that / what exactly it expects from FF side.
(Assignee)

Comment 10

5 years ago
Can you check by setting the appropriate dom.ipc.plugins.enabled.*.so pref to false that the plugin still works in process in FF17, please?

Does the plugin typically open a context menu on right click?
If so, is that still working as expected?

Is there somewhere the plugin can be downloaded without registering, or even better somewhere to view the source?
(Reporter)

Comment 11

5 years ago
I've tested changing dom.ipc.plugins.enabled globally:
FF16/false - works
FF16/true  - works
FF17/false - works
FF17/true  - doesn't work in window

There are no context menus.

If you want to try for yourself, you can register with a bugmenot email. Last time I checked, QL didn't require email verification. (If it starts in window mode by default, you won't see anything because of this bug, but alt-enter should work.)

Neither the game or the plugin are open source.
(Assignee)

Comment 12

5 years ago
Unfortunately, the site seems to want me to agree to not "disassemble, reverse engineer, or decompile the Software" before I can view any page.  If I agree to that, I won't be able to work out how to fix this.  So it seems we need a Quake Live engineer to tell us what the plugin is expecting.

... unless there is another Xt plugin showing the same problems.
The VLC plugin seems to work as expected.

The output from having NSPR_LOG_MODULES=IPCPlugins:9,PluginNPN:9,PluginNPP:9,Plugin:9 in the environment may provide some clues, especially if that differs between firefox 16 and 17.

Comparing the outputs from xwininfo -tree also may (or may not) give some clues.
Component: Plug-ins → Quake Live
Product: Core → Plugins
Version: 17 Branch → unspecified
(Reporter)

Comment 13

5 years ago
Created attachment 685357 [details]
FF16 log
(Reporter)

Comment 14

5 years ago
Created attachment 685358 [details]
FF17 log
(Reporter)

Comment 15

5 years ago
Both logs are from identical "session": run FF, open the site, wait for stuff to load, enter a server in full screen mode, switch to window mode, quit, close browser.

I don't know what to look for in those logs. I use a 1024x640 window and I can see it mentioned in both logs in some functions, and in the log from FF17 there are two more instances of "1024" in the log, with new functions CreateWindow and DeleteWindow, absent from FF16 log. I wish I knew what that means!
(Reporter)

Comment 16

5 years ago
FF16:

xwininfo: Window id: 0x240008d "QUAKE LIVE - Now Playing! - Mozilla Firefox"

  Root window id: 0x15d (the root window) (has no name)
  Parent window id: 0xeaffb1 (has no name)
     2 children:
     0x240008e (has no name): ()  1x1+-1+-1  +-1+23
     0x2400104 (has no name): ()  1152x815+0+0  +0+24
        1 child:
        0x2400355 (has no name): ()  1024x640+11+135  +11+159
           1 child:
           0x2400356 (has no name): ()  1024x640+0+0  +11+159
              1 child:
              0x3c00001 (has no name): ()  1024x640+0+0  +11+159
                 1 child:
                 0x3e00002 "QuakeLive": ()  1024x640+0+0  +11+159


FF17:

xwininfo: Window id: 0x240139b "QUAKE LIVE - Now Playing! - Mozilla Firefox"

  Root window id: 0x15d (the root window) (has no name)
  Parent window id: 0xeb110a (has no name)
     2 children:
     0x240139c (has no name): ()  1x1+-1+-1  +-1+23
     0x24013f9 (has no name): ()  1152x815+0+0  +0+24
        1 child:
        0x240246e (has no name): ()  1024x640+11+135  +11+159
           1 child:
           0x240246f (has no name): ()  1x1+0+0  +11+159
              1 child:
              0x3c00001 (has no name): ()  1024x640+0+0  +11+159
                 1 child:
                 0x3e00002 "QuakeLive": ()  1024x640+0+0  +11+159

Check out the 1x1+0+0 window!
(Assignee)

Comment 17

5 years ago
Yes, the 1x1 window would explain the problem.

That window should be created and sized by the browser process.  It is passed to the plugin container, but not (directly) to the Quake Live plugin itself.

The next thing I'd try is using xtrace (http://xtrace.alioth.debian.org/) to see which process is actually resizing that window (or not).  I run "xtrace -o /path/to/outfile.log firefox".  The log files can get quite large so try to do as little as possible while generating the log and kill the app as soon the problem has demonstrated.
Compress (gzip or similar) the file before attaching.  (I'm hoping it will then be small enough to attach.)  Note that this is different from the "xtrace" program that comes with glibc.

Comment 18

5 years ago
A customer reported a similar problem on Solaris.
The plugin is Creo View 10.1.20.15, np6_pvapplite9.so.

The regression window is rev 101144 to rev 101147.

The plugin works fine until you enlarge the window.
It only paints on the original size although it draws in larger scale.

xwininfo -tree shows a 1x1 window.

The plugin works fine with Firefox 16/ipc on or Firefox 17/ipc off.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

5 years ago
Assignee: nobody → karlt
Component: Quake Live → Plug-ins
Product: Plugins → Core
(Assignee)

Comment 19

5 years ago
This is also affecting Adobe's nppdf.so.
Can be observed by the following steps.

1. Make web browser window smallish.
2. Load something with mime type application/pdf.
3. Resize web browser larger.

Expected result:
PDF viewer fills up whole content area of browser.

Actual result:
PDF document becomes more magnified but is truncated to size when pdf was loaded.
Summary: Quake Live window mode stopped working in FF17 → Xt plugins cannot be resized larger in FF17
Version: unspecified → 17 Branch
(Assignee)

Comment 20

5 years ago
Created attachment 696652 [details] [diff] [review]
remove unused gtk_xtbin_set_position

Some preparatory clean-up.

Currently the socket window is always at 0,0, but if, in the future, the
window needs to be moved, then, after the following patch, the window can be
moved simply through methods on the GtkWidget (mSocketWidget in
nsPluginNativeWindowGtk2).  Given the window is really owned by GDK, it would
be more appropriate to use GTK/GDK to set the position so that GDK's internal
data is consistent.  This is particularly important with GDK client-side
windows as the parent X Window may not be in the same position as the parent
GDK window.
Attachment #696652 - Flags: review?(stransky)
(Assignee)

Comment 21

5 years ago
Created attachment 696653 [details] [diff] [review]
don't redirect embedder socket window resize requests to Xt client

Passing the ResizeRedirectMask bit to XSelectInput was meaning that when the
GtkSocket tried to resize the socket window (embedderid here), that was turned
in to a ResizeRequest event sent to the Xt client, which is never handled.

When in-process, gtk_xtbin_resize was called and that resized the window from the Xt client.  This is the top_widget resize that I didn't understand in Bug
544088 comment 65, and so is not in the OOP implementation.

Having the child client redirect resize requests on the parent/embedder's
windows seems quite odd (an XEmbed embedder selects SubstructureRedirectMask
on its socket window to redirect child client requests on the child window),
so I'd prefer to fix this by removing the ResizeRedirectMask and so the window
will be resized when GTK tries to resize it.  The Xt client will get a
ConfigureNotify event which it will use to update its internal data.

That then makes the gtk_xtbin_resize unnecessary as the mSocketWidget
GtkWidget resize is sufficient.
Attachment #696653 - Flags: review?(stransky)
(Assignee)

Comment 22

5 years ago
Created attachment 696655 [details] [diff] [review]
listen only to necessary Xt events and be explicit about those events

While checking for similar problems in other XSelectInput calls I found the
code quite misleading, so this patch makes things explicit about the events
actually handled.

See the events actually handled in xt_client_event_handler and
xt_client_focus_listener, and compare with the table at
http://tronche.com/gui/x/xlib/events/processing-overview.html
The events marked "N.A." in the "Event Mask" column of that table are selected
by the boolean third parameter "nonmaskable" to XtAddEventHandler.

xt_client_handle_xembed_message handles XEMBED_FOCUS_IN by sending a FocusIn
event to the client window.  The code in xt_client_event_handler looked like
it handled this by then sending another XEMBED_REQUEST_FOCUS, but in fact this
code was never reached because the event mask 0x0FFFFF does not include
FocusChangeMask (1L<<21).

If FocusChangeMask is added to the event mask for xt_client_event_handler,
then FocusIn events from pointer motion trigger sending XEMBED_REQUEST_FOCUS,
which causes the currently focused GtkWidget to lose focus and
nsWindow::OnContainerFocusOutEvent() calls LoseNonXEmbedPluginFocus().  When
this happens, VLC and nppdf.so no longer receive keyboard events until another
mouse out and mouse in triggers another SetNonXEmbedPluginFocus().  These
plugins each use descendant windows owned by yet another X connection and it
seems that Xt's keyboard/focus handling isn't enough to propagate events to
those windows - pointer focus is needed.  The XEmbed focus with receiving
keyboard events on GDK's dedicated focus window does not mix with the
XSetInputFocus trickery in Set/LoseNonXEmbedPluginFocus(), which relies on and
works with pointer focus in a descendant of the focus window.
Attachment #696655 - Flags: review?(stransky)
(Assignee)

Comment 23

5 years ago
Created attachment 696656 [details] [diff] [review]
remove unnecessary XSelectInput with XtAddEventHandler

This XSelectInput call is not required because XtAddEventHandler does this for us.
Attachment #696656 - Flags: review?(stransky)
(Assignee)

Comment 24

5 years ago
Some test builds at http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/ktomlinson@mozilla.com-fc74c0eaedf3/try-linux/
(Reporter)

Comment 25

5 years ago
Your Nightly test build works for me (with Quake Live as OOPP).

There's absolutely no problem with input focus or grabbing the pointer, but in this case, the "window" can't be resized without full game reload, unlike nppdf etc.

So at least this plugin appears fixed nicely.

Assuming more people report success, can this fix still make its way to Firefox 18, or 18.0.1 if that ever happens, or 19?

Updated

5 years ago
Attachment #696652 - Flags: review?(stransky) → review+

Updated

5 years ago
Attachment #696653 - Flags: review?(stransky) → review+

Comment 26

5 years ago
(In reply to Karl Tomlinson (:karlt) from comment #24)
> Some test builds at
> http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/
> ktomlinson@mozilla.com-fc74c0eaedf3/try-linux/

Which patches are included in this build? I tested Acrobat Reader plugin with it and it fails to open more than one PDF document. Just open a pdf file, then open another one (in the same window) and so on. nppdf.so plugin crashes for second or third opened document and whole browser has to be restarted. I don't see such crashes in official nightly builds (20.0a1).

But I looks like a regression from Bug 623380 to me.

Comment 27

5 years ago
I don't see the nppdf.so crash with patches from only this bug.

Updated

5 years ago
Attachment #696655 - Flags: review?(stransky) → review+

Updated

5 years ago
Attachment #696656 - Flags: review?(stransky) → review+
(Assignee)

Comment 28

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4a06c212f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd9494f9be3
https://hg.mozilla.org/integration/mozilla-inbound/rev/cffc2daf1e91
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9fd7cdd333

(In reply to Leszek Matok from comment #25)
> can this fix still make its way to
> Firefox 18, or 18.0.1 if that ever happens, or 19?

This should be in 20, and we might be able to get it into 19, but I think it is too late for 18.  This is not the kind of change that goes into .1 releases.
Status: NEW → ASSIGNED
status-firefox18: --- → affected
status-firefox19: --- → affected
https://hg.mozilla.org/mozilla-central/rev/ca4a06c212f8
https://hg.mozilla.org/mozilla-central/rev/6bd9494f9be3
https://hg.mozilla.org/mozilla-central/rev/cffc2daf1e91
https://hg.mozilla.org/mozilla-central/rev/df9fd7cdd333
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Reporter)

Comment 30

5 years ago
Is there some kind of voting process or something to get the fix into FF 19 while it's still in Aurora? :)

Updated

5 years ago
status-firefox18: affected → wontfix
tracking-firefox19: --- → ?

Comment 31

5 years ago
Yeah - we like playing video games. Fix ASAP would be very handy :)
(In reply to Karl Tomlinson (:karlt) from comment #28)
> This should be in 20, and we might be able to get it into 19, but I think it
> is too late for 18.  This is not the kind of change that goes into .1
> releases.

If we feel the fix is low risk, we can definitely get this change into FF19 (aurora or beta). A lower risk subset of patches would be even better :). Agreed on 18.0 and 18.0.1, however.
tracking-firefox19: ? → +

Updated

5 years ago
status-firefox-esr17: --- → affected
tracking-firefox-esr17: --- → ?
(Assignee)

Comment 33

5 years ago
Comment on attachment 696653 [details] [diff] [review]
don't redirect embedder socket window resize requests to Xt client

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 544088 and bug 782612 
User impact if declined: Some plugins can't be resized.
This affects the acroread plugin (among others) when resizing a browser window.
Also some sites (Quake Live at least) initially show the plugin at 1x1 px, so the plugin becomes unusable when the page is not able to successfully resize the plugin.

Testing completed (on m-c, etc.): on m-c, manual testing by assignee, reviewer, and reporter.

Risk to taking this patch (and alternatives if risky): There is some risk to this patch because it is hard to guess what assumptions plugins might be making about what the browser does, but risk is limited to Unix Xt (not-Cocoa) plugins.
There are few popular Xt plugins.  Flash Player is not an Xt plugin.  The most popular plugins have been tested here.

String or UUID changes made by this patch: none
Attachment #696653 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 34

5 years ago
Comment on attachment 696652 [details] [diff] [review]
remove unused gtk_xtbin_set_position

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 

See previous comment.

Risk to taking this patch (and alternatives if risky): 

This patch is zero risk as it is removing unused code and variables.
I'm requesting approval for this patch as it means that the attachment 696653 [details] [diff] [review] will not need modification for aurora and so will benefit from the testing that has already been done.

(The last two attachments are not required.)

String or UUID changes made by this patch: none.
Attachment #696652 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #696652 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(Assignee)

Updated

5 years ago
Attachment #696653 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?

Updated

5 years ago
tracking-firefox-esr17: ? → +
Comment on attachment 696652 [details] [diff] [review]
remove unused gtk_xtbin_set_position

It would be great to get a fix nominated for uplift to FF17esr as well, as this may impact our enterprise users.

Also adding qawanted/verifyme to re-test Acrobat and VLC once this lands.
Attachment #696652 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
Attachment #696653 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

5 years ago
Keywords: qawanted, verifyme
Adding this to my sign-off checklist for 19b2.
Keywords: qawanted
(Assignee)

Comment 37

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/0958aea4dfd2
https://hg.mozilla.org/releases/mozilla-beta/rev/31a324c5347c

Note that VLC can't be resized even with builds prior to this regression in Firefox 17.  (That is an issue with a VLC window, which is a descendant of the Gecko window that had this bug.)
status-firefox19: affected → fixed
(Assignee)

Comment 38

5 years ago
Comment on attachment 696652 [details] [diff] [review]
remove unused gtk_xtbin_set_position

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
requested in comment 35.

Fix Landed on Version: 20 and 19

User impact if declined: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 
See comment 33 and 34.
Attachment #696652 - Flags: approval-mozilla-esr17?
(Assignee)

Comment 39

5 years ago
Comment on attachment 696653 [details] [diff] [review]
don't redirect embedder socket window resize requests to Xt client

[Approval Request Comment]
See comment 38.
Attachment #696653 - Flags: approval-mozilla-esr17?
(Assignee)

Updated

5 years ago
status-firefox-esr17: affected → checkin-pending
Could not play the game in Firefox 17.0, 18.0 and 18.0.1.

Verified the fix for Firefox 19.0 beta 2 playing the game in browser mode and fullscreen mode.
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0 Beta 2
Build ID: 20130116072953
status-firefox19: fixed → verified
QA Contact: virgil.dicu
Comment 35 missed that this is linux-only and since we are not currently hearing about user pain on this in the enterprise mailing list I think we can leave this for now. We can always revisit if there is a user-driven request on this bug for a fix in ESR.
status-firefox-esr17: checkin-pending → wontfix
Attachment #696652 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17-
Attachment #696653 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17-

Comment 42

5 years ago
(In reply to Lukas Blakk [:lsblakk] from comment #41)
> Comment 35 missed that this is linux-only and since we are not currently
> hearing about user pain on this in the enterprise mailing list I think we
> can leave this for now. We can always revisit if there is a user-driven
> request on this bug for a fix in ESR.

See comment #18, we do have customers complain it.
tracking-firefox-esr17: + → -
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.