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

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: bryce, Assigned: rhunt)

Tracking

unspecified
mozilla61
x86_64
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

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
Easier STR: this happens when I use `./mach run` (commit 7b9da7139d94).
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.
Component: Graphics: Layers → Panning and Zooming
Flags: needinfo?(bugmail)
Whiteboard: [gfx-noted]
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
Flags: needinfo?(bugmail)
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.
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.
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.
Assignee: nobody → rhunt
Hmm, push to review isn't working on my windows machine, falling back to just a patch.
Attachment #8962884 - Flags: review?(bugmail)
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.
Attachment #8962884 - Flags: review?(bugmail) → review+
Is it still worth finding out the regressing changeset? (bug 1448917)
Flags: needinfo?(bugmail)
If it's not a lot of work I would still be interested to know which change introduced this. It could be a bug.
Flags: needinfo?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d247533c6dee
Remove assert in APZCallbackHelper and add comment about case it was triggered by. r=kats
https://hg.mozilla.org/mozilla-central/rev/d247533c6dee
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.