363722 - Marquee testcase from bug 239840 is crashing Mozilla

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

I'm filing this as a separate bug, just to not make bug 239840 even more confusing.

"Attached testcase" from bug 239840 is now crashing current trunk Mozilla builds.
This regressed between 2006-12-10 and 2006-12-12, I think a regression from my patch for bug 363285.
But my patch is just a markup patch, which means that the deeper problem is in layout code, it seems to me this is somehow a regression from the reflow branch landing, bug 300030.

Talkback ID: TB27275767Q
ntdll.dll + 0xeddc (0x7c90eddc)
MSVCR80.dll + 0x4ce9 (0x78134ce9)
0x06f06800
It seems like an infinite recursion crash.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I see several things:

1)  JS engine cutting off infinite recursion.
2)  Lots of asserts.

We should sort out the latter.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the issue is that we end up computing an unconstrained pref width when refreshing the size cache for a div whose parent is a boxframe for a div.  I haven't figured out yet how nsBlockFrame::GetPrefWidth is managing to return unconstrained.

I did figure out the JS recursion errors -- those come from trying to execute XBL fields from deep inside frame construction -- with 2600 frames on the stack (each binding adds a bunch of nested stuff, and frame construction is a bunch of frames per content node), we're over the JS engine's recursion limit, so it bails when we try to JS_CompileUCScriptForPrincipals.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, so the problem with the asserts is that the box has 100% margin on both sides.  So nsLayoutUtils::IntrinsicForContainer ends up returning nscoord_MAX for the pref width....  Then we end up setting that as the pref width of the div in the size cache, and reflowing at that size.

Not sure whether fixing that will fix the crash, but it's a start...

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Reduced Testcase

This simple testcase just has a doubly-nested marquee, and nothing else fancy.

It triggers the assertion failures, but not the js recursion errors. (As bz said, those are caused by having many levels of marquees)

Comment 5

[Eli Friedman]

Attachment: Fix for the assertions

This fix isn't really right, but it's probably the best we can do without some major architecture work.

(I'm not sure what's going on with the recursion yet.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Reduced Testcase (colors)

Variation on first reduced testcase, with colored backgrounds to make the marquee-within-marquee easier to see.

This testcase works in both IE and Konqueror, albeit differently in each:
  IE: Inner Marquee is as wide as outer marquee.
  Konqueror: Inner marquee is as wide as its content's preferred width.

The IE strategy seems more correct to me, since auto-width marquees generally expand to fill their container.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Another Testcase (colors, text in outer marquee)

Variant testcase with some text inside the outermost marquee.

The current patch (attachment 270095 [details] [diff] [review]) doesn't fix the assertions for this testcase.  Using that patch, I still get lots of assertions about unconstrained widths on this testcase. (I think it's most or all of the assertions that the patch had fixed for the other testcases.)

Comment 8

[Daniel Holbert [:dholbert]]

Using this bug's testcases, I found a place where we need to (but don't yet) check for nscoord_MAX when adding nscoords -- nsFrame::AddInlinePrefWidth().  (source: http://shorl.com/pydadrebebridu)
 
My patch for Bug 367673 covers nscoord_MAX arithmetic, using a helper function, NSCoordAddCheckingMax.

I just updated the patch on that bug to use the helper function in nsFrame::AddInlinePrefWidth().  With that change (i.e. if you apply patch for Bug 367673), Eli's patch will now fix the assertions and display on "Another Testcase".

Comment 9

[Daniel Holbert [:dholbert]]

In addition to fixing the assertions, Eli's patch also fixes the hang on the testcase that this bug is named after, attachment 148963 [details].  (My build also has the patch for Bug 367673 applied, but I'm not sure if that's necessary to fix the hang here.)

- Without the patch, that page takes > 1 minute to load. (I just killed the process after a minute; not sure how long it actually takes).

- After Eli's patch, the page loads in 8 sec.  It continues to use up 99% of the CPU (probably because of all the nested marquee-scrolling computations), but Firefox is still responsive, at least.

Comment 10

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

Why are you ignoring the nscoord_MAX pref width and setting mBlockPrefSize.width to the min size?  nscoord_MAX is a legitimate pref width.

And it seems like you still might be able to get some height information.  (Maybe reflow with a width of nscoord_MAX - 1?)

Comment 11

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

Daniel, I hope you can finish this patch off.

Comment 12

[Daniel Holbert [:dholbert]]

This bug has become WORKSFORME in current trunk.

I get no assertions on any of the testcases, and when I load attachment 148963 [details], I get the post-patch behavior I described in comment 9 (and no assertions):

> - the page loads in 8 sec.  It continues to use up 99% of
> the CPU (probably because of all the nested marquee-scrolling computations),
> but Firefox is still responsive, at least.

Resolving.

Comment 13

[Martijn Wargers (dead)]

Are you sure this wasn't "fixed" by bug 407016, which would mean it would still be possible to make it crash this way?

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #13)
> Are you sure this wasn't "fixed" by bug 407016,
> which would mean it would still
> be possible to make it crash this way?

After testing a few nightlies, it looks like the crash has actually been gone for some time. (Since before June, at least, according to a June nightly that I just tested.  I do remember that when I posted Comment 9 back in September, the crash was definitely gone at that point.)  The only remaining issue was asserts and long-ish hangs in debug builds, I think.
  
Those asserts and long-ish hangs are now fixed, probably due to the fix for bug 407016 or a related marquee fix.  

Perhaps it's still possible for someone to construct a different testcase that works around whatever fixed this and causes a longish hang or asserts or a crash -- I don't know.  But if so, I think that probably merits a new bug, because this one already covers multiple issues that have become WFM at various times.

Comment 15

[Martijn Wargers (dead)]

Ok, thanks for the explanation.

Comment 16

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020819 Minefield/3.0b4pre - no crash on testcases

- Verified fixed

Comment 17

[Martijn Wargers (dead)]

The testcase started crashing again in current trunk build, I filed bug 425253 for it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Could we just get this test into the test suite so that doesn't happen again?

Comment 19

[Daniel Holbert [:dholbert]]

I checked in "Reduced Testcase (colors)" and "Another Testcase (colors, text in outer marquee)" as crashtests:
http://hg.mozilla.org/mozilla-central/rev/30cf2c48c232
http://hg.mozilla.org/mozilla-central/rev/bc4871c9d39a
These tests both used to trigger assertions, but the assertions were fixed when this bug became fixed.

I specifically didn't add the originally-crashing testcase mentioned in comment 0 ("Attached testcase" from bug 239840), because that testcase still hangs for about 7 seconds in my mozilla-central nightly, and it could easily sporadically timeout on a tinderbox.

Comment 20

[(no longer active)]

Looks like we need to back out bug 407016 in order to address bug 508816, so this needs to be fixed for real.

Comment 21

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

If you back out bug 407016, does this reoccur?

Comment 22

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

Purely based on that this blocks bug 508816, making this a blocker. If this turns out not to block 508816 then it should be reset to nominated or not a blocker.

Comment 23

[(no longer active)]

(In reply to comment #21)
> If you back out bug 407016, does this reoccur?

I don't get crashes, but here's what I do get:

REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/265867-1.html | assertion count 6 is less than expected 9 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/363722-1.html | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/363722-2.html | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/370866-1.xhtml | assertion count 16 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/379917-1.xhtml | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/421671.html | assertion count 6 is more than expected 0 to 2 assertions

I need to investigate further.

Comment 24

[(no longer active)]

Attachment: Patch that I used to back out bug 407016

Comment 25

[(no longer active)]

Actually this is what I get from running the entire crashtest suite:

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/content/base/crashtests/371466-1.xhtml | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/crashtests/467647-1.html | assertion count 0 is less than expected 2 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/editor/txmgr/tests/crashtests/407072-1.html | assertion count 0 is less than expected 3 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/base/crashtests/265027-1.html | assertion count 113 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/layout/base/crashtests/265986-1.html | assertion count 0 is less than expected 8 to 12 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/base/crashtests/404491-1.html | assertion count 14 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/265867-1.html | assertion count 4 is less than expected 9 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/363722-1.html | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/363722-2.html | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/370866-1.xhtml | assertion count 16 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/379917-1.xhtml | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/421671.html | assertion count 6 is more than expected 0 to 2 assertions

Comment 26

[(no longer active)]

Attachment: Patch that I used to back out bug 407016

Actually I had a typo in porting the CVS patch over to trunk, so comment 23 and 25 should be ignored.

Comment 27

[(no longer active)]

Here's the real list of crashtest failures.

REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/editor/libeditor/html/crashtests/467647-1.html | assertion count 0 is less than expected 2 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/editor/txmgr/tests/crashtests/407072-1.html | assertion count 0 is less than expected 3 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/base/crashtests/265027-1.html | assertion count 113 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/layout/base/crashtests/265986-1.html | assertion count 0 is less than expected 8 to 12 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/base/crashtests/404491-1.html | assertion count 14 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-PASS | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/265867-1.html | assertion count 4 is less than expected 9 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/363722-1.html | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/363722-2.html | assertion count 4 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/370866-1.xhtml | assertion count 16 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/379917-1.xhtml | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/ehsanakhgari/moz/mozilla-central/layout/generic/crashtests/421671.html | assertion count 6 is more than expected 0 to 2 assertions

Comment 28

[(no longer active)]

Note to myself: smontagu's latest patches in bug 508816:

<http://tbpl.mozilla.org/?tree=MozillaTry&rev=d10d72af861d>

Comment 29

[(no longer active)]

None of the tests in this bug crash Firefox with the patches in bug 508816.  They just cause some test failures which I'm going to address on that bug.  IOW, this doesn't need to block.

Comment 30

[Simon Montagu :smontagu]

I think there's a misunderstanding here: <http://tbpl.mozilla.org/?tree=MozillaTry&rev=d10d72af861d> included attachment 270095 [details] [diff] [review] for testing purposes. Without it and with the other patches from bug 508816 I get tons of assertions as in comment 27, but it was r-'ed, so we are going to need an alternative version.

Comment 31

[(no longer active)]

Indeed.  There was confusion here on my part.

Comment 32

[(no longer active)]

Simon confirmed that there is nothing to be done here after all!