1427221 - Intermittent Assertion failure: uint8_t(mClass) < mozilla::ArrayLength(sLayoutFrameTypes), at /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:2796

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: nerli [at] mozilla.com

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

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

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/MqgorJVtTJW5vv2IpBGFig/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 1

[Intermittent Failures Robot]

11 failures in 147 pushes (0.075 failures/push) were associated with this bug in the last 7 days. 

This is the #18 most frequent failure this week.   

Repository breakdown:
* autoland: 7
* mozilla-inbound: 3
* mozilla-central: 1

Platform breakdown:
* linux32-stylo-disabled: 5
* linux64-stylo-disabled: 4
* linux64: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1427221&startday=2017-12-25&endday=2017-12-31&tree=all

Comment 2

[Chris Peterson [:cpeterson]]

This assertion failure is bad because it indicates we are reading off the end of an array. Mats added this assertion in nsIFrame bug 1381405.

  mozilla::LayoutFrameType Type() const {
    MOZ_ASSERT(uint8_t(mClass) < mozilla::ArrayLength(sLayoutFrameTypes));
    return sLayoutFrameTypes[uint8_t(mClass)];
  }

Comment 3

[Mats Palmgren (inactive)]

I doubt there's an issue with a valid frame having an invalid mClass.
I suspect the assertion fails because we use a frame that has been
destroyed and is now poisoned.

Comment 4

[Mats Palmgren (inactive)]

Attachment: The assertion stack for posterity

#01: nsIFrame::Type [layout/generic/nsIFrame.h:2796]
#02: RetainedDisplayListBuilder::AttemptPartialUpdate [layout/painting/RetainedDisplayListBuilder.cpp:756]
#03: nsLayoutUtils::PaintFrame [layout/base/nsLayoutUtils.cpp:3809]
...

Comment 5

[Intermittent Failures Robot]

29 failures in 462 pushes (0.063 failures/push) were associated with this bug in the last 7 days. 

This is the #32 most frequent failure this week.   

Repository breakdown:
* mozilla-inbound: 11
* autoland: 9
* mozilla-central: 8
* try: 1

Platform breakdown:
* linux64-stylo-disabled: 12
* linux64: 9
* linux32-stylo-disabled: 8

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1427221&startday=2018-01-01&endday=2018-01-07&tree=all

Comment 6

[Matt Woodrow (:mattwoodrow)]

This is a regression from bug 1426345.

When we destroy a frame, we call RemoveDisplayItemDataForDeletion, which includes removing references to this frame from the root frame's ModifiedFrameList.

Unfortunately, nsMathMLmfencedFrame's destructor (which runs after this) calls a function which adds it back again, and we end up with a dangling pointer.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Defer cleaning up the modified state until the last possible opportunity

Moving this code to the base class destructor should prevent similar bugs from popping up in other frame types.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Change test to fail every time

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8940562 [details] [diff] [review]
Defer cleaning up the modified state until the last possible opportunity

Doing "complicated" stuff like this in the dtor should be avoided.
I think this bug neatly demonstrates that it's a footgun.

Allow me to suggest an alternative fix...

Comment 12

[Mats Palmgren (inactive)]

Attachment: Alternative fix

Overriding DestroyFrom is the right way to do this I think.

I also added an assertion that would have caught this bug earlier.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8941025 [details] [diff] [review]
Alternative fix

Review of attachment 8941025 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +858,5 @@
> +    if (this != rootFrame) {
> +      nsTArray<nsIFrame*>* modifiedFrames =
> +        rootFrame->GetProperty(nsIFrame::ModifiedFrameList());
> +      if (modifiedFrames) {
> +        MOZ_ASSERT(!modifiedFrames->Contains(this),

Could we also/instead assert(!IsFrameModified()) from the nsFrame dtor?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3722e968f87b
part 1 - Do nsMathMLmfencedFrame cleanup in DestroyFrom, not in the dtor.  r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/6397787172eb
part 2 - Change test to fail every time.  r=mats

Comment 15

[Mats Palmgren (inactive)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Could we also/instead assert(!IsFrameModified()) from the nsFrame dtor?

That assertion would fail:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1580c0da1227bf73dce4ececc3936bd8e0503bf

I'm guessing it's because nsIFrame::RemoveDisplayItemDataForDeletion
doesn't call SetFrameIsModified(false).

It doesn't seem super-important to correct the bit there since we're
about to delete the frame anyway, but feel free to file a follow-up
bug if you want.

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3722e968f87b
https://hg.mozilla.org/mozilla-central/rev/6397787172eb

Comment 17

[Astley Chen (inactive)]

Matt, could you help to create the uplift request for 58 so that we can fix bug 1428178 accordingly?

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8941025 [details] [diff] [review]
Alternative fix

Approval Request Comment
[Feature/Bug causing the regression]:bug 1426345
[User impact if declined]:crash
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:no
[Why is the change risky/not risky?]:trivial fix
[String changes made/needed]:none

Comment 19

[Intermittent Failures Robot]

32 failures in 788 pushes (0.041 failures/push) were associated with this bug in the last 7 days.   

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **  

Repository breakdown:
* autoland: 12
* mozilla-inbound: 8
* try: 5
* mozilla-central: 5
* oak: 2

Platform breakdown:
* linux32-stylo-disabled: 17
* linux64-stylo-disabled: 11
* linux64: 3
* linux64-ccov: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1427221&startday=2018-01-08&endday=2018-01-14&tree=all

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8941025 [details] [diff] [review]
Alternative fix

Fix a regression. Beta58+.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/ae6c87e225d3

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/72faa446f04e (FIREFOX_58b_RELBRANCH)