366583-1.xul asserts in presence of -moz-window-inactive selector w/ release milestone.txt: "ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.: '!placeholder || ..."

RESOLVED INCOMPLETE

Status

()

defect
P3
critical
RESOLVED INCOMPLETE
2 years ago
Last year

People

(Reporter: RyanVM, Unassigned)

Tracking

Trunk
All
Windows
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59+ wontfix)

Details

+++ This bug was initially created as a clone of Bug #1426797 +++

This issue was originally noted in bug 1426797 comment 1, but comment 2 suggests that this should probably tracked in its own bug. I've been working around this by just annotating the assertions so far, but it would be good for someone to look into them before going the wallpapering route.
Jet, I'm happy to annotate this failure away if the assertion is deemed harmless, but I would appreciate someone from the Layout team making that judgement first. Can you please have someone take a look at this before Thursday's uplift to Beta? Thanks!
Flags: needinfo?(bugs)
FWIW, I'm pretty sure this *is* related to bug 1426797 (possibly the same underlying cause) -- it's some funny business with :-moz-window-inactive selectors causing stylo to misbehave, specifically in release-milestone builds.

(As noted in bug 1426797, we recently introduced a :-moz-window-inactive selector into a global XUL stylesheet *on Windows* - hence the windows-specific nature of this bug.)

As in bug 1426797, it seems that this issue can be made to reproduce on all platforms (debug-only, because this is an assertion) by doing the following:
 a) tweaking config/milestone.txt
 b) adding a :-moz-window-inactive selector to this testcase

Here's a Try run demonstrating this:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c69764ca4c108d3d1960347ad6139e9187d9ffe

(I'm using bug 1426797's repro-patch to do (a), and then I added an extra patch to do (b).)
See Also: → 1426797
Also: locally, I cannot reproduce this bug's crashtest assertion-failure in a "standard" debug build, i.e. one with optimizations disabled (--enable-debug --disable-optimize).

But I *can* reproduce locally in a debug+opt build (--enable-debug --enable-optimize), which I think is what Try is using as well.

Transferring needinfo=jet to xidorn, since this bug will likely get the same outcome as bug 1426797, since they seem to have the same underlying cause.
Flags: needinfo?(bugs) → needinfo?(xidorn+moz)
Summary: 366583-1.xul is going to permafail on Windows when Gecko 59 merges to Beta on 2018-01-11 → 366583-1.xul is going to permafail on Windows when Gecko 59 merges to Beta on 2018-01-11 (asserts in presence of -moz-window-inactive selector w/ release milestone.txt)
I'm going to land the wallpaper for now to avoid orange on tomorrow's uplift. Please remember to revert this change once the underlying issue is resolved.
Keywords: leave-open
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4ac537f9a4
Annotate 366583-1.xul to allow for up to 1 assertion on Windows. a=test-only
I can reproduce this issue with the steps in comment 3, and I suspect this is just an existing issue uncovered by :-moz-window-inactive which triggers a full page restyle.

What's happening here is that, the placeholder is for a <menupopup> in <treecolpicker> [1] inside the XBL binding of <treecols>, while the out-of-flow frame's parent is an nsPopupSetFrame for <popupgroup>. Given some of the code in nsCSSFrameConstructor [2] it seems this separation is intentionally, and if so, the assertion itself isn't correct for popup OOFs, since one can destroy a subtree which contains the OOF but not the placeholder.

I'm not familiar with how popup works, so I don't know what is supposed to happen here. Maybe bz or mats has some idea here.


[1] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/toolkit/content/widgets/tree.xml#1494-1497
[2] https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/layout/base/nsCSSFrameConstructor.cpp#1066
Flags: needinfo?(xidorn+moz)
Also, when bug 1409672 gets fixed, this issue should be wallpapered again, since we would no longer do a full page restyle, but instead only restyle affected elements, which there is none in this testcase.
I really don't understand how non-root popups are supposed to work....
Flags: needinfo?(mats)
When filing bugs about assertions it's always a good idea to include
the assertion message in the report to make it possible to query for it.

I'm guessing this bug is about this assertion in bug 1426797 comment 1:
ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.: '!placeholder || nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, placeholder)', file z:/build/build/src/layout/generic/nsFrame.cpp, line 767
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> I really don't understand how non-root popups are supposed to work....

I'm guessing Neil probably knows more about them than me.
Flags: needinfo?(mats)
(In reply to Mats Palmgren (:mats) from comment #10)
> I'm guessing this bug is about this assertion in bug 1426797 comment 1:
> ASSERTION: Placeholder relationship should have been torn down already

(You're correct -- that's the assertion I was hitting for this crashtest in the Try run that I linked in comment 2, at least. I'll add a truncated version of that to the bug summary and drop the stale "is going to..." text, so that the summary fits the requirement of being <= 255 chars.)
Summary: 366583-1.xul is going to permafail on Windows when Gecko 59 merges to Beta on 2018-01-11 (asserts in presence of -moz-window-inactive selector w/ release milestone.txt) → 366583-1.xul asserts in presence of -moz-window-inactive selector w/ release milestone.txt: "ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.: '!placeholder || ..."
Tracking for 59, hoping we can clear this up while 59 is in beta.
Neal, is there anything actionable here for 59?
Flags: needinfo?(enndeakin)
I don't think there is anything I can do, no.

The description in comment 7 seems correct, although I wouldn't think it would be possible to destroy the popup frame without the placeholder frame.
Flags: needinfo?(enndeakin)
I don't see any signs of this assertion on recent Try simulation runs. I think we should try reverting this now and see what happens. Either way, I don't there's need to keep tabs on this for 59 at this point.
Flags: needinfo?(aryx.bugmail)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea423e4b7856
Backed out changeset ba4ac537f9a4 because the assertion didn't reproduce anymore
Comment 8 makes this sound like it's not really fixed, just covered up again. That said, I don't think that the bug as currently filed makes much sense anymore. Should we revise the summary to something more appropriate or just close this out and file a new bug for fixing the root cause?
If we can revise the testcase to have it assert consistently, probably it's worth a separate bug to handle, otherwise... I have no idea what should we do.
Xidorn -- Should we close this bug?
Flags: needinfo?(xidorn+moz)
Priority: P1 → P3
As I've mentioned in comment 20, it's not really clear to me what to do with this bug. If we don't have a testcase which can trigger this, probably we should just close it as WORKSFORME, I guess.
Flags: needinfo?(xidorn+moz)
Thanks, Xidorn.  I'm going to resolve it as INCOMPLETE.  If someone comes up with a testcase to trigger this, we can reopen.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.