1610657 - Assertion hit when both RDM and Notes open at the same time.

Status: RESOLVED WORKSFORME

(Core :: Panning and Zooming, defect, P3)

Description

[Brad Werth [:bradwerth]]

Steps to Reproduce:

1. Install the Notes add-on from https://addons.mozilla.org/en-US/firefox/addon/notes-by-firefox.
2. Navigate to any page and open devtools' Responsive Design Mode.
3. Ensure that RDM Touch Simulation is enabled -- the button should be highlighted.
4. Choose the menu option: View -> Sidebar -> Notes

Assertion failure: !mUsingAsyncZoomContainer || !haveRootContentOutsideAsyncZoomContainer
#01: void mozilla::layers::APZCTreeManager::UpdateHitTestingTreeImpl<mozilla::layers::LayerMetricsWrapper>(mozilla::layers::LayerMetricsWrapper const&, bool, mozilla::layers::WRRootId, unsigned int)

This action has no obvious adverse effects in Release Firefox.

Comment 1

[Botond Ballo [:botond]]

The assertion is concerning but in the absence of visible adverse effects I'm not going to treat it with immediate priority.

I'm going to hang this off of the desktop zooming meta bug, since zooming is the scenario where the assertion failing is most likely to cause actual trouble.

Comment 2

[Botond Ballo [:botond]]

I've run into this with STR that doesn't involve a sidebar or addon:

1. In a debug build, navigate to any page and open RDM.
2. Click on the device selector dropdown, and change the selection to any other device.

Comment 3

[Botond Ballo [:botond]]

Attachment: Layer tree

Here's the offending layer tree with this STR

Comment 4

[Botond Ballo [:botond]]

So this is related to bug 1534459 after all. We don't have two nested async zoom containers because desktop zooming isn't enabled, but we do have two nested root content documents (an inner one for the content inside the RDM, and an outer one presumably for the document containing the RDM controls), and the inner one has an async zoom container, thereby tripping the assertion.

What we want here from APZ's point of view is to suppress the outer root content document (in the sense of marking it as FrameMetrics::mIsRootContent = false). Whatever mechanism we use to do that, we can also use to suppress the outer async zoom container with desktop zooming enabled, thereby fixing bug 1534459.

Comment 5

[Botond Ballo [:botond]]

One thing that's still a mystery to me is why it takes me switching to a new device in the device selector dropdown (or opening a sidebar in the original STR) for the outer RCD to appear.

Comment 6

[Botond Ballo [:botond]]

Setting devtools.responsive.browserUI.enabled=true appears to fix the STR from comment 2, but not the STR from comment 0.

Comment 7

[Botond Ballo [:botond]]

It probably still makes sense to investigate the STR from comment 0 with devtools.responsive.browserUI.enabled=true in place, since that it likely to change aspects of the document structure that are relevant to how we compute mIsRootContent.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I can't repro this assertion failure using either set of STR (comment 0 and comment 2). Tried with both WR and non-WR on macOS. Can you still repro?

Comment 9

[Brad Werth [:bradwerth]]

I can no longer reproduce it. It appears to be fixed.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Great, thanks! Feel free to reopen if it comes back.