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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Oliver Nelson [:oliverthor]]

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

Comment 1

[Oliver Nelson [:oliverthor]]

Attachment: logcat_20150505_0907.txt

Comment 2

[Peter Bylenga [:PBylenga]]

This was seen once, adding steps-wanted to see if we can find better STR's.

Comment 3

[Oliver Nelson [:oliverthor]]

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.

Comment 4

[Oliver Nelson [:oliverthor]]

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

* https://crash-stats.mozilla.com/report/index/7d56710f-a924-4994-b065-887282150505

Comment 5

[Gregor Wagner [:gwagner]]

Adding kats since there is some apz code in the stack.

Comment 6

[Gregor Wagner [:gwagner]]

Does this only show up on 3.0 builds?

Comment 7

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

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

Comment 8

[Botond Ballo [:botond]]

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.

Comment 9

[Lance（leave from Mozilla）]

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

Comment 10

[Lance（leave from Mozilla）]

Attachment: logcat_1051.txt

Comment 11

[Lance（leave from Mozilla）]

Attachment: 1051.MP4

Comment 12

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

It's probably what botond said in comment 8. Should be an easy fix, but I want to try to reproduce and confirm.

Comment 13

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

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.

Comment 14

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

Attachment: 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.

Comment 15

[Pi Wei Cheng [:piwei] (inactive)]

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.

Comment 16

[Botond Ballo [:botond]]

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?

Comment 17

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

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?

Comment 18

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

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

Comment 19

[Botond Ballo [: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.

Comment 20

[Botond Ballo [:botond]]

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?

Comment 21

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

Attachment: 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+

Comment 22

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

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

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2364aef0dd

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8b2364aef0dd

Comment 26

[mkdante381]

[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^