1535990 - Crash in [@ mozilla::dom::Flex::Flex] when inspecting the login autocomplete footer

Status: RESOLVED DUPLICATE of bug 1563163

(Core :: Layout: Flexbox, defect, P3)

Description

[Matthew N. [:MattN]]

Attachment: Screen Shot of the Browser Toolbox when the browser crashes

This bug is for crash report bp-b21d90dd-522d-4452-8c98-cb1460190318.

This was reproducible for me 4 times in a row in a new profile on Nightly 2019-03-17:

STR:

1. Enable the prefs signon.showAutoCompleteFooter
2. Enable chrome and remote debugging in devtools
3. Open the Browser Toolbox
4. Load https://wiki.mozilla.org/index.php?title=Special:UserLogin&returnto=Main+Page
5. Focus the username or password field so the login autocomplete popup appears.
6. In the Browser Toolbox Inspector tab, tyoe loginsfooter in the search box.
7. Hit Enter to search for a matching node

Expected result:
Node is selected and I can inspect it

Actual result:
Node is selected and the browser crashes.

Top 10 frames of crashing thread:

0 XUL mozilla::dom::Flex::Flex xpcom/ds/nsTArray.h:344
1 XUL mozilla::dom::Element_Binding::getAsFlexContainer dom/flex/Flex.cpp:24
2 XUL bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3144
3 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:442
4 XUL js::ForwardingProxyHandler::call const js/src/vm/Interpreter.cpp:605
5 XUL js::CrossCompartmentWrapper::call const js/src/proxy/CrossCompartmentWrapper.cpp:238
6 XUL js::Proxy::call js/src/proxy/Proxy.cpp:503
7 XUL js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:508
8 XUL Interpret js/src/vm/Interpreter.cpp:593
9 XUL js::RunScript js/src/vm/Interpreter.cpp:422

Comment 1

[Matthew N. [:MattN]]

I forgot to say that I think this is caused the the use of display:flex within XUL flexbox. I'm backing out the use of display:flex in bug 1533206 comment 7 so you'll have to test without the 2 new commits in order to reproduce after that.

Comment 2

[Brad Werth [:bradwerth]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #1)

I forgot to say that I think this is caused the the use of display:flex within XUL flexbox. I'm backing out the use of display:flex in bug 1533206 comment 7 so you'll have to test without the 2 new commits in order to reproduce after that.

I see that the intended bug/comment is Bug 1533206 comment 6.

Comment 3

[Matthew N. [:MattN]]

(In reply to Brad Werth [:bradwerth] from comment #2)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #1)

I forgot to say that I think this is caused the the use of display:flex within XUL flexbox. I'm backing out the use of display:flex in bug 1533206 comment 7 so you'll have to test without the 2 new commits in order to reproduce after that.

I see that the intended bug/comment is Bug 1533206 comment 6.

No, the backout was Bug 1533206 comment 7. The problematic implementation was in Bug 1533206 comment 6.

Comment 4

[Brad Werth [:bradwerth]]

I can reproduce this, after reapplying https://phabricator.services.mozilla.com/D23395. In a debug build, we hit the assert at https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/layout/generic/nsFlexContainerFrame.cpp#4419 which indicates that we weren't able to generate a FlexContainerInfo structure.

I'll keep digging.

Comment 5

[Brad Werth [:bradwerth]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #0)

STR:

1. Enable the prefs signon.showAutoCompleteFooter
2. Enable chrome and remote debugging in devtools
3. Open the Browser Toolbox
4. Load https://wiki.mozilla.org/index.php?title=Special:UserLogin&returnto=Main+Page
5. Focus the username or password field so the login autocomplete popup appears.

In order to reproduce, I have to follow these steps after Step 5 above:

6. In Browser Toolbox Inspector, select the Rules sub-tab.
7. In Browser Toolbox, click the Element Picker.
8. In the content page, select the pop-up element. Note that the Browser Toolbox Rules panel updates.
9. In Browser Toolbox Inspector, select the Layout sub-tab.

That will trigger the crash. What's happening is that marking the frame as dirty is not leading to a reflow. The reflow is necessary to generate the FlexComputedInfo structure. The reason why the reflow fails to trigger is because the parent of the nsFlexContainerFrame is already marked as dirty (somehow), though the presShell is not tracking any dirty roots. In more detail:

• We hit https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/layout/generic/nsFlexContainerFrame.cpp#4406.
• In nsIPresShell::FrameNeedsReflow(), the first parent of the nsFlexContainerFrame -- an nsHTMLScrollFrame -- is noted as being dirty at https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/layout/base/PresShell.cpp#2722
• In debug builds, we call VerifyHasDirtyRootAncestor() which early exits from a clause that mentions Bug 372769. If it continued, it would hit an assert because there is nothing in mDirtyRoots.
• When we later call nsIPresShell::FlushPendingNotifications, nsIPresShell::NeedFlush finds that mDirtyRoots is empty, so returns false. The flush does nothing.
• Debug builds will then assert at https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/layout/generic/nsFlexContainerFrame.cpp#4419. Release builds will crash in the mozilla::dom::Flex constructor.

So the issue is that the parent frame of the nsFlexContainerFrame is marked dirty without its ancestor being added to the presShell mDirtyRoots. I'll continue to investigate. Needinfo'ing dholbert in case I'm missing something.

Comment 6

[Brad Werth [:bradwerth]]

Also: if the Browser Toolbox Inspector is already on the Layout sub-tab before inspecting the element, there is no crash. The reflow triggers properly under those conditions.

Comment 7

[Daniel Holbert [:dholbert]]

I haven't been able to reproduce yet, so I don't have any extra debugging insight.

I used a local debug build from a source tree updated to https://hg.mozilla.org/mozilla-central/rev/db4a1fa6c07c (the patch that introduced the crash-triggering bit of UI, as far as I understand), and I tried STR from comment 0 and comment 5, but neither triggered the crash for me.

Anyway: I don't know how we get into the scenario described in comment 5 (frames marked as dirty but no dirty roots); but if we can't figure it out, one possible solution would be to just convert this assertion
https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/layout/generic/nsFlexContainerFrame.cpp#4419

...into an early "return null" bail-out scenario, which then would make getAsFlexContainer bail out instead of looking for the not-actually-present layout info in the property-table.

Comment 8

[Brad Werth [:bradwerth]]

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

Anyway: I don't know how we get into the scenario described in comment 5 (frames marked as dirty but no dirty roots); but if we can't figure it out, one possible solution would be to just convert this assertion
https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/layout/generic/nsFlexContainerFrame.cpp#4419

...into an early "return null" bail-out scenario, which then would make getAsFlexContainer bail out instead of looking for the not-actually-present layout info in the property-table.

Yes, I agree that's an approach that would prevent the crash, but there wouldn't be any Flex info in the toolbox either. I'm hoping to find a solution that makes the reflow happen correctly.