Closed Bug 1023344 Opened 11 years ago Closed 8 years ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: emorley, Assigned: dholbert)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

fix v1
895 bytes, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
frame dumps - non-asserting run followed by asserting run
26.05 KB, text/plain
Details
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 }
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.
Dan, can you suggest someone who can look into this rather frequent Android crashtest orange?
Flags: needinfo?(dholbert)
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)
Attached patch fix v1 — — Splinter Review
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)
Attachment #8485151 - Flags: review?(mats)
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+
Oops, I absolutely did! Good catch, thanks.
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/46995fe95c3e
Status: NEW → ASSIGNED
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/46995fe95c3e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Looks like this is still broken on inbound (per e.g. comment 64), even with the landed patch. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(er, s/the recent failures/the recent unexpected passes/, I should say)
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)
Looks like this has been sorted out. My best advice is to use the full logcat.
Flags: needinfo?(gbrown)
Attached file frame dumps - non-asserting runs (obsolete) —
(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
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.
Flags: needinfo?(ryanvm)
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.
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.
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
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)
Try run proved fruitful. Calling this FIXED by bug 812687.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 8 years ago
Depends on: 812687
Flags: needinfo?(dholbert)
Keywords: leave-open
Resolution: --- → FIXED
(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.

Attachment

General

Creator:
Created:
Updated:
Size: