Closed Bug 1469220 Opened 3 years ago Closed 3 years ago

Backout changesets which are related to animations landed in bug 1468182

Categories

(Core :: Web Painting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(1 file)

We have a bug report (bug 1468182) that there are suspicious changesets between 61.0b8 and 61.0b9.  Bug 1462497 is the most suspicious change there, particulaly the last two changesets which are related to animations.  So I did ask Marek (the reporter of the bug) to try a binary that those two changesets were backed out, and it seems to fixe the crash (see bug 1468182 comment 3). 

So, I'd suggest to back them out in nightly for a while, and if it reduces the crash rate, we should uplift them on beta.

Note that bp-f68087b1-9566-4a6d-8f76-7cb550180611 is the original crash report for bug 1468182, and crash happens at the assertion in HasMatchingItemInOldList.
Comment on attachment 8985869 [details]
Bug 1469220 - Backed out changeset 7fc66c715a0f and 39cb4d5f6602.

https://reviewboard.mozilla.org/r/251334/#review257650

This sucks, it was meant to make the crash happen less often, not more.

Do you have any ideas as to why the opacity value would change even more often?
Attachment #8985869 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Comment on attachment 8985869 [details]
> Bug 1469220 - Backed out changeset 7fc66c715a0f and 39cb4d5f6602.
> 
> https://reviewboard.mozilla.org/r/251334/#review257650
> 
> This sucks, it was meant to make the crash happen less often, not more.
> 
> Do you have any ideas as to why the opacity value would change even more
> often?

I am not sure, but my best guess I can tell for this crash situation is that |mMayHaveOpacityAnimation| (or |mMayHaveTransformAnimation| is also related) which is the flag that nsLayoutUtils::MayHaveAnimationOfProperty() checks seems not to be a suitable for.  The flag still persists *true* even after the opacity animation on the frame has finished.  That's said there are plenty tests that causes the situations in our test cases, so there must be other triggers, I guess one of the triggers is somewhat related to NAC elements having opacity animations inside video element, but any way I don't have any clear insight what's going on there unfortunately.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0600b9d08bdf
Backed out changeset 7fc66c715a0f and 39cb4d5f6602. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/0600b9d08bdf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee: nobody → hikezoe
Here is crash reports counts;

Rank    BuildId         Count        %
17 	20180619220118 	23 	2.09 %
12 	20180619102337 	42 	3.81 %
2 	20180619022742 	81 	7.34 %
15 	20180618220118 	37 	3.35 %
7 	20180618100844 	47 	4.26 %  <- The backed-out change landed in this build
11 	20180617220505 	42 	3.81 %

So, it looks to me the crash rate doesn't change.  The build '20180619022742' is pretty high but it includes lots of crashes that have the same install time.  So I guess people was trying to reproduce the crash with opening a URL in bug 1466010 (the URL is between comment 6 and 7).  I don't have the permission to see each URL in the crash reports, so it's just a guess.

[1] https://crash-stats.mozilla.com/signature/?build_id=20180619022742&signature=MergeState%3A%3AHasMatchingItemInOldList&date=%3E%3D2018-06-15T01%3A39%3A01.000Z&date=%3C2018-06-22T01%3A39%3A01.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=install_time&_sort=-date&page=1
Comment on attachment 8985869 [details]
Bug 1469220 - Backed out changeset 7fc66c715a0f and 39cb4d5f6602.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1468182
[User impact if declined]: Crash on beta
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No, unfortunately there is no reliable way to reproduce it
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: very very low
[Why is the change risky/not risky?]: These backed out changeset which introduced in bug 1468182 was an overkill, this patch does just restore the previous state
[String changes made/needed]: None
Attachment #8985869 - Flags: approval-mozilla-beta?
Comment on attachment 8985869 [details]
Bug 1469220 - Backed out changeset 7fc66c715a0f and 39cb4d5f6602.

Fx61 is on release now (we've already built the RC builds). I'll keep this on the radar as a ride-along if we need to respin.
Attachment #8985869 - Flags: approval-mozilla-beta? → approval-mozilla-release?
I'm not seeing any crashes matching bug 1466010 for Fx61 on release so far. Is there a different signature I should be looking at?
Flags: needinfo?(hikezoe)
The crash is caused by MOZ_DIAGNOSTIC_ASSERT, so it won't happen on release channels.  But even if the crash doesn't happen, it might result some rendering issues, I don't know what exactly happens as the result though, :mattwoodrow knows what happens, I think.
Flags: needinfo?(hikezoe)
Matt, I'm trying to decide if this is something I need to worry about for a dot release or not. What are your thoughts?
Flags: needinfo?(matt.woodrow)
Comment on attachment 8985869 [details]
Bug 1469220 - Backed out changeset 7fc66c715a0f and 39cb4d5f6602.

Per IRC discussion with Matt, this is very unlikely to matter much on release at this point.
Flags: needinfo?(matt.woodrow)
Attachment #8985869 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.