crash in nsLayoutUtils::CalculateAndSetDisplayPortMargins(nsIScrollableFrame*, nsLayoutUtils::RepaintMode)

RESOLVED FIXED in Firefox 40, Firefox OS master

Status

()

--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: onelson, Assigned: kats)

Tracking

({crash, regression})

unspecified
mozilla40
ARM
Gonk (Firefox OS)
crash, regression
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.5+, firefox40 fixed, b2g-v2.2 unaffected, b2g-master fixed)

Details

(Whiteboard: [3.0-Daily-Testing], crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
This bug was filed from the Socorro interface and is 
report bp-57160961-b2e7-43d3-aafd-7e07b2150505.
=============================================================

Description:
Experienced a crash in the Browser app after searching for 'nuclear throne' and selecting the 3rd link down from Google for the reddit page ('www.reddit.com/r/NuclearThrone/'). This was observed only once, and the crash occurred before the new webpage could present itself on screen.

Repro Steps:
1) Update phone to 20150505010204
2) Open Browser
3) Begin search for 'nuclear'
4) Tap 'No Internet Connection' and connect to Wi-Fi
5) Drag down notification tray to kill 'Network Connections' screens
6) Finish search for 'nuclear throne'
7) Select 3rd link down for 'r/NuclearThrone' (reddit page)

Actual:
Browser closes and feeds user a crash report on screen

Expected:
Page opens and user may continue browsing
(Reporter)

Comment 1

4 years ago
Created attachment 8601551 [details]
logcat_20150505_0907.txt
(Reporter)

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Whiteboard: [3.0-Daily-Testing]
This was seen once, adding steps-wanted to see if we can find better STR's.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: steps-wanted
(Reporter)

Comment 3

4 years ago
So I just reproed this crash after downloading 'Cut the Rope' from marketplace and opening it.

* https://crash-stats.mozilla.com/report/index/7ad87bad-febe-4b61-b496-00ef12150505

Appears identical, pretty surprising. This app opens in landscape, which kind of plays along with the crash signature, but the original find of this in browser was all done in portrait.
(Reporter)

Comment 4

4 years ago
Crashed again opening Settings app after phone display was off for awhile.

* https://crash-stats.mozilla.com/report/index/7d56710f-a924-4994-b065-887282150505
Component: Gaia::Browser → Layout
Product: Firefox OS → Core
Adding kats since there is some apz code in the stack.
Flags: needinfo?(bugmail.mozilla)
Does this only show up on 3.0 builds?
blocking-b2g: --- → 3.0+
Keywords: qawanted
I was unable to reproduce this issue on the latest Nightly build.  Leaving steps wanted tag so others can attempt.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150505010204
Gaia: 70077825aab2c7a79611befb40a5fe7e610d5443
Gecko: 102d0e9aa9e1
Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
Flags: needinfo?(ktucker)
This might be a regression from bug 1156401. Prior to that patch, we would always return early from PrepareForSetTargetAPZCNotification if scrollAncestor was null, but now we can get into the CalculateAndSetDisplayPortMargins call if we don't already have a display port set, and that will crash. We assumed this would never happen (I mention this in bug 1156401 comment 12), but we may have been wrong.
This bug still exist in Message, contacts and Phone... apps in Flame 3.0, can't reproduced in Flame 2.2. After we launch the app, when the page display white-screen, use two fingers to long tap left and right edge at the same time, then crash will pop up.

Steps:
1.Launch Message.
2.Press Home key to back Home, then launch Contacts.
3.When the page display white-screen, use two fingers to long tap left and right edge at the same time.

Actual result:
3.The Contacts crash will pop up.

Crash reports title:
B2G 40.0a1 Crash Report [@ nsLayoutUtils::CalculateAndSetDisplayPortMargins(nsIScrollableFrame*, nsLayoutUtils::RepaintMode) ]

Crash reports link:
https://crash-stats.mozilla.com/report/index/95717e05-93ea-4df6-8463-1952b2150506

Found time:10:51
See attachment:1051.mp4 and logcat_1051.txt

Device: Flame 2.2 version(Pass):
Build ID               20150505002501
Gaia Revision          772a9491909abd02dc67278dd453746e2dd358a8
Gaia Date              2015-05-05 02:02:24
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2df83538ae20
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150505.041743
Firmware Date          Tue May  5 04:18:00 EDT 2015
Bootloader             L1TC000118D0

Device: Flame 3.0 version(Fail):
Build ID               20150505010204
Gaia Revision          70077825aab2c7a79611befb40a5fe7e610d5443
Gaia Date              2015-05-04 18:09:33
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/102d0e9aa9e1
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150505.043622
Firmware Date          Tue May  5 04:36:34 EDT 2015
Bootloader             L1TC000118D0
Created attachment 8601953 [details]
logcat_1051.txt
It's probably what botond said in comment 8. Should be an easy fix, but I want to try to reproduce and confirm.
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugmail.mozilla)
I can reproduce this fairly reliably by starting the settings app and then tapping on the screen immediately while it's loading. I use both thumbs to increase the tap rate.
Blocks: 1156401
Created attachment 8602222 [details] [diff] [review]
Patch

This is basically the APZCCallbackHelper equivalent of the fix in bug 1161040. We don't want to use the root document element in the child process, only the parent process. This aligns the code with the comment in the function.
Attachment #8602222 - Flags: review?(botond)
See comment 9 or comment 13 for STR. I can also repro using those steps. Also setting 2.2 as unaffected based on comment 9.
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.2: --- → unaffected
Keywords: qawanted, steps-wanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Keywords: regression
Comment on attachment 8602222 [details] [diff] [review]
Patch

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

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +508,5 @@
>    // displayport on during initialization.
>    if (nsView* view = nsView::GetViewFor(aWidget)) {
>      if (nsIPresShell* shell = view->GetPresShell()) {
>        MOZ_ASSERT(shell->GetDocument());
> +      if (!shell->GetRootScrollFrame()) {

If there is a root scroll frame, why is scrollAncestor in PrepareForSetTargetAPZCNotification null to begin with?
Hm, that's a good question. It looks like the GetFrameForPoint call is returning a null target, which then gives us a null scrollAncestor. The display list in the GetFrameForPoint call is empty, even though the root frame is non-null. Presumably none of the frames in the document at that point generate display items because they're all transparent or empty or something. I suppose in this case we still want to pick GetRootScrollFrame() as the scrollAncestor and proceed as normal?
Created attachment 8602904 [details] [diff] [review]
Alternate patch

This works too. I guess it boils down to the question "if the user touches into a root content document in the child process with no visible content, should the touch be handled by APZ or not?" and the answer I think is "it doesn't really matter". I think this second patch might be more correct but the first patch makes things more consistent with other parts of the code, so I don't know which is better (or maybe we should do both?)
Attachment #8602904 - Flags: review?(botond)
Comment on attachment 8602904 [details] [diff] [review]
Alternate patch

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

I agree that it doesn't matter, but for consistency with Layout code that special-cases the RCD-RSF, I think it makes sense to do so here too.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +475,3 @@
>  {
>    if (!aTarget) {
> +    return aRootFrame->PresContext()->PresShell()->GetRootScrollFrameAsScrollable();

I think this would read better as a check in PrepareForSetTargetAPZCNotification.
Attachment #8602904 - Flags: review?(botond) → review+
Comment on attachment 8602222 [details] [diff] [review]
Patch

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

I don't think adding this check makes sense given that the other patch handles the RCD-RSF case specially earlier on in PrepareForSetTargetAPZCNotification.

That said - how confident are we that we'll always have either a scroll frame or a display port on the root element? Would it make sense to make PrepareForSetTargetAPZCNotification more tolerant by returning early if scrollAncestor is null?
Attachment #8602222 - Flags: review?(botond) → review-
Created attachment 8603397 [details] [diff] [review]
Alternate patch v2

This moves the check out into PrepareForSetTargetAPZCNotification and removes the nullcheck from GetScrollableAncestorFrame since that should never happen any more. Carrying r+
Attachment #8602222 - Attachment is obsolete: true
Attachment #8602904 - Attachment is obsolete: true
Attachment #8603397 - Flags: review+
(In reply to Botond Ballo [:botond] [at c++ standards meeting May 4-9] from comment > > That said - how confident are we that we'll always have either a scroll
> frame or a display port on the root element? Would it make sense to make
> PrepareForSetTargetAPZCNotification more tolerant by returning early if
> scrollAncestor is null?

Given that ChromeProcessController always sets a displayport on the root element in the parent process, and child content processes should always have a root scrollframe, I think I'm fairly confident in this. The only bad case would be if we have chrome document at the root of a child process - and if there is an environment where that happens I'd rather we found out about it by crashing rather than letting it sneak by.
See Also: → bug 1163226
https://hg.mozilla.org/mozilla-central/rev/8b2364aef0dd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Duplicate of this bug: 1163226

Comment 26

4 years ago
[Tracking Requested - why for this release]:
Nightly 41.0a1 crash report
https://crash-stats.mozilla.com/report/index/6e547976-438e-4ea9-9302-daf6a2150521

Exit from Nightly(Menu>Exit the program)and go to other window(Explorer or something)>Nightly crash with this crash bug^
status-firefox41: --- → ?
tracking-firefox41: --- → ?
status-b2g-master: affected → fixed
status-b2g-v2.5: --- → fixed
status-firefox41: ? → ---
tracking-firefox41: ? → ---
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.