473042 - Crash [@ nsStyleContext::Destroy] with display:none root

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Crashes calling 0xdddddddd.  Regression from bug 283686?

Comment 1

[Daniel Holbert [:dholbert]]

I can reproduce this crash on Linux:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090112
Minefield/3.2a1pre
http://crash-stats.mozilla.com/report/index/2afa9993-1e21-4a53-8508-29a9d2090112?p=1
http://crash-stats.mozilla.com/report/index/7e3d5691-1e82-4679-abf9-d08e02090112?p=1

OS --> All

Comment 2

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

Attachment: valgrind's explanation for the crash

Comment 3

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

Attachment: patch

Comment 4

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

Oops, that assertion should be asserting about undisplayedParent, not localContent.  I'll fix that locally.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 356626 [details] [diff] [review]
patch

With the assert fixed, looks good.

Comment 6

[Daniel Holbert [:dholbert]]

Note that the bug that caused this behavior, bug 283686, is currnetly marked as "[needs 1.9.1 landing]".  If/when that lands, we should make sure to land this as well.

Comment 7

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

Fixed: http://hg.mozilla.org/mozilla-central/rev/046f5da25280

Comment 8

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

Backed out due to crash in layout/forms/test/ mochitests.
http://hg.mozilla.org/mozilla-central/rev/dc598a5eafe1
http://hg.mozilla.org/mozilla-central/rev/1d21f44cf01c

Comment 9

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

Specifically, it's layout/forms/test/test_bug402198.html , which is actually toggling display:none on the root.

Comment 10

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

Attachment: valgrind's explanation for crash on mochitest

Comment 11

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

Attachment: patch

This has two additional changes from the previous patch.

The first is the fix for the crash:  the problem was that the primary frame for the root element was ending up being the canvas frame because the code to reset the primary frames for the scrollbars could end up, when no scrollbars were present, setting the primary frame for the root to be the canvas.  This meant that, when the root was undisplayed and we reframed it, we'd delete the frame constructor's mDocElementContainingBlock without clearing the mDocElementContainingBlock pointer.

The second change is the added call to ClearUndisplayedContentIn, which I think is also important.  (I wrote that part first, actually.)

Comment 12

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

Attachment: patch

Here's a version that also has an assertion in SetPrimaryFrameFor.

Comment 13

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

And I checked that the assertion does fire without the ConstructDocElementFrame part of the patch.

Comment 14

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

Comment on attachment 357027 [details] [diff] [review]
patch

Ran full reftests and mochitests without crashing and without hitting my new assertion.  (I think the failures I saw in mochitest are unrelated.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 357027 [details] [diff] [review]
patch

r+sr=bzbarsky

Comment 16

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

Fixed: http://hg.mozilla.org/mozilla-central/rev/867b1cf06252

Comment 17

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d3e2e7c4578f

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090204 Minefield/3.2a1pre ID:20090204032912

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204034421

Comment 19

[Daniel Veditz [:dveditz]]

I don't crash on 1.9.0.11pre, I assume we aren't going to take correctness bug 283686 on the 1.9.0 branch and therefore don't need this fix either.

Comment 20

[Daniel Veditz [:dveditz]]

Incidentally, is this the right testcase? I get an XML error in both 1.9.0 and 1.9.1 -- my previous comment was based on adding the missing </mrow> to make the parser happy, but maybe that wasn't the point.

Comment 21

[Mats Palmgren (inactive)]

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fa55f7a3d14

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3fa55f7a3d14