1466954 - useOpacity changes between paints without an invalidation.

Status: RESOLVED DUPLICATE of bug 1475769

(Core :: Web Painting, defect, P2)

Description

[Matt Woodrow (:mattwoodrow)]

This is a cause of the crashes in bug 1466010, but using a separate bug since I suspect it's not 100% of them.

I can reproduce this crash sometimes when making a youtube video fullscreen, but only in my downloaded Nightly, never in a local build.

I think I understand the cause, but hard to confirm since I can't repro with anything I build myself (tested opt and debug, plus rr on linux).

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1466954 - Create an nsDisplayOpacity item if we might have animations for it, so that we avoid changing the display list without an invalidation.

Review commit: https://reviewboard.mozilla.org/r/249434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249434/

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8983575 [details]
Bug 1466954 - Create an nsDisplayOpacity item if we might have animations for it, so that we avoid changing the display list without an invalidation.

https://reviewboard.mozilla.org/r/249434/#review255620

I don't think this fixes the crash since 'May' in MayHaveAnimationOfProperty means that the function returns true even if the animation for the given CSS property has been already finished.  More precisely, we don't clear the flags that are used for MayHaveAnimationOfProperty when the animation finished.  The flags persist until the corresponding EffectSet is destroyed.

So this patch probably creates a stacking context for an element which had an opacity animation but still has other animations (e.g. background-color).

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Another suspicious point is that there are still some cases that nsIFrame::mMayHaveOpacityAnimation isn't properly set.

Comment 4

[Tyson Smith [:tsmith]]

Attachment: testcase.html

This testcase is pretty consistent but it seems that it must be opened via http:// not file://

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Great!

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Tyson Smith [:tsmith] from comment #4)
> Created attachment 8984329 [details]
> testcase.html
> 
> This testcase is pretty consistent but it seems that it must be opened via
> http:// not file://

I can reproduce this one!

Unfortunately it's not related to opacity :(

I filed bug 1467688 for this problem. Thanks Tyson!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Tyson, do you have any other test cases that cause the same crash?  Especially including opacity property.  It would be really helpful for us to debug this crash.

Comment 8

[Tyson Smith [:tsmith]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> Tyson, do you have any other test cases that cause the same crash? 
> Especially including opacity property.  It would be really helpful for us to
> debug this crash.

It's hard to tell because the initial testcases are huge (512K or more) and it takes time to reduce them. Since the bug seems to be inconsistently reproducible that makes reduction take much longer.

So if there are differences in the stack to look for that would be very helpful. Or if the issue manifests as an assertion as well that is easy to find in our backlog.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I believe this has been fixed by bug 1475769.