366021 - "ASSERTION: cannot call on a dirty frame not currently being reflowed" with abs pos, <tr>, <xul:window>

Status: RESOLVED WORKSFORME

(Core :: Layout: Positioned, defect)

Description

[Jesse Ruderman]

Loading this testcase triggers two of these assertions (with different line numbers), each twice:

###!!! ASSERTION: cannot call on a dirty frame not currently being reflowed: '!(GetStateBits() & (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)) || (GetStateBits() & NS_FRAME_IN_REFLOW)', file /Users/admin/trunk/mozilla/layout/generic/nsFrame.cpp, line 714

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: stack trace

Comment 3

[Alex Vincent [:WeirdAl]]

Also be aware of bug 369563, which triggers a similar assertion with a totally different stack trace and different steps to reproduce.

Comment 4

[Jesse Ruderman]

I filed bug 387213 with another testcase that triggers this assertion.  The testcase in that bug is simpler than the one here.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

This should fix this one and bug 369563

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure if I need to deal with recursion. If I don't the patch can be simplified to not use |disableAssert|

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

Doh! Edited out too much of the previous patch before attaching.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v3

Roc asked me to create a helperclass instead. Not that much less code, but it is more readable.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v4

Ugh, manual patch editing failed again.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Disabling during painting seems wrong.  I agree that we should disable it during event handling (there's another bug discussing that), but why does it need to be disabled during painting?

This bug, on the other hand, is about the assertion firing during reflow, which your patch wouldn't fix.  Maybe it belongs on a different bug?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #10)
> Disabling during painting seems wrong.  I agree that we should disable it
> during event handling (there's another bug discussing that),

... bug 399352.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We aren't always able to fully flush reflows before painting. See
http://mxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#5894

[This will still be true even in a post-compositor world, assuming we do interruptible reflow as well. For example if you have some translucent chrome floating over a content window and we need to repaint that chrome, we'll have to paint the content window without flushing its reflows, otherwise chrome event processing would be blocked by the reflow flush, basically eliminating the point of interruptible reflow. In general we need to be able to paint and handle events in a dirty frame tree.]

I'm not really sure what we should do about this. Perhaps we should just convert those assertions to warnings.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, we could make frames have two style context pointers, one reflecting what should be used in the next reflow, and one reflecting what was used in the last one.

But this discussion really belongs in bug 399352.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 295335 [details] [diff] [review]
Patch v4

Moved patch and discussion to bug 399352

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Reassigning to nobody. The patches above are for when this assertion fires under completely different circumstances. So they don't relate to this bug at all.

Sorry about the confusion :(

The stack in this bug looks very similar to bug 387213 though, so possibly a dupe.

Comment 16

[Jesse Ruderman]

WFM on trunk.  I can still reproduce bug 387213, though.