Closed Bug 363722 Opened 14 years ago Closed 10 years ago

Marquee testcase from bug 239840 is crashing Mozilla

Categories

(Core :: Layout, defect, P2)

x86
All
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: martijn.martijn, Assigned: ehsan)

References

(Blocks 1 open bug, )

Details

(Keywords: crash, regression, testcase)

Attachments

(5 files, 1 obsolete file)

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.
So I see several things:

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

We should sort out the latter.
Flags: blocking1.9?
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.
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...
Flags: blocking1.9? → blocking1.9+
Attached file 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)
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.)
Attachment #270095 - Flags: review?(dbaron)
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.
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.)
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".
Depends on: 367673
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.
OS: Windows XP → All
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?)
Whiteboard: [dbaron-1.9:RwCr]
Attachment #270095 - Flags: review?(dbaron) → review-
Daniel, I hope you can finish this patch off.
Assignee: nobody → 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.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
Flags: in-testsuite?
Are you sure this wasn't "fixed" by bug 407016, which would mean it would still be possible to make it crash this way?
(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.
Ok, thanks for the explanation.
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
Status: RESOLVED → VERIFIED
The testcase started crashing again in current trunk build, I filed bug 425253 for it.
Could we just get this test into the test suite so that doesn't happen again?
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.
Flags: in-testsuite? → in-testsuite+
Looks like we need to back out bug 407016 in order to address bug 508816, so this needs to be fixed for real.
Assignee: dholbert → ehsan
Status: VERIFIED → REOPENED
blocking2.0: --- → ?
Resolution: WORKSFORME → ---
Whiteboard: [dbaron-1.9:RwCr]
Blocks: 508816
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.
blocking2.0: ? → betaN+
Whiteboard: [hardblocker]
(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.
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
Actually I had a typo in porting the CVS patch over to trunk, so comment 23 and 25 should be ignored.
Attachment #508846 - Attachment is obsolete: true
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
Note to myself: smontagu's latest patches in bug 508816:

<http://tbpl.mozilla.org/?tree=MozillaTry&rev=d10d72af861d>
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.
Status: REOPENED → RESOLVED
blocking2.0: betaN+ → -
Closed: 13 years ago10 years ago
Resolution: --- → WORKSFORME
Whiteboard: [hardblocker]
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.
Indeed.  There was confusion here on my part.
Status: RESOLVED → REOPENED
blocking2.0: - → betaN+
Resolution: WORKSFORME → ---
Whiteboard: [hardblocker]
Simon confirmed that there is nothing to be done here after all!
Status: REOPENED → RESOLVED
blocking2.0: betaN+ → ---
Closed: 10 years ago10 years ago
Resolution: --- → WORKSFORME
Whiteboard: [hardblocker]
You need to log in before you can comment on or make changes to this bug.