Closed
Bug 1161592
Opened 9 years ago
Closed 9 years ago
crash in nsLayoutUtils::CalculateAndSetDisplayPortMargins(nsIScrollableFrame*, nsLayoutUtils::RepaintMode)
Categories
(Core :: Layout, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
b2g-v2.2 | --- | unaffected |
b2g-master | --- | fixed |
People
(Reporter: onelson, Assigned: kats)
References
Details
(Keywords: crash, regression, Whiteboard: [3.0-Daily-Testing])
Crash Data
Attachments
(4 files, 2 obsolete files)
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•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Whiteboard: [3.0-Daily-Testing]
Comment 2•9 years ago
|
||
This was seen once, adding steps-wanted to see if we can find better STR's.
Reporter | ||
Comment 3•9 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•9 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
Updated•9 years ago
|
Component: Gaia::Browser → Layout
Product: Firefox OS → Core
Comment 5•9 years ago
|
||
Adding kats since there is some apz code in the stack.
Flags: needinfo?(bugmail.mozilla)
Comment 6•9 years ago
|
||
Does this only show up on 3.0 builds?
blocking-b2g: --- → 3.0+
Keywords: qawanted
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
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)
Comment 15•9 years ago
|
||
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.
Updated•9 years ago
|
Comment 16•9 years ago
|
||
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?
Assignee | ||
Comment 17•9 years ago
|
||
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?
Assignee | ||
Comment 18•9 years ago
|
||
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 19•9 years ago
|
||
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 20•9 years ago
|
||
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-
Assignee | ||
Comment 21•9 years ago
|
||
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+
Assignee | ||
Comment 22•9 years ago
|
||
(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 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8b2364aef0dd
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 26•9 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:
--- → ?
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
status-b2g-v2.5:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•