Closed Bug 1705978 Opened 4 years ago Closed 3 years ago

Assertion failure: !haveNestedAsyncZoomContainers (Should not have nested async zoom container), at /Users/jyavenard/Work/Mozilla/gecko/gfx/layers/apz/src/APZCTreeManager.cpp:578 #01:

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
106 Branch
Tracking Flags:
Tracking Status
firefox-esr91 --- wontfix
firefox-esr102 --- wontfix
firefox104 --- wontfix
firefox105 --- wontfix
firefox106 --- fixed

People

(Reporter: jya, Assigned: hiro)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: intermittent-failure, regression, Whiteboard: [stockwell disable-recommended])

Attachments

(2 files)

Bug 1705978 - Part 1: Track whether BrowsingContext is embedded in a content document, r=smaug!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1705978 - Part 2: Check bc's GetEmbeddedInContentDocument in nsPresCOntext::IsRootContentDocument. r?botond
48 bytes, text/x-phabricator-request
Details | Review

STR:
In a local debug build.
Make sure Firefox Multi-Account containers is enabled.
Go into Add-Ons and Themes, click on [...] for Firefox Multi-Account containers, select "Preferences"

Firefox will crash with:

Assertion failure: !haveNestedAsyncZoomContainers (Should not have nested async zoom container), at /Users/jyavenard/Work/Mozilla/gecko/gfx/layers/apz/src/APZCTreeManager.cpp:578
#01: mozilla::detail::RunnableFunction<mozilla::layers::APZUpdater::UpdateScrollDataAndTreeState(mozilla::layers::LayersId, mozilla::layers::LayersId, mozilla::wr::Epoch const&, mozilla::layers::WebRenderScrollData&&)::$_28>::Run()[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x140e9cd]
#02: mozilla::layers::APZUpdater::ProcessQueue()[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x13ec8a3]
#03: mozilla::layers::APZUpdater::CompleteSceneSwap(mozilla::wr::WrWindowId const&, mozilla::wr::WrPipelineInfo const&)[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x13ec4d4]
#04: apz_post_scene_swap[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x13ee7b3]
#05: _$LT$webrender_bindings..bindings..APZCallbacks$u20$as$u20$webrender..renderer..SceneBuilderHooks$GT$::post_scene_swap::h73f6174105323343[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x74acdc5]
#06: webrender::scene_builder_thread::SceneBuilderThread::run::hc98d23f8f610941c[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x76879e3]
#07: std::sys_common::backtrace::__rust_begin_short_backtrace::h262d53df88cb8885[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x74bc3fd]
#08: _$LT$std..panic..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::hb217000d695fd344[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x7741d3b]
#09: std::panicking::try::hec7c34a241e09139[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x773199b]
#10: std::panic::catch_unwind::hb88dfec398b291d8[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x774277b]
#11: core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d$::h380211196377f1f0[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x75417ae]
#12: std::sys::unix::thread::Thread::new::thread_start::ha736b2d9de7b4dbc[/Users/jyavenard/Work/Mozilla/gecko/obj-x86_64-apple-darwin21.0.0/toolkit/library/build/XUL +0x874164d]
#13: _pthread_start[/usr/lib/system/libsystem_pthread.dylib +0x67a8]
Crash Annotation GraphicsCriticalError: |[C0][GFX1-]: Receive IPC close with reason=AbnormalShutdown (t=13.4802) [GFX1-]: Receive IPC close with reason=AbnormalShutdown
Exiting due to channel error.
Exiting due to channel error.
Crash Annotation GraphicsCriticalError: |[C0][GFX1-]: Receive IPC close with reason=AbnormalShutdown (t=14.003) [GFX1-]: Receive IPC close with reason=AbnormalShutdown
Exiting due to channel error.
Exiting due to channel error.
[Socket 2267, Main Thread] WARNING: Shutting down Socket process early due to a crash!: file /Users/jyavenard/Work/Mozilla/gecko/netwerk/ipc/SocketProcessChild.cpp:160
Flags: needinfo?(botond)

Both of about:addons (the tab's top level document, in process) and moz-extension://3030bc6f-ee9d-3d41-9efd-f706e0a2d206/options.html (the uri for the options of the containers addon, in another process) return true from IsRootContentDocumentCrossProcess, so they both think they are zoom able.

Blocks: 1534459, desktop-zoom-post
Severity: -- → S2
Flags: needinfo?(botond)
Priority: -- → P2

Do you know about how this is supposed to work and what might be going wrong?

Flags: needinfo?(kmaglione+bmo)

I actually hit this running toolkit/mozapps/extensions/test/browser/browser_html_options_ui.js locally, not sure how automation avoids hitting this assert. Very weird.

Confirmed, both issues are regressions from

294868c1cf03701f1d1d6cce6187579da5d1c6a9 Luca Greco — Bug 1525179 - Use aboutaddons.html at the root of about:addons. r=robwu

Regressed by: 1525179
Has Regression Range: --- → yes

Having nested root content documents is not something we want to happen. Can you shed light on whether this is something that should be fixed at the frontend level or the platform level?

Flags: needinfo?(lgreco)

(In reply to Timothy Nikkel (:tnikkel) from comment #5)

Having nested root content documents is not something we want to happen. Can you shed light on whether this is something that should be fixed at the frontend level or the platform level?

That's for sure an unfortunate special case that we have never been happy about, but it shouldn't be exactly a regression from Bug 1525179:

  • On Firefox < 68: we were still using the XUL about:addons page, in these versions there were 2 nested levels:
    • tab's browser "XUL about:addons" => browser "Extension Options UI page"
  • On Firefox >= 68 and < 87: we start to use the HTML about:addons view for almost everything in the about:addons page, but the top level frame was still a XUL page that was embedding the aboutaddons.html page, and so in these versions there were 3 nesting levels:
    • tab's browser "XUL about:addons" => browser "chrome aboutaddons.html HTML about:addons views" => browser "Extensions Options UI page"
  • On Firefox >= 87: we removed the XUL about:addons (which was now almost empty and all views actually loaded in the browser element at the first nesting level) and moved the existing aboutaddons.html view to the top level frame, and so we went back to having 2 nested levels again:
    • tab's browser "HTML-based about:addons" => browser "Extension Options UI page"

You may remember some questions about issues with these special "browser elements nesting" cases from mstriemer, while he was working on the HTML views that we are now using (I think that it may have been because we were re-evaluating a workaround we have been since the XUL about:addons page to prevent some issue with the popup positioning, due to the nested options UI page being scrollable on its own, and/or about some APZ-related assertions that one of our tests may have been triggering in debug builds).

At the moment we don't have plans to change the way the options_ui page are implemented, but (as one of the people investigating and fixing issues related to this being a very special case) I would be definitely open to at least evaluate if we do have other options and what those options would require (but I think that it may also be something that would be good to get an explicit product and/or ux sign-offs on before proceeding, because most options that I could think of would change either the design and/or the user flow).

Today I did also briefly discussed with mstriemer about some ideas I do have at the moment (but I'm definitely open for other ideas as well), I'm describing those here as a starting point:

  • option 1: we don't embed the options ui pages in about:addons anymore, we always open then in a new tab (as if the extension did configure it explicitly to do so by using the options_ui.open_in_tab manifest property)

    • from an implementation effort perspective, this would be the simplest option, as it would require us to remove some code and adapt some tests (and remove some test cases that would not be needed anymore)
    • from a sign-offs perspective, I think this may only require product sign-off because from a ux perspective the feature would work exactly like it does already work for extensions with options_ui.open_in_tab set to true in their manifest (and so not a new design or a completely new user flow)

  • option 2: embed the options_ui page in an overlay layer (e.g. inside the kind of tab modal that we are using for the printing dialog)

    • from an implementation perspective this may require quite some work (especially because I wouldn't be surprised that we may open some other cans of new special cases that needs to be handled explicitly to work as expected, just an an example if the options page is embedded in a "new style" tab modal then we have to figure out how to deal with the tab modals triggered by the extension options_ui page itself, e.g. when the extension page calls alert/confirm and friends)
    • from a sign-offs perspective, this would require both a ux sign-off (given that it would be a new design and a slightly different user flow) and product sign-off (given the amount of potential work needed and issues we may not see yet at this stage, and that some resources from outside of our team would likely be needed needed)

I'm going to double-check what are the opinions (and ideas) that the rest of my team may have about this (and the two options described above) and then comment back here about that too.

Flags: needinfo?(lgreco)

re-assigning a needinfo to myself, as a reminder to comment here about the perspective from the rest of my team.

Flags: needinfo?(lgreco)

Note that, from an APZ point of view, changing the page structure isn't necessarily a requirement.

The main issue for APZ is that root content documents are pinch-zoomable [note: this may not currently be hooked up for some about: pages, but the intent is that it will be], and we don't want two nested pinch-zoomable things.

If we can:

  • Agree on which of the two nested documents should be pinch-zoomable in this case; and
  • Add some sort of attribute / CSS property / annotation on the other one that tells APZ not to treat it as zoomable

that should be sufficient for APZ's purposes.

ah, that sounds great, and I can reply to it right now.

From this perspective, what we'll want is:

  • for the about:addons tab (the top level frame) to be the pinch-zoomable one
  • and the options-ui page to just follow the zoom level of the top level page that is embedding it

As an additional confirmation that this is the behavior that would be more reasonable, in Bug 1398481 emilio did apply some changes to make sure that when the about:addons page is zoomed we do also update the zoom level used inside the embedded options_ui frame, which is also consistent with the preferred behavior on the pinch-zoomable frame described above.

Flags: needinfo?(lgreco)

Ah, so the part of bug 1525179 that made us hit the assertion is changing from xul to html. With xul there is no root scroll frame, which likely disables pinch zooming, so we didn't hit the assert even though there were already nested root content documents.

If we want to continue having two nested things that are both IsRootContentDocumentCrossProcess we should rename that function to something else and do an audit of its uses to make sure it doesn't violate other assumptions besides the specific assert mentioned here.

Actually, if we are going to set a property on the browser we want async zooming on, why don't we instead set a property on the the descendant browser saying that it is not a root content document? And make IsRootContentDocumentCrossProcess follow that, then IsRootContentDocumentCrossProcess is still a useful function. Of course a platform level fix that addresses this would be better, but I'm not sure if that is feasible or not.

Yeah, I totally agree with Timothy. We highly rely on IsRootContentDocumentCrossProcess in these days, I've confirmed drop down positions in the Multi-Account containers preferences are wrong because the function returns true for the inlined preference document.

I don't have much to add at this point, except to say that drop-down positions are somewhat flaky in those inline options panes even at the best of times, at this point.

Flags: needinfo?(kmaglione+bmo)

Setting Fission Milestone flag? since the preference content is broken on Fission, when pinch zoom in/out on the content, the content gets zoomed instead of the about:addons window.

Fission Milestone: --- → ?

Dropping the Fission flag since the assertion can happen with/without Fission.

Fission Milestone: ? → ---

We (APZ team) discussed this bug today and decided to decrease the severity to S3 since we believe the user-noticeable effects (outside of a debug build where the assertion causes a crash) are limited to the scenario of pinch-zooming on such an add-on preferences page which is not a common or critical interaction.

A solution here would involve collaborating with folks with more knowledge of the DOM / browser side to either:

  1. Add an attribute or CSS property that influences that return value of IsRootContentDocumentCrossProcess() as described in comment 8
  2. Get IsRootContentDocumentCrossProcess() to return a value that more accurately reflects the reality without the need for any annotation of the sort proposed in #1 (this is likely to involve changes at the BrowsingContext level)
  3. Change the page structure as described in comment 6
  4. Something else

Note that, as long as about:addons continues to be loaded in the parent process, the ability to pinch-zoom it will also depend on bug 1648427.

Severity: S2 → S3
See Also: → 1648427

This bug has increased frequency since the 1st of August.
Retriggers and backfills point to Bug 1742842.

Florian, can you take a look please?
Thank you.

Flags: needinfo?(florian)

Not sure if investigating this via bug 1742842 is going to be fruitful. Seems like something just upset the delicate balance of somehow not hitting this assert in automation (I'm surprised we don't hit this assert in every run actually). Fixing the root problem here might be the best way to go.

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

Both of about:addons (the tab's top level document, in process) and moz-extension://3030bc6f-ee9d-3d41-9efd-f706e0a2d206/options.html (the uri for the options of the containers addon, in another process) return true from IsRootContentDocumentCrossProcess, so they both think they are zoom able.

This is the root problem. Opening about:addons and opening the options for an addon will load two documents as parent/child and they both return true for IsRootContentDocumentCrossProcess. We have lots of logic (in apz, layout) depending on only one document in a document tree being the root content document cross process. This gets into BrowsingContext stuff that I'm not very familiar with. Perhaps Nika can suggest someone to look into this?

Flags: needinfo?(nika)

(In reply to Timothy Nikkel (:tnikkel) from comment #28)

Not sure if investigating this via bug 1742842 is going to be fruitful. Seems like something just upset the delicate balance of somehow not hitting this assert in automation (I'm surprised we don't hit this assert in every run actually).

Bug 1742842 hasn't changed anything to what's happening during the tests. The only difference is that now we wait until vsync is disabled before starting the next test. This can take a few hundred milliseconds, so that gives the previous test that amount of time to run more cleanup tasks.

I agree there is not much room to investigate this failure from the angle of bug 1742842, which just made an existing intermittent failure more likely to happen.

Flags: needinfo?(florian)

(In reply to Timothy Nikkel (:tnikkel) from comment #28)

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

Both of about:addons (the tab's top level document, in process) and moz-extension://3030bc6f-ee9d-3d41-9efd-f706e0a2d206/options.html (the uri for the options of the containers addon, in another process) return true from IsRootContentDocumentCrossProcess, so they both think they are zoom able.

This is the root problem. Opening about:addons and opening the options for an addon will load two documents as parent/child and they both return true for IsRootContentDocumentCrossProcess. We have lots of logic (in apz, layout) depending on only one document in a document tree being the root content document cross process. This gets into BrowsingContext stuff that I'm not very familiar with. Perhaps Nika can suggest someone to look into this?

This behaviour was intentional and has been the case since e10s. The about:addons page is in the parent process, and in order to have remote untrusted content embedded in it, it loads that content using a xul browser with remote=true specified. This ends up creating a new BrowsingContext tree detached from the previous one (similar to the old <iframe mozbrowser> element). This was done originally in e10s due to the process divide and not being able to have cross-process iframes, and has been preserved into Fission as we don't support mixing the parent process and content processes within a single BrowsingContext tree.

There are actually multiple places which do this beyond about:addons, with even more in thunderbird (there's a strict allowlist for pages which are able to do this: https://searchfox.org/mozilla-central/rev/dcb0cfb66e4ed3b9c7fbef1e80572426ff5f3c3a/dom/base/nsFrameLoader.cpp#2665-2674), and it's also possible to do in mochitest-chrome.

I don't fully understand the requirements of this apz/layout code and what specifically it relies on, but perhaps we can set up some time to talk about what's actually needed and potential alternative solutions here. We could perhaps add some flags to the content process BrowsingContexts that indicate they should zoom as part of their parent process container document?

Flags: needinfo?(nika) → needinfo?(tnikkel)

(In reply to Nika Layzell [:nika] (ni? for response) from comment #32)

We could perhaps add some flags to the content process BrowsingContexts that indicate they should zoom as part of their parent process container document?

Yes, based on comment 9 (which says it's the parent RCD that should be zoomable), if we can add a flag to the BrowsingContext of the descendant RCD that says it should not be (independently) zoomable, that should allow us to easily resolve the issue on the APZ side (we'd have IsRootContentDocumentCrossProcess check the flag and return false if it's set).

There have been 47 total failures in the last 7 days, recent failure log.
Affected platforms are:

  • linux1804-64-qr
  • macosx1015-64-qr
  • windows10-32-2004-qr

(In reply to Cosmin Sabou [:CosminS] from comment #41)

This hasn't failed since 29th of August: https://treeherder.mozilla.org/intermittent-failures/bugdetails?startday=2022-08-27&endday=2022-09-03&tree=all&bug=1705978

The bug 1787703 with the same assert has failed plenty in the last week.

I believe the next step towards a fix here would be to do what is discussed in comment 35: "add some flags to the content process BrowsingContexts that indicate they should zoom as part of their parent process container document".

Nika, would you be able to help us with that, or know someone who can? Once that is in place, I can make the needed APZ change that will complete the fix.

Flags: needinfo?(tnikkel) → needinfo?(nika)
Assignee: nobody → nika
Status: NEW → ASSIGNED

(In reply to Botond Ballo [:botond] from comment #45)

I believe the next step towards a fix here would be to do what is discussed in comment 35: "add some flags to the content process BrowsingContexts that indicate they should zoom as part of their parent process container document".

Nika, would you be able to help us with that, or know someone who can? Once that is in place, I can make the needed APZ change that will complete the fix.

I've attached a patch to add something which I think does what you're asking.

Assignee: nika → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(nika)
Assignee: nobody → nika
Status: NEW → ASSIGNED
Assignee: nika → botond
Whiteboard: [stockwell disable-recommended] → [stockwell needswork:owner]

I've pushed a try run with the APZ part change; https://treeherder.mozilla.org/jobs?repo=try&revision=f1a3c95cc06aff9edfe4bfca5eb1e4f111cebc62, and pushed another try run with retriggering browser mochitests in question; https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=C-YzU_bCShiiGnToRErl3A.0&revision=14be1fd2f673f7531a11a3b5a9b564d2d29344b9. Looks like it works.

Stealing from Botond.

Assignee: botond → hikezoe.birchill

With the check now we can tell the top level content document properly.

Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/33f01b928f13 Part 1: Track whether BrowsingContext is embedded in a content document, r=smaug https://hg.mozilla.org/integration/autoland/rev/2f074155062c Part 2: Check bc's GetEmbeddedInContentDocument in nsPresCOntext::IsRootContentDocument. r=botond

https://hg.mozilla.org/mozilla-central/rev/33f01b928f13
https://hg.mozilla.org/mozilla-central/rev/2f074155062c

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox106: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
See Also: → 1821985
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: