1448485 - PrepareForSetTargetAPZCNotification hits if(!scrollAncestor) MOZ_ASSERT(false);

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Running local debug build under visual studio 2017, made from central commit 5bc910c1f477. Noticed issue starting on central 7771df14ea18. When starting up with my debug profile I get the restore session page (with history sidebar open). Clicking anywhere in the browser window hits the assert mentioned in the title[0].

Can work around by using only the keyboard to navigate to another page, at which point I can click within Firefox again without issue.

[0]: https://searchfox.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp#678

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Easier STR: this happens when I use `./mach run` (commit 7b9da7139d94).

Comment 2

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Kats, I'm able to reproduce this as well and collected some info.

When (!scrollAncestor) happens and we assert false:

target != nullptr
nsLayoutUtils::GetAsyncScrollableAncestorFrame(target) == nullptr
aRootFrame->PresShell()->GetRootScrollFrameAsScrollable() == nullptr
guidIsValid == true
nsLayoutUtils::HasDisplayPort(dpElement) == false

It seems like the theory in the comment after the assert is correct then, and there is no display port. So does it make sense to remove the assert here?

I can gather any more data that you want.

Comment 3

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

So if we have a root element, the displayport on it is supposed to be initialized at [1] during startup. It would be good to step through that function and see if it's failing - and if it's not, check to see if the document element at that point matches the dpElement at the point of the assertion. If they're not the same then it must be that the document element got swapped. Either way, yes, we'll probably want to remove the assertion, but it would be good to update the comment with the exact scenario we're hitting.

[1] https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/gfx/layers/apz/util/ChromeProcessController.cpp#52

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I hit this a lot lately, most of time it crashes seconds after the window shows up.

It sometimes can happen consistently until I remove the profile.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Oh and it sometimes doesn't happen automatically, but happens when I have any interaction with the window, e.g. click something, even the hamburger menu button.

Comment 6

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Indeed it looks like ChromeProcessController::InitializeRoot is being run and is succeeding, but the documentElement is different by the time PrepareForSetTargetAPZCNotification is being run.

I'll add a patch updating the comment and removing the assert.

Comment 7

[Ryan Hunt [:rhunt] (PTO Sept 7-11)]

Attachment: apz-callback-helper.patch

Hmm, push to review isn't working on my windows machine, falling back to just a patch.

Comment 8

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

Comment on attachment 8962884 [details] [diff] [review]
apz-callback-helper.patch

Review of attachment 8962884 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +681,4 @@
>      APZCCH_LOG("Widget %p's document element %p didn't have a displayport\n",
>          aWidget, dpElement.get());
>      APZCCallbackHelper::InitializeRootDisplayport(aRootFrame->PresShell());
>      return false;

Change this to a |return true|. This is a pre-existing problem, but since we're now going down this codepath regularly we might as well fix it.

Comment 9

[Honza Bambas (:mayhemer)]

Is it still worth finding out the regressing changeset? (bug 1448917)

Comment 10

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

If it's not a lot of work I would still be interested to know which change introduced this. It could be a bug.

Comment 11

[Florian Quèze [:florian]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> If it's not a lot of work I would still be interested to know which change
> introduced this. It could be a bug.

The comment inside the patch matches what we are doing. We are loading asap a window with about:blank and later during startup changing the location to chrome://browser/content/browser.xul, which indeed changes the document element.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d247533c6dee
Remove assert in APZCallbackHelper and add comment about case it was triggered by. r=kats

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d247533c6dee