1181135 - [APZ] Crashing on pages while clicking-selecting-dragging text [@mozilla::DisplayItemClip::operator=(mozilla::DisplayItemClip const&), @mozilla::DisplayItemClip::IntersectWith(mozilla::DisplayItemClip const&)]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mayank Bansal]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150706030206

Steps to reproduce:

 Using my normal profile.   e10s+APZ+OMTC on win7.

On any page, click a few times on text, select it, drag it a bit.
Reproduces intermittently.


Actual results:

getting crashes.

https://crash-stats.mozilla.com/report/index/c87115f3-ea1e-4286-a342-d16742150707
https://crash-stats.mozilla.com/report/index/8ed219c6-2e42-482b-bfb4-fe7c22150707
https://crash-stats.mozilla.com/report/index/ad7c841f-38bd-4158-9fb6-94ea02150707

and so on.
 Had about 7 crashes so far. Started happening with this :  https://hg.mozilla.org/mozilla-central/rev/cef11c3e86c3  . Didnt happen with the previous nightly.

Comment 1

[Mayank Bansal]

Adapter Description: Intel(R) HD Graphics 5500
Adapter Drivers: igdumdim64 igd10iumd64 igd10iumd64 igdumdim32 igd10iumd32 igd10iumd32
Adapter RAM: Unknown
Asynchronous Pan/Zoom: wheel input enabled
ClearType Parameters: Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 50
Device ID: 0x1616
Direct2D Enabled: true
DirectWrite Enabled: true (6.2.9200.17292)
Driver Date: 5-22-2015
Driver Version: 10.18.14.4222
GPU #2 Active: false
GPU Accelerated Windows: 2/2 Direct3D 11 (OMTC)
Subsys ID: 2336103c
Supports Hardware H264 Decoding: true
Vendor ID: 0x8086
WebGL Renderer: Google Inc. -- ANGLE (Intel(R) HD Graphics 5500 Direct3D11 vs_5_0 ps_5_0)
windowLayerManagerRemote: true
AzureCanvasBackend: direct2d 1.1
AzureContentBackend: direct2d 1.1
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0

Comment 2

[Mayank Bansal]

best STR I have is to go to https://groups.google.com/forum/#!topic/mozilla.dev.platform/-cDq6DqBZ2k , and start quickly clicking on some text such that it is selected-deselected.

Comment 3

[Mayank Bansal]

appears to be APZ related. I disabled it, and havent reproduced teh crash yet.

Comment 4

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

The crash stack does look like it's running through APZ-related code, but on the layout side of things. Moving component and CC'ing markus.

Comment 5

[Mayank Bansal]

i had APZ disabled for the last 2 weeks, and no crashes. On a lark, i enabled APZ, and within 5 minutes, i got a crash.
https://crash-stats.mozilla.com/report/index/1a07e512-13ee-44ae-bebb-331832150722

Is there any plan to fix this?

Comment 6

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

Yeah, I can take a look. I tried reproducing it on Linux - haven't seen this crash yet but following the STR I'm hitting a different crash with an assertion in FrameLayerBuilder. I'll file a separate bug for that.

Comment 7

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

(In reply to mayankleoboy1 from comment #5)
> Is there any plan to fix this?

But to answer this question, yes, this is blocking bug 1178298 so we will need to fix this before we can enable APZ on aurora and beyond.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a regression - though if it isn't going to move to aurora this cycle, we can move the tracking to 44.   Note to self and anyone else... the pref to flip is layers.async-pan-zoom.enabled.

Comment 9

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1181135 - Copy DisplayItemClips in order to avoid dangling pointers. r?tn

Bug 1181135 - Copy DisplayItemClips in order to avoid dangling pointers. r?tn

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8655656 [details]
MozReview Request: Bug 1181135 - Copy DisplayItemClips in order to avoid dangling pointers. r?tn

Bug 1181135 - Copy DisplayItemClips in order to avoid dangling pointers. r?tn

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8655656 [details]
MozReview Request: Bug 1181135 - Copy DisplayItemClips in order to avoid dangling pointers. r?tn

https://reviewboard.mozilla.org/r/17969/#review16199

Comment 13

[Daniel Veditz [:dveditz]]

I set the status of 42 and 43 to "disabled" to reflect the state it will be when those versions are released -- that is to reflect whether we need to backport fixes to those branches once we have them. Looking at the dependency list on bug 1178298 I'm assuming we won't let it ride the train in 43 with only another couple of weeks before uplift, but if we do we should flip status 43 back to "affected".

Comment 14

[Mats Palmgren (inactive)]

In my x86-64 Linux build, a DisplayItemClip is 32 bytes in size and it has
a nsTArray member which carries additional heap memory in the common case.
It seems a bit wasteful that mAncestorClip and mAncestorClipForCaret are
stored permanently on the scroll frame when they are only used during
painting (AFAICT).  It looks like these members are not actually used
by the scroll frame code itself, it just serves them to other code through
ComputeScrollClip and ComputeFrameMetrics.

Would it be possible store these members somewhere else such that
the memory is reclaimed after painting is done?

Comment 15

[Markus Stange [:mstange]]

Yes. I am working on a patch that does exactly that; the patch in this bug is more of a stop-gap solution.
(My new patch will store the clip in a ScrollClip struct that's allocated into the display list builder arena, and each display item will have a member that points to its ScrollClip.)

Comment 16

[Mats Palmgren (inactive)]

Sounds great, thanks Markus.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dace02633b3

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1dace02633b3

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8655656 [details]
MozReview Request: Bug 1181135 - Copy DisplayItemClips in order to avoid dangling pointers. r?tn

Approval Request Comment
[Feature/regressing bug #]: bug 1148582 and bug 1178745
[User impact if declined]: crashes when dragging text, if the user manually enabled layers.async-pan-zoom.enabled
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low, touches a feature that's disabled by default
[String/UUID change made/needed]: none

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Markus, why should we take this patch if the feature is disabled? Thanks

Comment 21

[Markus Stange [:mstange]]

It's a very attractive pref because of the large perf win it gives, so I wouldn't be surprised if many people start testing it out. Also, even if I don't see how, it's conceivable that attackers might find a way to exploit the bug even when the pref is off.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8655656 [details]
MozReview Request: Bug 1181135 - Copy DisplayItemClips in order to avoid dangling pointers. r?tn

Ok, let's take it then!
Thanks for the input.