Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Never treat Firefox tooltips as normal windows #102

Closed
wants to merge 1 commit into from

Conversation

robotanarchy
Copy link

More often than not, tooltips were recognized as normal windows instead of unmanaged windows (Screenshot). It's fixed by checking x11.api.xcb_get_property_value_length(reply) as in the commit.


While debugging this, I found out, that it's probably a race condition (property values are not always ready, when asking for them?) - when I've added a simple printf (which will need some time to execute) just before the xcb_get_property_value() call, it always worked on my machine:

// ...
view->type &= ~WLC_BIT_UNMANAGED | ~WLC_BIT_SPLASH | ~WLC_BIT_MODAL;
printf("DEBUG: XCB_ATOM_ATOM marker\n");
xcb_atom_t *atoms = x11.api.xcb_get_property_value(reply);
// ...

@Cloudef
Copy link
Owner

Cloudef commented Jan 4, 2016

I don't think this is proper fix for the issue. Instead the view creation probably should've delayed until we get all the atoms we need. (I assume the issue is with atoms coming later than the surface is actually created).

@robotanarchy
Copy link
Author

  • Do you mean: Just wait for x11.atoms[NET_WM_WINDOW_TYPE], XCB_ATOM_ATOM (A), or for all the atoms inside the struct props inside read_properties() (B)?
  • Is this the right position to patch it?
// ...
case XCB_CREATE_NOTIFY:
{
   xcb_create_notify_event_t *ev = (xcb_create_notify_event_t*)event;
   wlc_dlog(WLC_DBG_XWM, "XCB_CREATE_NOTIFY (%u : %d)", ev->window, ev->override_redirect);
   // if( all atoms there of id ev->window ) {
   add_window(xwm, ev->window, ev->override_redirect);
   // }
}
// ...

@Cloudef
Copy link
Owner

Cloudef commented Jan 4, 2016

XCB_PROPERTY_NOTIFY notifies when new atom is available. I'm not really sure what is the best way to handle this to be honest. It might make sense to look from other X11 window managers or weston. NET_WM_WINDOW_TYPE for example might not come at all if the client does not set it.

It might work if we delay window creation until first property or configuration notify was received. But more research is probably needed.

@robotanarchy
Copy link
Author

Then how about including my commit as a workaround, until we can fix it properly?
Without this, Firefox is basically unusable for me, and it also "fixes" menus in other GTK2 applications (geany, roxterm, ...).

@Cloudef
Copy link
Owner

Cloudef commented Jan 4, 2016

I'm having doubts whether this fix just works for you though. As race conditions behave differently between machines / setups.

If you can check for what atom types the propery value length is zero though, it might give idea what switch case it avoids running, and see if there is actual bug here.

@robotanarchy
Copy link
Author

Looks like you're right. I can't even reproduce the issue anymore (without my 'fix', just like it was before).
When I can reproduce it again, I'll try to find out more.

@robotanarchy
Copy link
Author

After some more digging into this, I found out that the bug only appears when automatically compiling as Void Linux package and installing.

I've run my debug code by simply running cmake ., make and then copying the libwlc.so.0.0.1 file directly to the file system.

Now I've used the Void Linux packaging tools to create a package with modified code for debugging. I found out, that x11.api.xcb_get_property_value_length(reply) is always > 0, even when the bug occurs.

I know that the Void Linux packaging tools strip the library, and use hardening compile flags.

The AUR package has the '!strip'-flag set, maybe that is causing it? Is there a reason for that?

EDIT: I've created the Void Linux package recipes myself here, but I'm not doing anything fancy except for replacing all so-file references in the source code with the versioned sonames (as mentioned here).

If you want to, I can create an issue for this (doesn't have much to do with the current PR anymore).

@Earnestly
Copy link
Contributor

The !strip (combined with debug) option allows the symbols to remain in the package (as opposed to a separate -debug package) which is there to provide useful backtraces.

@robotanarchy
Copy link
Author

Thanks @Earnestly .

So after more testing, the bug also appears when compiling without Void Linux packaging scripts. When I'm compiling the exact same code the exact same way as before, where the bug did not appear. So it doesn't seem to be related to my Void Linux recipe either...

I've opened the ticket so hopefully we'll get more information about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants