Crash in [@ ExpandHeightForDynamicToolbarImpl<T>]
Categories
(Core :: Layout, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox85 | --- | wontfix |
firefox86 | --- | fixed |
firefox87 | --- | fixed |
People
(Reporter: fluffyemily, Assigned: hiro)
Details
(Keywords: crash)
Crash Data
Attachments
(2 files)
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-release+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-release+
|
Details | Review |
Crash report: https://crash-stats.mozilla.org/report/index/af14c242-bb69-4cdb-8b62-c11740210127
Reason: SIGSEGV /SEGV_MAPERR
Top 10 frames of crashing thread:
0 libxul.so nsSize ExpandHeightForDynamicToolbarImpl<nsSize> layout/base/nsLayoutUtils.cpp:9565
1 libxul.so nsLayoutUtils::ExpandHeightForDynamicToolbar layout/base/nsLayoutUtils.cpp:9582
2 libxul.so DescendIntoChild layout/generic/nsIFrame.cpp:3924
3 libxul.so nsIFrame::BuildDisplayListForChild layout/generic/nsIFrame.cpp:4143
4 libxul.so mozilla::ViewportFrame::BuildDisplayList layout/generic/ViewportFrame.cpp:63
5 libxul.so nsIFrame::BuildDisplayListForStackingContext layout/generic/nsIFrame.cpp:3455
6 libxul.so nsLayoutUtils::GetFramesForArea layout/base/nsLayoutUtils.cpp:2693
7 libxul.so nsLayoutUtils::GetFrameForPoint layout/base/nsLayoutUtils.cpp:2649
8 libxul.so mozilla::FindFrameTargetedByInputEvent layout/base/PositionedEventTargeting.cpp:490
9 libxul.so mozilla::PresShell::EventHandler::GetFrameToHandleNonTouchEvent layout/base/PresShell.cpp:7167
Reporter | ||
Updated•4 years ago
|
Comment 1•4 years ago
|
||
Looks like a null-deref in this chunk:
mozilla::ScreenCoord(aPresContext->GetDynamicToolbarMaxHeight()) /
mozilla::ViewAs<mozilla::ScreenPixel>(
MVM->DisplaySize(),
mozilla::PixelCastJustification::LayoutDeviceIsScreenForBounds)
.height;
Presumably either aPresContext
or MVM
are null here.
hiro: would you mind taking a look? It looks like you wrote the original version of this function (in https://hg.mozilla.org/mozilla-central/rev/b2e402b376f9 ) so I'm guessing you'd be a good person to investigate.
(Would it make sense that any calling patterns end up with nullptr here? Maybe first step is to add a diagnostic patch to help determine which variable is null, if it's not already clear which one it must be?)
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
aPresContext
should be valid there, MVM
could be null but..
Anyway I could manage to reproduce this crash with setting these prefs;
apz.mvm.force-enabled: false
dom.meta-viewport.enabled: false
apz.allow_zooming: false
That said, I am quite unsure it's replicating the crash situation or not, because the CPU architecture of the crash reports are either x86 or amd64 which look odd, for example the device of this crash (bp-286bf9da-1dbf-43a3-9b87-90e1d0210201) is Samsung SM-G970N which seems be arm architecture.
Assignee | ||
Comment 3•4 years ago
|
||
Otherwise on Android, we always use the toolbar height set by applications even
if the pref was set (Note that GeckoView Test Runner doesn't have the dynamic
toolbar thus the height is always 0).
Assignee | ||
Comment 4•4 years ago
|
||
Depends on D103611
Comment 6•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4b90835c5e07
https://hg.mozilla.org/mozilla-central/rev/529e8d674765
Comment 7•4 years ago
|
||
This looks like a pretty frequent Fenix crash - can we nominate this for Beta approval in time for 86?
Assignee | ||
Comment 8•4 years ago
|
||
Comment on attachment 9200352 [details]
Bug 1689371 - Use nsLayoutUtils::GetContentViewerSize to query the device display size if there is no MobileViewportManager. r?botond
Beta/Release Uplift Approval Request
- User impact if declined: Crash
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce: The change includes an automated test but it's artificial, I don't think it's replicating the crash happening in the wild.
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The change is a null check plus a decent fallback so it made the code mode robust.
- String changes made/needed:
Assignee | ||
Updated•4 years ago
|
Comment 9•4 years ago
|
||
Comment on attachment 9200352 [details]
Bug 1689371 - Use nsLayoutUtils::GetContentViewerSize to query the device display size if there is no MobileViewportManager. r?botond
Approved for our RC build.
Updated•4 years ago
|
Comment 10•4 years ago
|
||
bugherder uplift |
Description
•