1345899 - CrossProcessSemaphore crash at startup with tiled layers disabled

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P5)

Description

[Landry Breuil (:gaston)]

Followup to/regression from bug #1342843, esp comment 18 - right now with 54 aurora will crash at startup with an empty profile. 

starts fine with browser.tabs.remote.autostart.2=false + layers.enable-tiles=true, and crashes on sem_init() with browser.tabs.remote.autostart.2=false + layers.enable-tiles=false.

For now, sem_init() isnt an option on OpenBSD as we dont support shared semaphores for $reasons.

I dunno whats tiled layers nor if it's planned to enable it by default, and dunno if sem_init() w/ shared semaphores can be 'fixed' in OpenBSD soon.

Comment 1

[Landry Breuil (:gaston)]

Fix title, also crashes without e10s.

Comment 2

[Jan Beich]

Do you prefer to crash in ipc/glue/CrossProcessSemaphore_unimplemented.cpp instead? If not then the regression is due to bug 1325227, not bug 1342843.

Comment 3

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #2)
> Do you prefer to crash in ipc/glue/CrossProcessSemaphore_unimplemented.cpp
> instead? If not then the regression is due to bug 1325227, not bug 1342843.

I don't really have an opinion, nor an idea about all that stuff, nor time to dig into the details, but feel free to fix the depends :)

Comment 4

[Nathan Froyd [:froydnj]]

If this only affects *BSD, I'm going to mark this fix-optional so it disappears from tracking dashboards.  Feel free to disagree.

Comment 5

[Landry Breuil (:gaston)]

So, to summarise what needs to be done, if i understood the situation correctly:
- starting with 54, some codepaths are used (because of #1325227) and cause this crash ?
- tiled layers are disabled by default, except on macos (https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#4677) ?
- CrossProcessSemaphore cant work on openbsd as long as shared semaphores are not available there ?

Should layers.enable-tiles default to true on OpenBSD starting with 54 ? I can do that in a local prefs.js patch but not sure that'd propagate to existing profiles - or a local patch to our port.

What's the end-user consequence of using tiles, and why is it enabled only on macos ?

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Landry Breuil (:gaston) from comment #5)
> So, to summarise what needs to be done, if i understood the situation
> correctly:
> - starting with 54, some codepaths are used (because of #1325227) and cause
> this crash ?
> - tiled layers are disabled by default, except on macos
> (https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js#4677) ?
> - CrossProcessSemaphore cant work on openbsd as long as shared semaphores
> are not available there ?

Yes, that appears to be the case.

> 
> Should layers.enable-tiles default to true on OpenBSD starting with 54 ? I
> can do that in a local prefs.js patch but not sure that'd propagate to
> existing profiles - or a local patch to our port.
> 
> What's the end-user consequence of using tiles, and why is it enabled only
> on macos ?

We only enabled it on OSX, as that's the only platform where we finished testing sufficiently to feel comfortable turning it. We tried on Windows and had performance issues with Direct2D (changing render targets for each tile is slow on the GPU).

It should work fine on OpenBSD, but it's worth doing some testing.

Comment 8

[Landry Breuil (:gaston)]

So how is one supposed to change the *default* value for all profiles ? i tried the following:

--- modules/libpref/init/all.js.orig    Wed Apr 26 18:47:43 2017
+++ modules/libpref/init/all.js Wed Apr 26 18:49:13 2017
@@ -4664,7 +4664,7 @@ pref("layers.tiles.adjust", true);
 // 0  -> full-tilt mode: Recomposite even if not transaction occured.
 pref("layers.offmainthreadcomposition.frame-rate", -1);
 
-#ifdef XP_MACOSX
+#ifdef __OpenBSD__
 pref("layers.enable-tiles", true);
 pref("layers.tile-width", 512);
 pref("layers.tile-height", 512);

but that didnt work, it was still crashing, i had to manually edit prefs.js in my profile to enable layers.enable-tiles but i want to do that globally for all users of the package....

I wonder if it's a preprocessor issue (and i have to figure out which define to pass it for OpenBSD, so that it its upstreamable for 54) or if the default has to be changed somewhere else......

Comment 9

[Jan Beich]

54.0b3 would have bug 1341496. Does it help?

Comment 10

[Landry Breuil (:gaston)]

Interesting, i'll make sure to double-check with/without layers.enable-tiles when 54.0b3 is out. In the meantime im trying to understand which preprocessor flags are used when processing greprefs.js, and how to change them for openbsd...

Comment 11

[Jan Beich]

moz.build uses its own preprocessor[1] for non-C/C++ code and has fewer (if any) predefined macros. moz.configure uses set_define('XP_FOO', is_foo) for most Tier1 platforms[2] to get -DXP_FOO on command-line. For one ad-hoc case something like the following may work:

modules/libpref/moz.build:
if CONFIG['OS_TARGET'] == 'OpenBSD':
    DEFINES['XP_OPENBSD'] = True

modules/libpref/init/all.js:
#if defined(XP_MACOSX) || defined(XP_OPENBSD)
pref("layers.enable-tiles", true);
...

[1] http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca8/python/mozbuild/mozbuild/preprocessor.py
[2] http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca8/build/moz.configure/init.configure

Comment 12

[Landry Breuil (:gaston)]

I've played around a bit, and without changing moz.build this works:

-#ifdef XP_MACOSX
+#if defined(XP_MACOSX) || OS_ARCH == OpenBSD

i think this would work too:

#if defined(XP_MACOSX) || defined(OS_OPENBSD)

as the preprocessor has (among others) this: -DOS_OPENBSD=1 -DOS_ARCH=OpenBSD

Comment 13

[Landry Breuil (:gaston)]

Attachment: bug-1345899

Might not be the best solution, but ive been using this in all my beta builds without regressions. And until we have working shared semaphores, i dont see another option.. hoping this can make beta/54.

Comment 14

[Jan Beich]

Comment on attachment 8870124 [details]
bug-1345899

Nothing defines XP_OPENBSD. Did you mean OS_OPENBSD or forgot to adjust moz.build (see comment 11)?

Comment 15

[Landry Breuil (:gaston)]

Hmmm probably; f**k. Will fix the patch, and actually test it at runtime...

Comment 16

[Landry Breuil (:gaston)]

And fwiw, also affects TB 54.0b1 (but there's no tracking flag for TB beta ?). Without enabling tiles in my profile, console is filled with Crash Annotation GraphicsCriticalError and the window is black.

Comment 17

[Landry Breuil (:gaston)]

Attachment: bug-1345899

This one actually tested at runtime with an empty profile -> layers.enable-tiles defaults to true as expected.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/895d9a8826a3
enable tiles by default on OpenBSD r=mattwoodrow

Comment 19

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/895d9a8826a3

Comment 20

[Landry Breuil (:gaston)]

Comment on attachment 8870379 [details] [diff] [review]
bug-1345899

Approval Request Comment
[Feature/Bug causing the regression]: #1342843
[User impact if declined]: crash at startup on OpenBSD
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No
[Why is the change risky/not risky?]: Tier3 & tested locally with all 54 betas.
[String changes made/needed]:

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8870379 [details] [diff] [review]
bug-1345899

Seems like a very low risk fix that should help OpenBSD, Beta54+

Comment 22

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4cc48c061ec5

Comment 23

[Andrei Vaida [:avaida]]

(In reply to Landry Breuil (:gaston) from comment #20)
> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Landry Breuil's assessment on manual testing needs.