Intermittent 876074-1.html | assertion count 0 is less than expected 1 to 4 assertions

RESOLVED FIXED in mozilla35

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: emorley, Assigned: dholbert)

Tracking

({intermittent-failure})

Trunk
mozilla35
intermittent-failure
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Rev4 MacOSX Snow Leopard 10.6 fx-team debug test crashtest on 2014-06-10 00:34:20 PDT for push 410f509e9913

slave: t-snow-r4-0013

https://tbpl.mozilla.org/php/getParsedLog.php?id=41418627&tree=Fx-Team

{
00:43:22     INFO -  REFTEST TEST-START | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/layout/generic/crashtests/876074-1.html
00:43:23     INFO -  REFTEST TEST-LOAD | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/layout/generic/crashtests/876074-1.html | 1783 / 2659 (67%)
00:43:23     INFO -  ++DOMWINDOW == 139 (0x14959e400) [pid = 972] [serial = 4172] [outer = 0x11a083000]
00:43:23     INFO -  [972] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /builds/slave/fx-team-osx64-d-00000000000000/build/dom/events/ContentEventHandler.cpp, line 110
00:43:23     INFO -  [972] WARNING: no buffer: 'mDTBuffer', file /builds/slave/fx-team-osx64-d-00000000000000/build/gfx/layers/RotatedBuffer.cpp, line 339
00:43:23     INFO -  REFTEST TEST-PASS | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/layout/generic/crashtests/876074-1.html | (LOAD ONLY)
00:43:23     INFO -  REFTEST INFO | Loading a blank page
00:43:23     INFO -  ++DOMWINDOW == 140 (0x12d6aac00) [pid = 972] [serial = 4173] [outer = 0x11a083000]
00:43:23     INFO -  REFTEST TEST-UNEXPECTED-PASS | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/layout/generic/crashtests/876074-1.html | assertion count 0 is less than expected 1 to 4 assertions
}
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
I think this afternoon's burst of failures here were not actually the same failure. They all started after Bug 1041751 landed, and they appear to have disappeared after that was backed out.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Dan, can you suggest someone who can look into this rather frequent Android crashtest orange?
Flags: needinfo?(dholbert)
(Assignee)

Comment 53

4 years ago
Sure, I can take it.

If Android isn't asserting, that implies that it's not actually triggering the error condition -- which probably means it's doing the dynamic tweak ("boom()") before we've performed our first layout.

We should key the dynamic tweak off of MozReftestInvalidate, to be sure it's actually operating on an already-reflowed document, so that it actually triggers the issue. Then it should assert reliably, I'd expect.
Assignee: nobody → dholbert
Flags: needinfo?(dholbert)
(Assignee)

Comment 54

4 years ago
Created attachment 8485151 [details] [diff] [review]
fix v1
Attachment #8485151 - Flags: review?(ryanvm)
Comment on attachment 8485151 [details] [diff] [review]
fix v1

I don't think I'm a capable reviewer of this.
Attachment #8485151 - Flags: review?(ryanvm)
(Assignee)

Updated

4 years ago
Attachment #8485151 - Flags: review?(mats)
Comment hidden (Treeherder Robot)
Comment on attachment 8485151 [details] [diff] [review]
fix v1

You meant to also remove the onload, right?
r=mats with that.
Attachment #8485151 - Flags: review?(mats) → review+
(Assignee)

Comment 58

4 years ago
Oops, I absolutely did! Good catch, thanks.
(Assignee)

Comment 59

4 years ago
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/46995fe95c3e
Status: NEW → ASSIGNED
Flags: in-testsuite-
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
https://hg.mozilla.org/mozilla-central/rev/46995fe95c3e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment hidden (Treeherder Robot)
(Assignee)

Comment 67

4 years ago
Looks like this is still broken on inbound (per e.g. comment 64), even with the landed patch. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 71

4 years ago
I don't have any bright ideas here.

Really, this testcase is serving two purposes:
 (1) Make sure we don't abort with this content (as we did when bug 876074 was filed)
 (2) Serve as a canary to check that bug 876749 is still an issue. (That's the source of the assertions.)

The fact that we sometimes *don't assert* (and hence trigger a crashtest orange) is mysterious.  But really, not-asserting is the goal, and we'll get there once bug 876749 is fixed. :) So it's probably not worth spending too much time trying to figure out what's going on with the occasional-not-asserting here.

It looks like the recent failures are all Android-specific, so we could probably just annotate this as  asserts-if(Android,0-4) to silence these. That would let the test continue serving purpose (1) on Android, while still serving purpose (1) and (2) on other platforms.
(Assignee)

Comment 72

4 years ago
(er, s/the recent failures/the recent unexpected passes/, I should say)
(Assignee)

Comment 73

4 years ago
Landed a patch to make the change I suggested at the end of comment 71:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f6bbcd623b

Marking "leave-open" for now, so it continues to show up as a valid suggestion in Tbpl, to see if we still get any instances of this on other platforms.
Keywords: leave-open
OS: Mac OS X → All
Hardware: x86_64 → All
Fwiw, I was interested in what the frame trees looks like in FlextContainerFrame::Reflow,
but it seems we don't get frame dumps in the log file for some reason.  (I guess the
logcat limit is too small to have caught it.)  Is there a way to mark a region in the
output so that it's included in the test results?
https://tbpl.mozilla.org/?tree=Try&rev=ecba8f8d8422
(there were 2 failures in ~250 runs of this single test)
(In reply to Mats Palmgren (:mats) from comment #74)
> Is there a way to mark a region in the
> output so that it's included in the test results?
> https://tbpl.mozilla.org/?tree=Try&rev=ecba8f8d8422
> (there were 2 failures in ~250 runs of this single test)

Maybe Geoff can answer that.
Flags: needinfo?(gbrown)
Comment hidden (obsolete)
Comment hidden (obsolete)
Looks like this has been sorted out. My best advice is to use the full logcat.
Flags: needinfo?(gbrown)
Created attachment 8486508 [details]
frame dumps - non-asserting runs

(In reply to Daniel Holbert [:dholbert] from comment #77)
> So, it's not surprising that the log contains
> some assertions.

Right, the 2 "failures" I mentioned above is the runs that *didn't* assert.
Here are the frame dumps for those 2 runs.

Unfortunately, it doesn't show anything interesting since I dumped
the frames at the start of Reflow, before the sorting occurs.
It's the same frame trees as in the asserting runs.

Here's a new try that also dumps frames after sorting and
some other info leading up to that:
https://tbpl.mozilla.org/?tree=Try&rev=43e3cf30aeae
Created attachment 8486822 [details]
frame dumps - non-asserting run followed by asserting run

The first four frame trees are from a test run that does NOT assert.
(2 tree dumps per reflow, and 2 reflows per test)
In the 1st reflow, we have flex items: div, table, canvas.
In the 2nd reflow, we have flex items: div, canvas, table.
HasAnyStateBits(NS_STATE_FLEX_CHILDREN_REORDERED) == 0 always.

The last four frame trees are from a test run that does assert.
It has the same order of flex items.

The FlexContainerFrame has the same frame state in the first
reflow in both runs, but a different state in the 2nd reflow.
The non-asserting run has 0x0002120000001201
The asserting run has     0x0000120000001201
diff --------------------------^

// Frame has a descendant frame that needs painting - This includes
// cross-doc children.
FRAME_STATE_BIT(Generic, 49, NS_FRAME_DESCENDANT_NEEDS_PAINT)

Looking the state of the flex items, that appears to be the
TableOuter frame which has it too: 0x0002000000000200
and Table has 0x0001000000000000 :

// Frame is marked as needing painting
FRAME_STATE_BIT(Generic, 48, NS_FRAME_NEEDS_PAINT)

So my conclusion is that the run that didn't assert unloaded the
document with a not fully painted frame tree.  If it had painted
it it would have asserted b/c the frame trees are the same in
both runs (and the assertion happens during paint).

So, I think the missing assertion can be safely ignored.

It might be a bug in the reftest framework though.
Attachment #8486508 - Attachment is obsolete: true
BTW, all test runs warns:
REFTEST INFO | [CONTENT] MozInvalidateEvent didn't invalidate
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#459

I'm guessing the boom() function doesn't cause a paint. Maybe
the reftest framework only cares about paints caused by the
MozReftestInvalidate callback and not anything that happened
before that.
Comment hidden (Treeherder Robot)
Flags: needinfo?(ryanvm)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 92

3 years ago
The "assertion count 5 is more than expected 1 to 4 assertions" issues here seem to all be cases where we're spamming the same assertion 5 times (instead of spamming it 1-4 times).

I think we spam it each time we reflow the nsFlexContainerFrame. It's marginally concerning that we're occasionally getting 1 more reflow than the maximum that we used to be getting here. But the extra assertion isn't concerning in and of itself; we can just bump the count up to 1-5 I guess.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 106

3 years ago
Pushed a tweak to broaden the allowed assertion range from 1-4 to 0-5 ^^^, to allow for the recent intermittents here.

Justification:
 * The instances with 5 assertions aren't really any more concerning than the ones with 4 assertions, per comment 92.

 * The instances with 0 assertions are a bit mysterious (since I think they imply that we may not be reflowing, or something); see comment 71 for thoughts on those. But per that comment & comment 73, we're already allowing for that on Android; and we're still getting the benefit of making sure this test doesn't crash/abort.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Comment 121

2 years ago
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
(Assignee)

Comment 122

11 months ago
As discussed in bug 876749 comment 4 (that's the tracking bug for the "asserts" annotation that we were violating here until comment 106): this testcase's assertions have probably gone away.

If my Try run over there proves fruitful, I'll close out that bug and this bug as well.
Flags: needinfo?(dholbert)
(Assignee)

Comment 123

11 months ago
Try run proved fruitful. Calling this FIXED by bug 812687.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago11 months ago
Depends on: 812687
Flags: needinfo?(dholbert)
Keywords: leave-open
Resolution: --- → FIXED
(Assignee)

Comment 124

11 months ago
(And I removed the asserts(0-5) annotation in bug 876749 comment 6, too.)
You need to log in before you can comment on or make changes to this bug.