Closed Bug 1430787 Opened 6 years ago Closed 5 years ago

AsyncCompositionManager asserts when a sticky element is inside a transform

Categories

(Core :: Panning and Zooming, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: botond)

References

Details

(Keywords: crash, intermittent-failure, Whiteboard: [gfx-noted])

Crash Data

Attachments

(2 files)

Filed by: apavel [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=156591538&repo=autoland

https://queue.taskcluster.net/v1/task/NUzMV13hTKixen6BWqN7Rw/runs/0/artifacts/public/logs/live_backing.log

task 2018-01-16T14:55:07.773Z] 14:55:07     INFO -  REFTEST TEST-START | http://10.0.2.2:8854/tests/layout/reftests/position-sticky/transformed-1.html == http://10.0.2.2:8854/tests/layout/reftests/position-sticky/transformed-1-ref.html
[task 2018-01-16T14:55:07.774Z] 14:55:07     INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/position-sticky/transformed-1.html | 0 / 1 (0%)
[task 2018-01-16T14:55:28.463Z] 14:55:28     INFO -  INFO | automation.py | Application ran for: 0:02:00.777174
[task 2018-01-16T14:55:28.464Z] 14:55:28     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmp9p59jUpidlog
[task 2018-01-16T14:55:29.661Z] 14:55:29     INFO -  REFTEST INFO | Copy/paste: /builds/worker/workspace/build/linux64-minidump_stackwalk /tmp/tmpbbibdB/30444c40-2511-8bc9-ecc7-0e269f29fe30.dmp /builds/worker/workspace/build/symbols
[task 2018-01-16T14:55:40.885Z] 14:55:40     INFO -  REFTEST INFO | Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/30444c40-2511-8bc9-ecc7-0e269f29fe30.dmp
[task 2018-01-16T14:55:40.886Z] 14:55:40     INFO -  REFTEST INFO | Saved app info as /builds/worker/workspace/build/blobber_upload_dir/30444c40-2511-8bc9-ecc7-0e269f29fe30.extra
[task 2018-01-16T14:55:40.890Z] 14:55:40     INFO -  REFTEST PROCESS-CRASH | http://10.0.2.2:8854/tests/layout/reftests/position-sticky/transformed-1.html == http://10.0.2.2:8854/tests/layout/reftests/position-sticky/transformed-1-ref.html | application crashed [@ mozilla::layers::AsyncCompositionManager::AlignFixedAndStickyLayers]
[task 2018-01-16T14:55:40.891Z] 14:55:40     INFO -  Crash dump filename: /tmp/tmpbbibdB/30444c40-2511-8bc9-ecc7-0e269f29fe30.dmp
[task 2018-01-16T14:55:40.891Z] 14:55:40     INFO -  Operating system: Android
[task 2018-01-16T14:55:40.891Z] 14:55:40     INFO -                    0.0.0 Linux 2.6.29-gea477bb #1 Wed Sep 26 11:04:45 PDT 2012 armv7l
[task 2018-01-16T14:55:40.892Z] 14:55:40     INFO -  CPU: arm
[task 2018-01-16T14:55:40.892Z] 14:55:40     INFO -       ARMv7 ARM Cortex-A8 features: swp,half,thumb,fastmult,vfpv2,edsp,neon,vfpv3
[task 2018-01-16T14:55:40.892Z] 14:55:40     INFO -       1 CPU
[task 2018-01-16T14:55:40.892Z] 14:55:40     INFO -  GPU: UNKNOWN
[task 2018-01-16T14:55:40.892Z] 14:55:40     INFO -  Crash reason:  SIGSEGV
[task 2018-01-16T14:55:40.893Z] 14:55:40     INFO -  Crash address: 0x0
[task 2018-01-16T14:55:40.893Z] 14:55:40     INFO -  Process uptime: not available
[task 2018-01-16T14:55:40.893Z] 14:55:40     INFO -  Thread 49 (crashed)
[task 2018-01-16T14:55:40.894Z] 14:55:40     INFO -   0  libxul.so!mozilla::layers::AsyncCompositionManager::AlignFixedAndStickyLayers [AsyncCompositionManager.cpp:ba85e9043c8a : 482 + 0x0]
[task 2018-01-16T14:55:40.894Z] 14:55:40     INFO -       r0 = 0x00000000    r1 = 0x5e604b34    r2 = 0x6080d8ab    r3 = 0x000001e2
[task 2018-01-16T14:55:40.894Z] 14:55:40     INFO -       r4 = 0x000001e2    r5 = 0x56900224    r6 = 0x569001b0    r7 = 0x56900288
[task 2018-01-16T14:55:40.895Z] 14:55:40     INFO -       r8 = 0x623fac00    r9 = 0x569001fc   r10 = 0x56900210   r12 = 0x00000003
[task 2018-01-16T14:55:40.895Z] 14:55:40     INFO -       fp = 0x623fac00    sp = 0x56900118    lr = 0x5ce06d61    pc = 0x5d7534a4
[task 2018-01-16T14:55:40.895Z] 14:55:40     INFO -      Found by: given as instruction pointer in context
[task 2018-01-16T14:55:40.896Z] 14:55:40     INFO -   1  libxul.so!mozilla::layers::AsyncCompositionManager::AlignFixedAndStickyLayers [AsyncCompositionManager.cpp:ba85e9043c8a : 462 + 0x19]
[task 2018-01-16T14:55:40.896Z] 14:55:40     INFO -       r4 = 0x56900af4    r5 = 0x56900968    r6 = 0x623fb400    r7 = 0x56900400
[task 2018-01-16T14:55:40.896Z] 14:55:40     INFO -       r8 = 0x623fac00    r9 = 0x00000000   r10 = 0x00000004    fp = 0x530e6230
[task 2018-01-16T14:55:40.896Z] 14:55:40     INFO -       sp = 0x56900290    lr = 0x5d7530fd    pc = 0x5d7530fd
[task 2018-01-16T14:55:40.897Z] 14:55:40     INFO -      Found by: call frame info
[task 2018-01-16T14:55:40.898Z] 14:55:40     INFO -   2  libxul.so!mozilla::layers::AsyncCompositionManager::AlignFixedAndStickyLayers [AsyncCompositionManager.cpp:ba85e9043c8a : 462 + 0x19]
[task 2018-01-16T14:55:40.898Z] 14:55:40     INFO -       r4 = 0x56900af4    r5 = 0x56900968    r6 = 0x623fb000    r7 = 0x56900578
[task 2018-01-16T14:55:40.899Z] 14:55:40     INFO -       r8 = 0x623fac00    r9 = 0x00000000   r10 = 0x00000004    fp = 0x530e6230
[task 2018-01-16T14:55:40.899Z] 14:55:40     INFO -       sp = 0x56900408    lr = 0x5d7530fd    pc = 0x5d7530fd
[task 2018-01-16T14:55:40.899Z] 14:55:40     INFO -      Found by: call frame info
[task 2018-01-16T14:55:40.899Z] 14:55:40     INFO -   3  libxul.so!<name omitted> [AsyncCompositionManager.cpp:ba85e9043c8a : 999 + 0x19]
[task 2018-01-16T14:55:40.899Z] 14:55:40     INFO -       r4 = 0x56900adc    r5 = 0x56900850    r6 = 0x620c1e88    r7 = 0x56900a70
[task 2018-01-16T14:55:40.899Z] 14:55:40     INFO -       r8 = 0x56900710    r9 = 0x569007d0   r10 = 0x569008d8    fp = 0x569006d0
[task 2018-01-16T14:55:40.899Z] 14:55:40     INFO -       sp = 0x56900580    lr = 0x5d75e2fd    pc = 0x5d75e2fd
[task 2018-01-16T14:55:40.900Z] 14:55:40     INFO -      Found by: call frame info
[task 2018-01-16T14:55:40.900Z] 14:55:40     INFO -   4  libxul.so!mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer *, (lambda at /builds/worker/workspace/build/src/gfx/layers/composite/AsyncCompositionManager.cpp:837:7), (lambda at /builds/worker/workspace/build/src/gfx/layers/composite/AsyncCompositionManager.cpp:841:7)> [TreeTraversal.h:ba85e9043c8a : 145 + 0x7]
[task 2018-01-16T14:55:40.900Z] 14:55:40     INFO -       r4 = 0x56900adc    r5 = 0x00000000    r6 = 0x56900af0    r7 = 0x56900aa0
[task 2018-01-16T14:55:40.901Z] 14:55:40     INFO -       r8 = 0x623fac00    r9 = 0x530e6274   r10 = 0x00000000    fp = 0x530e6230
[task 2018-01-16T14:55:40.901Z] 14:55:40     INFO -       sp = 0x56900a78    lr = 0x5d753923    pc = 0x5d753923
[task 2018-01-16T14:55:40.901Z] 14:55:40     INFO -      Found by: call frame info
[task 2018-01-16T14:55:40.901Z] 14:55:40     INFO -   5  libxul.so!mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer *, (lambda at /builds/worker/workspace/build/src/gfx/layers/composite/AsyncCompositionManager.cpp:837:7), (lambda at /builds/worker/workspace/build/src/gfx/layers/composite/AsyncCompositionManager.cpp:841:7)> [TreeTraversal.h:ba85e9043c8a : 142 + 0x9]
[task 2018-01-16T14:55:40.902Z] 14:55:40     INFO -       r4 = 0x56900adc    r5 = 0x623fac00    r6 = 0x56900af0    r7 = 0x56900ad0
[task 2018-01-16T14:55:40.902Z] 14:55:40     INFO -       r8 = 0x5525b800    r9 = 0x530e6274   r10 = 0x00000000    fp = 0x530e6230
[task 2018-01-16T14:55:40.902Z] 14:55:40     INFO -       sp = 0x56900aa8    lr = 0x5d75390f    pc = 0x5d75390f
[task 2018-01-16T14:55:40.902Z] 14:55:40     INFO -      Found by: call frame info
[task 2018-01-16T14:55:40.903Z] 14:55:40   

[task 2018-01-16T14:55:43.210Z] 14:55:43     INFO -  01-16 06:55:00.884 I/Gecko   (  785): {"action":"log","time":1516114500868,"thread":null,"pid":null,"source":"reftest","level":"DEBUG","message":"[CONTENT] Using browser remote=false\n"}
[task 2018-01-16T14:55:43.210Z] 14:55:43     INFO -  01-16 06:55:01.584 I/Gecko   (  785): ++DOMWINDOW == 11 (0x623f1c00) [pid = 785] [serial = 11] [outer = 0x5628b4c0]
[task 2018-01-16T14:55:43.210Z] 14:55:43     INFO -  01-16 06:55:02.134 I/Gecko   (  785): [785, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B0051: file /builds/worker/workspace/build/src/dom/storage/LocalStorageManager.cpp, line 116
[task 2018-01-16T14:55:43.210Z] 14:55:43     INFO -  01-16 06:55:04.334 D/GeckoNetworkManager(  785): Incoming event disableNotifications for state OnWithListeners -> OnNoListeners
[task 2018-01-16T14:55:43.211Z] 14:55:43     INFO -  01-16 06:55:05.084 F/MOZ_Assert(  785): Assertion failure: ancestorTransform.IsIdentity(), at /builds/worker/workspace/build/src/gfx/layers/composite/AsyncCompositionManager.cpp:482
[task 2018-01-16T14:55:43.211Z] 14:55:43     INFO -  01-16 06:55:05.135 W/google-breakpad(  930): ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2018-01-16T14:55:43.211Z] 14:55:43     INFO -  01-16 06:55:05.135 W/google-breakpad(  785): ExceptionHandler::GenerateDump cloned child
[task 2018-01-16T14:55:43.211Z] 14:55:43     INFO -  01-16 06:55:05.135 W/google-breakpad(  785): 930m.IsIdentity(), at /builds/wo
[task 2018-01-16T14:55:43.212Z] 14:55:43     INFO -  01-16 06:55:05.135 W/google-breakpad(  785):
[task 2018-01-16T14:55:43.212Z] 14:55:43     INFO -  01-16 06:55:05.154 W/google-breakpad(  785): ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2018-01-16T14:55:43.585Z] 14:55:43     INFO -    os: sdk-eng 4.3.1 JLS36I eng.gbrown.20150308.182649 test-keys
[task 2018-01-16T14:55:43.585Z] 14:55:43     INFO -    id: emulator-5554
[task 2018-01-16T14:55:43.585Z] 14:55:43     INFO -  Test root: /storage/sdcard/tests
[task 2018-01-16T14:55:43.585Z] 14:55:43    ERROR - Return code: 1
[task 2018-01-16T14:55:43.586Z] 14:55:43    ERROR - No tests run or test summary not found
This is from bug 1426386.
Assignee: nobody → bugmail
Blocks: 1426386
...which was backed out. I'll make sure to fix this before re-landing.
AFAICT this isn't a regression from my patches, it's just that one of the reftests I added exposes a pre-existing bug in AsyncCompositionManager. The bug manifests as a failing assertion. I'll need to dig into it more to figure out what's going on.
Component: Layout → Graphics: Layers
Priority: P5 → P3
Whiteboard: [gfx-noted]
Here's the test case in question: https://staktrace.github.io/moz-pages/bug/1430787.html
Here's the layer tree on Android: https://gist.github.com/staktrace/d01522b6463fa7e5192fe88165195f95#file-01-android-layer-tree

In this case, the assertion fails when layer=0x8c80a000 (the sticky layer) and aTransformedSubtreeRoot=0x8c809800, because in between these two is a ContainerLayerComposite with transform=[ 0.408163 0; 0 0.408163; 0 97.9592; ].

On desktop the layer tree looks like this: https://gist.github.com/staktrace/d01522b6463fa7e5192fe88165195f95#file-02-desktop-layer-tree

In this case the sticky layer is 0x7f594b767800 and the immediate parent of that is the transformed subtree root 0x7f594b758000, so the AccumulateLayerTransforms function produces an identity transform.
So I don't know if I agree with the comment at [1], but I'm not sure if I'm understanding it correctly. Specifically, the comment seems to be implying some sort of equivalence between "containing blocks" (as created by transforms) and the "subtree root layer" which really is just a layer with an APZC). I don't follow the logic here - Botond, does this comment still make sense to you?

[1] https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/gfx/layers/composite/AsyncCompositionManager.cpp#472-474
Flags: needinfo?(botond)
The comment is referring to the following from the CSS Transforms spec:

  "For elements whose layout is governed by the CSS box model, 
   any value other than none for the transform also causes 
   the element to become a containing block, and the object 
   acts as a containing block for fixed positioned 
   descendants." [1]

My understanding is that "containing block for fixed positioned descendants" means that the fixed element is fixed with respect to the transformed element (as opposed to with respect to the enclosing document's viewport).

I would expect that to be reflected in the fixed layer's GetFixedPositionScrollContainerId(), which in turn determines what the subtree root layer is.

[1] https://drafts.csswg.org/css-transforms-1/#transform-rendering
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] from comment #6)
> I would expect that to be reflected in the fixed layer's
> GetFixedPositionScrollContainerId(), which in turn determines what the
> subtree root layer is.

Except that the subtree root layer is a layer with scroll metadata, and I don't, in retrospect, see why the transform can't be in between the layer with the scroll metadata and its descendant fixed layer, as is happening in the first layer tree linked in comment 4.

It looks like the original basis for this assumption was bug 1214151 comment 11:

> AFAIK the root scroll frame container is the only scenario 
> where there can be a transform between a fixed layer and 
> the first unscrolled ancestor layer

Markus, do you recall what your rationale was for this assumption? Why does it break down in the first layer tree linked in comment 4?
Flags: needinfo?(mstange)
Comment #6 makes sense, and I just tried a couple of test pages that seem to confirm that behaviour (that fixed position items are fixed with respect to an enclosing transformed element as opposed to the document's viewport).

However, I think this is not the case for *sticky* elements, and the assertion is in a codepath that is run for both fixed and sticky elements. So that probably explains why the assertion is failing.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> However, I think this is not the case for *sticky* elements, and the
> assertion is in a codepath that is run for both fixed and sticky elements.
> So that probably explains why the assertion is failing.

Hmm, you're right, this may be the case as well. I would find it a bit strange, though, in that the reason given for having transformed elements be a containing block for fixed-position elements is implementation complexity [1], and from an implementation point of view, sticky behaviour is a superset of fixed behaviour (in that a sticky element behaves as fixed under some circumstances), so it seems the same implementation concerns should apply.

Perhaps Markus can shed some light on this question as well.

[1] https://github.com/w3c/csswg-drafts/issues/913
Comment on attachment 8944079 [details]
Bug 1430787 - Restrict a debug assertion to fixed-pos layers.

https://reviewboard.mozilla.org/r/214398/#review220086

Unfortunately, if there is indeed a non-identity transform, the correct behaviour would not be to ignore it, as this patch would do.

The correct behaviour would involve:

  - Multiplying the ancestor transform into |aPreviousTransformForRoot|
    and |aCurrentTransformForRoot| prior to computing 
    |transformedAnchor| (essentially, backing out [1]).
    
  - Also muliplying the ancestor transform into the transforms used
    in this branch [2]. (My motivation for doing [1] in the first
    place was to avoid doing this.)
    
It may be worth waiting for a reply from Markus before proceeding, so we can better understand whether / why this is necessary to begin with.

Alternatively, if you'd just like to be unblocked by this, we can downgrade the assertion to a warning in the sticky case (i.e. what you're doing now, but still warn) until we figure out the proper thing to do.
    
[1] https://reviewboard.mozilla.org/r/81220/diff/2#index_header
[2] https://searchfox.org/mozilla-central/rev/9f2721b19b10a9a441fa5ca7116720d2fcebcc4a/gfx/layers/composite/AsyncCompositionManager.cpp#555
Attachment #8944079 - Flags: review?(botond) → review-
(In reply to Botond Ballo [:botond] from comment #11)
> 
> Unfortunately, if there is indeed a non-identity transform, the correct
> behaviour would not be to ignore it, as this patch would do.

Do you know in what circumstances the current patch would display incorrect behaviour? I'm just wondering because the testcase in question seems to render/behave correctly with the patch. Maybe the transforms involved are too simple to demonstrate the incorrect behaviour though.

(In reply to Botond Ballo [:botond] from comment #11)
> Alternatively, if you'd just like to be unblocked by this, we can downgrade
> the assertion to a warning in the sticky case (i.e. what you're doing now,
> but still warn) until we figure out the proper thing to do.

For now I can just mark the reftests in bug 1426386 as skip-if(Android) to unblock the landing of that bug. We can leave this bug open to figure out the right solution.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> (In reply to Botond Ballo [:botond] from comment #11)
> > 
> > Unfortunately, if there is indeed a non-identity transform, the correct
> > behaviour would not be to ignore it, as this patch would do.
> 
> Do you know in what circumstances the current patch would display incorrect
> behaviour? I'm just wondering because the testcase in question seems to
> render/behave correctly with the patch. Maybe the transforms involved are
> too simple to demonstrate the incorrect behaviour though.

I would expect incorrect behaviour in cases where there is a non-identity ancestor transform, and we composite a frame with an async scroll offset on the "subtree root" layer. In such a case, the "fixup" applied to the fixed layer to keep it fixed may not be correct.

Granted, we would be including the ancestor transform in both the "previous" and "current" transforms, and the computation involves applying the "previous" and the *inverse* of the "current", so in many cases (but I think not all) the tranforms would just cancel out.

If you'd like, I can try to come up with a testcase that triggers the incorrect behaviour.
(In reply to Botond Ballo [:botond] from comment #14)
> If you'd like, I can try to come up with a testcase that triggers the
> incorrect behaviour.

I wouldn't spend too much time on it. This isn't blocking me at the moment and it might not be worth the effort to fix right now.
I'm not actively working on this.
Assignee: bugmail → nobody
Component: Graphics: Layers → Panning and Zooming
Blocks: 1464227
Bug 1492250 added another test that hits this (sticky-pos-scrollable-7.html) which I've disabled for Android and referenced this bug.
(In reply to Botond Ballo [:botond] from comment #7)
> (In reply to Botond Ballo [:botond] from comment #6)
> > I would expect that to be reflected in the fixed layer's
> > GetFixedPositionScrollContainerId(), which in turn determines what the
> > subtree root layer is.
> 
> Except that the subtree root layer is a layer with scroll metadata, and I
> don't, in retrospect, see why the transform can't be in between the layer
> with the scroll metadata and its descendant fixed layer, as is happening in
> the first layer tree linked in comment 4.
> 
> It looks like the original basis for this assumption was bug 1214151 comment
> 11:
> 
> > AFAIK the root scroll frame container is the only scenario 
> > where there can be a transform between a fixed layer and 
> > the first unscrolled ancestor layer
> 
> Markus, do you recall what your rationale was for this assumption? Why does
> it break down in the first layer tree linked in comment 4?

I think I understand now: when a fixed element is inside a transformed element, it scrolls with the transformed element, so there is no need to layerize it separately, so there will be no layer with a GetFixedPositionScrollContainerId(). So, the invariant that layers between a fixed layer and its subtree root layer do not have a transform, should be preserved.

(In reply to Botond Ballo [:botond] from comment #9)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> > However, I think this is not the case for *sticky* elements, and the
> > assertion is in a codepath that is run for both fixed and sticky elements.
> > So that probably explains why the assertion is failing.
> 
> Hmm, you're right, this may be the case as well. I would find it a bit
> strange, though, in that the reason given for having transformed elements be
> a containing block for fixed-position elements is implementation complexity
> [1], and from an implementation point of view, sticky behaviour is a
> superset of fixed behaviour (in that a sticky element behaves as fixed under
> some circumstances), so it seems the same implementation concerns should
> apply.
> 
> Perhaps Markus can shed some light on this question as well.
> 
> [1] https://github.com/w3c/csswg-drafts/issues/913

Clearly, as evidenced from this and similar bugs, the behaviour of our Layout code is that sticky elements do _not_ attach to an enclosing transformed element the way fixed elements do. As a result, the invariant mentioned above does not hold for sticky elements.

I have no idea whether this is correct according to the spec, but for the purposes of this bug, I don't care. AsyncCompositionManager should just gracefully handle what Layout gives it. In this case, that means having the code in question handle non-identity transforms (while perhaps retaining the assertion for fixed layers only).
Assignee: nobody → botond
Flags: needinfo?(mstange)
Summary: Perma-failing layout/reftests/position-sticky/transformed-1.html == layout/reftests/position-sticky/transformed-1-ref.html | application crashed [@ mozilla::layers::AsyncCompositionManager::AlignFixedAndStickyLayers] → AsyncCompositionManager asserts when a sticky element is inside a transform
No longer blocks: 1464227
Also enable tests that were previously disabled due to this.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac8da2a3a2fd
Handle position:sticky inside a transform in AsyncCompositionManager. r=kats
https://hg.mozilla.org/mozilla-central/rev/ac8da2a3a2fd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: