595740 - Crash [@ nsFloatManager::GetFlowArea ] or [@ nsStyleContext::DoGetStyleDisplay(int) ] when print-previewing paradiso-design.net/videostandards.html

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Daniel Holbert [:dholbert]]

STEPS TO REPRODUCE:
 1. Load URL: http://www.paradiso-design.net/videostandards.html
 2. Print-preview before the background-image shows up. (Shift-reload & try again if you're too late.)

ACTUAL RESULTS:
 Crash [@ nsFloatManager::GetFlowArea ]

Crash reports:
 bp-28a6ac57-a8a4-4a8d-8515-3c6a42100912
 bp-8f29f59b-0787-49f5-b8c1-23eb62100912

The Crash Address is "0xf0dea823", which I think (?) is a frame-poisoned value.

Comment 1

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

This seems like something that could be a regression from bug 563584.

Comment 2

[Scoobidiver (away)]

Crash reports for Windows :
Signature    nsStyleContext::DoGetStyleDisplay(int)
bp-2ac521cb-36e1-4e83-acb9-118322100913

Comment 4

[Sheila Mooney]

I am tagging this with the keyword topcrash. This falls inside our top30 criteria on 4.0b6 and it still appearing on the trunk so it hasn't been fixed yet.

Comment 5

[Carsten Book [:Tomcat]]

(In reply to comment #4)
> I am tagging this with the keyword topcrash. This falls inside our top30
> criteria on 4.0b6 and it still appearing on the trunk so it hasn't been fixed
> yet.

you are sure this is a topcrash, shows 18 crashes for beta6 ?

Comment 6

[Sheila Mooney]

I think the volume used to be higher. Chris indicated that any in the top 30 could be considered a topcrash. There are just a handful of crashes now. I am removing the topcrash keyword.

Comment 7

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

Before the crash, I hit two occurrences of:

###!!! ASSERTION: Two floats with same parent in same floats list, expect weird errors.: 'c == f || c->GetParent() != this || !mFloats.ContainsFrame(c)', file /home/dbaron/builds/mozilla-central/mozilla/layout/generic/nsBlockFrame.cpp, line 4633

Comment 8

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

So the reason we have the assertion mentioned in the previous comment is that we push a first-in-flow float to a block that already has that float's next-in-flow.  In other words, the code related to pushing first-in-flows isn't following the invariants about not pushing something to a block that already has a continuation of that float.

I should probably try to confirm that the assertion is related to the crash, though.

Comment 9

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

(In that case, what we probably need to do is destroy the next-in-flow.)

Comment 10

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

I'm reasonably confident it's related; we're crashing after reading the poison pattern out of a "frame" at the same address as the next-in-flow that we asserted about.  (We get two assertions because the loop in nsBlockFrame::DrainPushedFloats (unnecessarily) asserts for the pair both ways round (prev-next and then next-prev).)

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #11)
> *** Bug 622965 has been marked as a duplicate of this bug. ***

Bug 622965 sees a crash with the same signature when trying to save a page (http://www.hpcwire.com/offthewire/Alteras-Stratix-IV-GT-FPGA-Passes-100GbE-Interoperability-Test-112874204.html) as pdf

Comment 13

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

Attachment: valgrind warnings

I read through code yesterday to see how we handle this sort of case in the non-float case, and as far as I can tell we allow it.  As far as I can tell, we allow it; it's ok to have two continuations within the same parent, and presumably by the end of reflow we'll end up with all but the first of them completely empty.

So then, I suppose, the next question is why we crash when we do the same thing with floats.  Here's what valgrind says.

Comment 14

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

...and I think the key there may be that the stacks in the first valgrind warning are mostly common:  all but the top 3 frames of the read and the top 7 frames of the free.  But I'm not really sure, since it's not clear which functions have different parameters.

Comment 15

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

I wonder if we're going through a multi-pass reflow without properly pushing and popping float manager state.

Comment 16

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

Attachment: equivalent stacks, with parameters

Comment 17

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

Attachment: reflow log

And when I set up reflow logging, there was a little bit of reflowing between the two-floats assertion and the DestroyFrom call, but none between the DestroyFrom call and the crash.

Comment 18

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

http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/524a87d58df3/handle-multiple-continuations
fixes the crash (although not the assertions) on http://www.paradiso-design.net/videostandards.html .

I still need to:
 * figure out whether I want to remove the assertions or replace them with something else
 * figure out if I can make a simplified testcase

Comment 19

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

(In reply to comment #18)
> http://www.paradiso-design.net/videostandards.html .

Unfortunately, save-page-as, complete, doesn't even give a reproducable testcase, so I'm stumped as far as getting a simpler testcase goes.

Comment 20

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

I can use <base href>, though.

Comment 21

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

Attachment: partially simplified testcase

simplified from http://www.paradiso-design.net/videostandards.html

Comment 22

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

Attachment: a bit more simplified

Comment 23

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

Attachment: and more simplified

The trick is that the "y" has to end up at the very bottom of the page, such that a letter following the <br> after it would be on the next page.

Comment 24

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

Attachment: minimal testcase (for default US printer settings, anyway)

Comment 25

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

Attachment: patch 1: move float assertion

Comment 26

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

Attachment: patch 2: handle multiple continuations correctly

Comment 27

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

Attachment: patch 3: test

Comment 28

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

http://hg.mozilla.org/mozilla-central/rev/5eb94b2c7606
http://hg.mozilla.org/mozilla-central/rev/e7504a9cf0cb
http://hg.mozilla.org/mozilla-central/rev/37d585cbcb75