Right now, we blockify display: -moz-box elements in certain situations (as a child of a grid container, with position: absolute/fixed, etc.). That behaviour is not always wanted, especially for the grid case.

:dholbert suggested landing this change behind a preference so we can fix all the visual regressions before preffing it on.

FYI I had a recent-ish try run in https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b231eb3adb7e3c49a497c028af987bc7ada8a7... Most of the orange is unrelated.

If we do this, we should also blockify -moz-inline-box to -moz-box, for sanity.

The patch still needs tests, and a pref to control it (so we can make the change experimentally/gradually).

Also, per IRC, emilio's got a slightly-different approach in
https://hg.mozilla.org/try/rev/8048fc847b3927facdc5d4df673b63ea859ba000#l5.31
and
https://hg.mozilla.org/try/rev/3686e153c092316ab975a7b383752fb86c51b814#l2.2
(relevant bits from comment 1's try run)

FWIW, a try run without gunk that caused a bunch of that orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b2db38c5ca4e88e26589930087a583b4188643a

Per bug 579776 comment 9, we may want to keep this behavior for out-of-flows (or just forbid out-of-flow XUL stuff with an explicit crash).

FWIW, the trade-offs there are now quite different than when bug 579776 was closed, now that these values are not exposed to content at all... That probably makes changing this way less risky, from a security point-of-view.

(In reply to Daniel Holbert [:dholbert] from comment #3)

Also, per IRC, emilio's got a slightly-different approach in
https://hg.mozilla.org/try/rev/8048fc847b3927facdc5d4df673b63ea859ba000#l5.31
and
https://hg.mozilla.org/try/rev/3686e153c092316ab975a7b383752fb86c51b814#l2.2
(relevant bits from comment 1's try run)

note: ntim tried these patches and ended up with a Firefox build that looked quite broken -- see attachment 9092482 [details] / bug 1580302 comment 4.

So it's possible that the fallout from making -moz-box be "block-outside" has too many side-effects. For now, I'll proceed with the other strategy.

Comment on attachment 9091613 [details]
Bug 1580012 - Set display: block; on elements relying on blockification behaviour.

Revision D45280 was moved to bug 1581973. Setting attachment 9091613 [details] to obsolete.

https://hg.mozilla.org/mozilla-central/rev/416de5a80d29

Hi Richard, you may want to see what turning on layout.css.xul-box-display-values.survive-blockification.enabled breaks in Thunderbird and apply display: block manually there (like what bug 1581973 and bug 1582316 do).

The cases that should change are described here: https://groups.google.com/forum/#!topic/mozilla.dev.platform/IpRMO1A6uwg

If that's helpful for spotting breakage, here's a patch from Daniel that triggers a crash in the position: absolute/fixed blockification case.

Also, note that it should somewhat simplify the grid conversions to CSS grid that Khushil has been doing once we turn this on by default, since you won't need to apply display: flex on child elements anymore.

note: in followup-bug 1582530, we'll try turning this pref on by default.

--> Adjusting the relationship so that the followup bug is dependent on this one, since I think that makes more sense & better matches reality, and clarifying the bug title here.

No crash patch needed. TB crashes already with the pref enabled. :-(
I have no debug skills, so n-i Magnus. Maybe he can check this.

Richard, you might want to search for position: absolute/fixed, go through all usages and add display: block for XUL elements (so not ::before/::after or HTML elements) and see the crash still reproduces. See bug 1581973 and bug 1582316, the changes should just be landable even with the pref off.

I can confirm setting layout.css.xul-box-display-values.survive-blockification.enabled to true will crash Thunderbird (almost directly, and then on startup). I don't really understand how https://hg.mozilla.org/mozilla-central/rev/416de5a80d29 causes it, but it does.

Any ideas?

WindowGlobalChild is null?

#0 0x00007fffee1d94eb in nsCOMPtr_base::assign_with_AddRef(nsISupports*) (this=0x98, aRawPtr=0x7fffdd902ca0) at /home/magnus/Code/tb/mozilla/xpcom/base/nsCOMPtr.cpp:40
#1 0x00007ffff0984709 in nsCOMPtr<nsIURI>::operator=(nsIURI*) (this=0x98, aRhs=0x7fffdd902ca0) at /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:672
#2 0x00007ffff0984709 in mozilla::dom::WindowGlobalChild::SetDocumentURI(nsIURI*) (this=0x0, aDocumentURI=0x7fffdd902ca0)
at /home/magnus/Code/tb/mozilla/dom/ipc/WindowGlobalChild.cpp:383
#3 0x00007fffef43ac0f in nsGlobalWindowOuter::SetNewDocument(mozilla::dom::Document*, nsISupports*, bool, mozilla::dom::WindowGlobalChild*)
(this=<optimized out>, aDocument=<optimized out>, aState=0x7fffe27c18b0, aForceReuseInnerWindow=<optimized out>, aActor=0x7fffe42a4c00)
at /home/magnus/Code/tb/mozilla/dom/base/nsGlobalWindowOuter.cpp:2225
#4 0x00007ffff0e262c7 in nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::dom::WindowGlobalChild*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool)
(this=0x7fffe04efac0, aParentWidget=<optimized out>, aState=0x0, aActor=0x0, aBounds=..., aDoCreation=<optimized out>, aNeedMakeCX=<optimized out>, aForceSetNewDocument=<optimized out>) at /home/magnus/Code/tb/mozilla/layout/base/nsDocumentViewer.cpp:983
#5 0x00007ffff0e25e2b in nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::dom::WindowGlobalChild*)
(this=0x7fffdd902ca0, aParentWidget=0x7fffdd902ca0, aBounds=..., aActor=0xb) at /home/magnus/Code/tb/mozilla/layout/base/nsDocumentViewer.cpp:743
#6 0x00007ffff1d1a8fd in nsDocShell::SetupNewViewer(nsIContentViewer*, mozilla::dom::WindowGlobalChild*) (this=0x7fffe0127800, aNewViewer=0x7fffe04efac0, aWindowActor=0x0)
at /home/magnus/Code/tb/mozilla/docshell/base/nsDocShell.cpp:8453
#7 0x00007ffff1d1a2e6 in nsDocShell::Embed(nsIContentViewer*, mozilla::dom::WindowGlobalChild*) (this=0x7fffe0127800, aContentViewer=0x7fffdd902ca0, aWindowActor=0x3)
at /home/magnus/Code/tb/mozilla/docshell/base/nsDocShell.cpp:6232
#8 0x00007ffff1d1d5a9 in nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIPrincipal*, nsIContentSecurityPolicy*, nsIURI*, bool, bool, mozilla::dom::WindowGlobalChild*)
(this=0x7fffe0127800, aPrincipal=<optimized out>, aStoragePrincipal=0x0, aCSP=
0x7fffe28658f0, aBaseURI=0x0, aTryToSaveOldPresentation=<optimized out>, aCheckPermitUnload=<optimized out>, aActor=0x0)
at /home/magnus/Code/tb/mozilla/docshell/base/nsDocShell.cpp:7067
#9 0x00007ffff1d1da0d in non-virtual thunk to nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIPrincipal*, nsIContentSecurityPolicy*) ()
at /home/magnus/Code/tb/mozilla/docshell/base/nsDocShell.cpp:7093
#10 0x00007fffee279f12 in NS_InvokeByIndex () at /home/magnus/Code/tb/mozilla/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106
#11 0x00007fffeed0ede7 in CallMethodHelper::Invoke() (this=<optimized out>) at /home/magnus/Code/tb/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1650
#12 0x00007fffeed0ede7 in CallMethodHelper::Call() (this=<optimized out>) at /home/magnus/Code/tb/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1187
#13 0x00007fffeed0ede7 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (ccx=..., mode=<optimized out>)
at /home/magnus/Code/tb/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1149
#14 0x00007fffeed10399 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (cx=<optimized out>, argc=2, vp=<optimized out>)
at /home/magnus/Code/tb/mozilla/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:943
#15 0x00007ffff2256e0b in CallJSNative(JSContext*, bool ()(JSContext, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)
(cx=<optimized out>, native=0x7fffeed10130 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, args=...)
at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:458
#16 0x00007ffff2256e0b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
(cx=0x7fffe6f2e000, args=..., construct=<optimized out>, reason=<optimized out>) at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:551
#17 0x00007ffff2250ad4 in js::CallFromStack(JSContext*, JS::CallArgs const&) (cx=0x7fffe6f2e000, args=...) at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:624
#18 0x00007ffff2250ad4 in Interpret(JSContext*, js::RunState&) (cx=<optimized out>, state=...) at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:3113
#19 0x00007ffff224696d in js::RunScript(JSContext*, js::RunState&) (cx=0x7fffe6f2e000, state=...) at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:424
#20 0x00007ffff225722a in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
(cx=0x7fffe6f2e000, args=..., construct=<optimized out>, reason=<optimized out>) at /home/magnus/Code/tb/mozilla/js/src/vm/Interpreter.cpp:592
#21 0x00007ffff2854822 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)
(cx=0x7fffe6f2e000, frame=0x7fffffffb020, stub=0x7fffde09d510, argc=<optimized out>, vp=0x7fffffffafd0, res=...) at /home/magnus/Code/tb/mozilla/js/src/jit/BaselineIC.cpp:3229

I tried to add to every position: absolute/fixed a display: block;. Still immediate crash. Then I exchanged every display: -moz-box; to display: block; but still the crash. So I don't know what I could do.

FYI, that crash cannot be related to this bug, fwiw. It seems all that code that's crashing landed in bug 1523638 recently, so you probably need to bisect it and see what broke it. Probably because of e10s being disabled.

The crash in comment 17 most definitely is triggered from this bug, if not directly caused by it. Backing out (only) 416de5a80d29 locally was the easiest way I had to get back into thunderbird to flip the pref back.

Huh, well, I'll shut my mouth now, but... that's quite bizarre.

I can try to debug thunderbird if you need it, just ni? me and I'll get to it eventually...

(In reply to Emilio Cobos Álvarez (:emilio) from comment #21)

I can try to debug thunderbird if you need it, just ni? me and I'll get to it eventually...

Yes, please if you have some time. I have no such skills to debug.

(In reply to Richard Marti (:Paenglab) from comment #18)

I tried to add to every position: absolute/fixed a display: block;. Still immediate crash. Then I exchanged every display: -moz-box; to display: block; but still the crash. So I don't know what I could do.

Did you take in account spots where it's being set from JS (X.style.position = "fixed/absolute") ? Also there are other cases where blockification happens where you'll also want to add display: block, see: https://groups.google.com/forum/#!topic/mozilla.dev.platform/IpRMO1A6uwg

It would be useful to make the change anyway regardless of the crash you're seeing, so the time doing this is definitively not wasted :)

(In reply to Tim Nguyen :ntim from comment #23)

(In reply to Richard Marti (:Paenglab) from comment #18)

I tried to add to every position: absolute/fixed a display: block;. Still immediate crash. Then I exchanged every display: -moz-box; to display: block; but still the crash. So I don't know what I could do.

Did you take in account spots where it's being set from JS (X.style.position = "fixed/absolute") ? Also there are other cases where blockification happens where you'll also want to add display: block, see: https://groups.google.com/forum/#!topic/mozilla.dev.platform/IpRMO1A6uwg

Still no luck with the two occurrencies at https://searchfox.org/comm-central/search?q=style.position&case=false&regexp=false&path= (editor/.. is no more used). Naturally together with the other position: absolute/fixed places.

I'm out now. This needs to be done from someone with debug skills.

Running thunderbird with the pref enabled insta-crashes with this assertion due to the styles applied to #header-view-toolbox.

To figure out the element, btw, I just had to do call mFrame->mContent->List((FILE*)stderr, 0) on rr.

With that fixed with:

diff --git a/mail/themes/linux/mail/messageHeader.css b/mail/themes/linux/mail/messageHeader.css
index da17b2d464..79daf703f0 100644
--- a/mail/themes/linux/mail/messageHeader.css
+++ b/mail/themes/linux/mail/messageHeader.css
@@ -284,6 +284,7 @@ mail-headerfield.headerValue:focus {
 }
 
 #header-view-toolbox {
+  display: block;
   float: right;
   padding-top: 2px;
 }

Thunderbird seems to start up just fine. Does that help? Or am I missing something? Do you want me to print the element that makes the assertions fail in debug builds? The assertion message could certainly be improved.

But I cannot repro the WindowGlobalChild bits above. Or maybe it was a release build and the assertion didn't fire or something.

Many thanks, Emilio. With this line added, TB starts without crash. I'll file a bug for the needed TB fixes.

Thanks Emilio! I was indeed running a non-debug build.
Unless the concerned code is something that will just go away soon, sounds like somewhere there should be a check to prevent this specific css to cause a crash.

This specific CSS is failing a fatal assertion. It will crash debug builds. It will also, apparently, crash opt builds.

Note that this can all happen only for privileged CSS, because CSS on the web can't set display: -moz-box to start with.

(In reply to Magnus Melin [:mkmelin] from comment #27)

Thanks Emilio! I was indeed running a non-debug build.
Unless the concerned code is something that will just go away soon, sounds like somewhere there should be a check to prevent this specific css to cause a crash.

The crash is intentionally here as a diagnostic tool so that you can confirm that nothing is relying on this behavior. You'll want to know any time this happens, because otherwise it would just silently break the layout in various places in the UI where you didn't make display: block explicit. Once you are past the startup crash then a try push will hopefully point you to individual tests / pieces of UI that are still relying on it.

The plan as I understand it is:

1. Flip the pref to true by default in Firefox (Bug 1582530)
2. Wait for a while to collect any reports of breakage
3. Remove the pref

I don't think there's a particular harm in delaying between 2 and 3 for a while so you have a chance to adapt to the new behavior, but we also shouldn't let this pref live on forever (since we will start relying on the new behavior for things like CSS grid conversions and if the pref gets flipped back it will lead to broken UI).

Won't this crash Thunderbird by add-on provided css though? Still supporting non-WX ones ... atm.

The crash is via MOZ_DIAGNOSTIC_ASSERT which is debug-only for the release and beta channels:
https://searchfox.org/mozilla-central/rev/153feabebc2d13bb4c29ef8adf104ec1ebd246ae/mfbt/Assertions.h#382-387

So non-nightly users shouldn't be affected.

If we do hit a crash for some XUL from an add-on in a Thunderbird nightly build, we should try to get the add-on adjusted with display:block as-needed... We'll probably remove/soften the assertions at some point, though; and if this change doesn't actually impact the add-on's layout in a user-impacting sort of way, then perhaps we don't need to bother messing with the add-on after all.