Closed Bug 1689371 Opened 4 years ago Closed 4 years ago

Crash in [@ ExpandHeightForDynamicToolbarImpl<T>]

Categories

(Core :: Layout, defect)

x86
Android
defect

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox85 --- wontfix
firefox86 --- fixed
firefox87 --- fixed

People

(Reporter: fluffyemily, Assigned: hiro)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

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
Component: General → Layout
Product: GeckoView → Core

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?)

Flags: needinfo?(hikezoe.birchill)
Severity: -- → S2

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: nobody → hikezoe.birchill
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe.birchill)

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).

Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4b90835c5e07 Don't overwrite the dynamic toolbar height if we have set the pref value. r=botond https://hg.mozilla.org/integration/autoland/rev/529e8d674765 Use nsLayoutUtils::GetContentViewerSize to query the device display size if there is no MobileViewportManager. r=botond
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

This looks like a pretty frequent Fenix crash - can we nominate this for Beta approval in time for 86?

Flags: needinfo?(hikezoe.birchill)
Flags: in-testsuite+

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:
Flags: needinfo?(hikezoe.birchill)
Attachment #9200352 - Flags: approval-mozilla-beta?
Attachment #9200351 - Flags: approval-mozilla-beta?

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.

Attachment #9200352 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Attachment #9200351 - Flags: approval-mozilla-beta? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: