Bugzilla

Stefan Hindli [:stefan_hindli]

Comment 2

•

5 years ago

There are 47 total failures in the last 4 days on android-em-7-0-x86_64 debug

Recent failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248071606&repo=mozilla-central&lineNumber=3463

Flags: needinfo?(wkocher)

Comment hidden (Intermittent Failures Robot)

Comment 7

•

5 years ago

There are 85 failures on this bug over the last 7 days. These happen only on android-em-7-0-x86_64.

Here is the most recent failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249543448&repo=mozilla-central&lineNumber=3656

Flags: needinfo?(htsai)

Comment hidden (Intermittent Failures Robot)

Hsin-Yi Tsai (she/her) [:hsinyi]

Comment 9

•

5 years ago

:kats would know better here. :)

Flags: needinfo?(htsai) → needinfo?(kats)

Comment 10

•

5 years ago

According to the line number in the crash traces, it's failing this assertion. Not really obvious to me why but I can try to repro locally and debug. /cc Botond as well who might have an idea off the top of his head.

Note that android-em-7-0-x86_64 is running on the geckoview test app with e10s enabled on Android, so this is maybe just a configuration we neglected to properly handle.

Assignee: nobody → kats

Flags: needinfo?(kats)

Assignee

Updated

•

5 years ago

Depends on: 1556517

Assignee

Updated

•

5 years ago

No longer depends on: 1556517

Comment 11

•

5 years ago

Attached file Layers dump of the crashing page (deleted) — Details

Comment 12

•

5 years ago

Botond understands this code better and can also reproduce now, reassigning to him.

Assignee: kats → botond

Comment 13

•

5 years ago

Also for posterity, the steps to repro locally are:

put this in a regular fennec/android mozconfig: ac_add_options --target=x86_64-linux-android
./mach build && ./mach package && ./mach gradle geckoview:installWithGeckoBinariesDebugAndroidTest
./mach android-emulator --version x86-7.0
./mach wpt --package-name org.mozilla.geckoview.test testing/web-platform/tests/html/browsers/history/the-history-interface/traverse_the_history_write_after_load_1.html and say "no" when it asks to install the test app

Assignee

Comment 14

•

5 years ago

Attached file Display list dump (deleted) — Details

And here's the corresponding content-side display list dump.

Assignee

Updated

•

5 years ago

Flags: needinfo?(wkocher)

Assignee

Comment 15

•

5 years ago

I guess we're hitting the codepath for setting a fallback ScrollMetadata on the root layer. I don't think we've considered the interactions of this codepath with having an async zoom container.

Assignee

Comment 16

•

5 years ago

So the fallback codepath in question basically traverses the layer tree after layer building to see if any layers have scroll metadata for the root content element; if none do (e.g. because the page doesn't have any scrollable content), it adds such scroll metadata to the root container layer.

This was a reasonable thing to do with container scrolling, but no longer is with containerless scrolling.

It's not immediately clear what we should do with containerless srolling. Some possible options are:

Remove the async zoom container. This is probably not what we want, since even if page doesn't have scrollable content, it could have fixed content that still needs to be zoomed by the async zoom container.
Put the root metadata on the async zoom container. This breaks current APZ invariants, but we could probably teach APZ to swallow it if need be.
Invent a descendant layer of the async zoom container (such as an additional ContainerLayer wrapping its children) to house the root scroll metadata.

Markus, any thoughts on this? Does option 3 seem reasonable / easy to do? Or should we try to pursue option 2 instead?

Blocks: 1534458

Flags: needinfo?(mstange)

Comment hidden (Intermittent Failures Robot)

Markus Stange [:mstange]

Comment 19

•

5 years ago

I would prefer pursuing option 2. Inventing a layer for something that doesn't exist seems like a workaround.

What's the purpose of the fallback scroll metadata? If the page is not scrollable, does the fallback scroll metadata only exist for the purposes of zooming, and for scrolling the zoomed container? In that case, the async zoom container really seems like the natural place to put it. I could go even further, but feel free to ignore this part: We could think about splitting scroll metadata into two structs, one containing the pieces that are needed only for scrollable pages and the other containing the pieces that are needed zooming of all pages, and then set the latter always on the async zoom container and not just in the fallback case.

Flags: needinfo?(mstange)

Edgar Chen [:edgar]

Updated

•

5 years ago

Priority: -- → P2

Bogdan Tara[:bogdan_tara | bogdant]

Assignee

Comment 20

•

5 years ago

(In reply to Markus Stange [:mstange] from comment #19)

What's the purpose of the fallback scroll metadata? If the page is not scrollable, does the fallback scroll metadata only exist for the purposes of zooming, and for scrolling the zoomed container? In that case, the async zoom container really seems like the natural place to put it.

It ensures that we create an APZC for the content area. Having an APZC is important not just for zooming and scrolling the zoomed container, but also things like hit testing and event delivery.

I could go even further, but feel free to ignore this part: We could think about splitting scroll metadata into two structs, one containing the pieces that are needed only for scrollable pages and the other containing the pieces that are needed zooming of all pages, and then set the latter always on the async zoom container and not just in the fallback case.

This sounds like a significantly more involved change which I'm not going to entertain for the purposes of fixing this bug, but we could consider it as a future enhancement, especially if there are additional motivations for it.

I'm happy to try option 2 from comment 16 for the purpose of fixing this bug.

Updated

•

5 years ago

Whiteboard: [stockwell disable-recommended]

Comment hidden (Intermittent Failures Robot)

Comment 22

•

5 years ago

This is perma-failing on tier2. Botond, are you working on the fix mentioned in the previous comment?

Flags: needinfo?(botond)

Summary: Intermittent tier 2 mozrunner-startup | application crashed [@ mozilla::layers::APZCTreeManager::UpdateHitTestingTreeImpl<mozilla::layers::LayerMetricsWrapper>(mozilla::layers::LayersId, mozilla::layers::LayerMetricsWrapper const&, bool, mozilla::layers:: → Perma tier 2 mozrunner-startup | application crashed [@ mozilla::layers::APZCTreeManager::UpdateHitTestingTreeImpl<mozilla::layers::LayerMetricsWrapper>(mozilla::layers::LayersId, mozilla::layers::LayerMetricsWrapper const&, bool, mozilla::layers::

Assignee

Comment 23

•

5 years ago

I am, but it might take a few days to prepare. If the perma-fail is making life difficult in the meantime, please feel free to disable the tier 2 test for the time being, and I will re-enable it as part of my patch.

Flags: needinfo?(botond)

Comment 24

•

5 years ago

I looked in 10 most recent failure logs and this is failing in different tests:

html/browsers/browsing-the-web/unloading-documents/002.html

html/browsers/history/the-history-interface/traverse_the_history_write_after_load_1.html

html/browsers/history/the-location-interface/reload_document_open_write.html

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/ignore-opens-during-unload.window.html

Joel any ideas here until the fix is ready?

Flags: needinfo?(jmaher)

Assignee

Comment 25

•

5 years ago

Attached file Bug 1553045 - Have APZ support the RCD-RSF scroll metadata being on the async zoom container layer. r=kats (deleted) — Details

This mostly already works, e.g. AsyncCompositionManager will apply both parts
of the transform independently, just a check in APZCTreeManager::
ComputeTransformForNode() needed to be adjusted.

Assignee

Comment 26

•

5 years ago

Attached file Bug 1553045 - Place the fallback scroll metadata onto the async zoom container layer if there is one. r=kats (deleted) — Details

This preserves the APZ invariant that if there is an async zoom container,
then the RCD-RSF scroll metadata is on the same layer or descendant layers.

Depends on D34253

Assignee

Comment 27

•

5 years ago

(In reply to Andreea Pavel [:apavel] from comment #24)

I looked in 10 most recent failure logs and this is failing in different tests:

html/browsers/browsing-the-web/unloading-documents/002.html

html/browsers/history/the-history-interface/traverse_the_history_write_after_load_1.html

html/browsers/history/the-location-interface/reload_document_open_write.html

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/ignore-opens-during-unload.window.html

Joel any ideas here until the fix is ready?

The fix is up for review now, so hopefully disabling things won't be necessary.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da83b7fb047ffbc17c896708b6ab8edc7e98067e

Assignee

Comment 28

•

5 years ago

Comment 29

•

5 years ago

Botond that was fast, thank you.

Removing ni for Joel for now.

Flags: needinfo?(jmaher)

Comment hidden (Intermittent Failures Robot)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49198f1393e51e92a75fda5653487a4a758b5193

Assignee

Comment 31

•

5 years ago

This is looking better: