search

electron / libchromiumcontent

Shared library build of Chromium’s Content module
MIT License
485 stars 183 forks source link

fix: workaround for X11ErrorTrackers issue #673

Closed ckerr closed 6 years ago

ckerr commented 6 years ago
Description of Change

Fixes the crashing powerMonitor test that was disabled @ https://github.com/electron/electron/commit/76f26341bf3bc79f748fb2725251ee56fe7f37ab, so that now it can be re-enabled safely.

Only one X11ErrorTracker should exist at a time, but upstream has a bug where two can exist if running in headless mode --

  1. ui::(anonymous namespace)::SupportsEWMH() [inner tracker is created]
  2. ui::WmSupportsHint()
  3. ui::IsX11WindowFullScreen()
  4. ui::ScreensaverWindowFinder::IsScreensaverWindow()
  5. ui::ScreensaverWindowFinder::ShouldStopIterating()
  6. ui::EnumerateTopLevelWindows()
  7. ui::ScreensaverWindowFinder::ScreensaverWindowExists() [outer tracker created]
  8. ui::CheckIdleStateIsLocked()
  9. ui::CalculateIdleState()

Removal of either tracker could have side-effects in some code paths, so ultimately this is probably handled better by a refactor that is out-of-scope for Electron. Instead, this patch removes the check that prevents more than one tracker from existing at a time.

cc @deepak1556 @MarshallOfSound

Checklist
codebytere commented 6 years ago

@ckerr doesn't look like this patch applies cleanly, exits on script/update

ckerr commented 6 years ago

The problem with removing either X11ErrorTracker is that both ScreensaverWindowExists() and SupportsEWMH() base their return values on information from the trackers. If we remove one, one of those functions is going to start returning incorrect values in some conditions.

Enforcing the existence of only one X11ErrorTracker at a time seems to be an architectural safeguard: if an outer and inner function are both looking at low-level XErrors, then that code needs to be refactored. At the code level, though, it doesn't seem to do harm: when the inner X11ErrorTracker goes out of scope, it will restore the previous error handler, similar to how nested errno checkers operate. That's why this is the safer workaround.

I completely agree that either approach is a workaround and that the real fix is to not have this code path to begin with. Before reporting this upstream, I want to confirm that it's valid to call ui::CalculateIdleState() when headless. If it's not, then the bug is ours and we could change the Atom code that calls ui::CalculateIdleState().