462392 - RebuildAllStyleData on iframe can crash-on-unwind after flushing parent and destroying iframe

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

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

Attachment: simplified testcase

Yesterday Jesse Ruderman gave me an unreduced testcase (that he was having trouble reducing) that crashed in a way that I thought was debuggable with valgrind.  It turns out it was, and from the stacks I got from valgrind I wrote a more reliable and simpler testcase to exercise the same bug.

Marking [sg:critical?] given that it's dereferencing deleted objects (and potentially making virtual function calls on them).

Comment 1

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

Attachment: assertion followed by first valgrind warning

The valgrind warning here explains the problem.

I'm not sure if the assertion showed up on Jesse's testcase, or exactly how related it is...

Comment 2

[Jesse Ruderman]

My testcase triggered the AllocateFrame assertion too.

Comment 3

[Martijn Wargers (dead)]

I guess bug 435895 might be related to this.

Comment 4

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

There seem to be two ways a page can trigger RebuildAllStyleData:
 * location.reload() or history.go(0) within a resize event
 * triggering a media feature value change (e.g., by a resize) -- which can be flushed

Comment 5

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

I'm thinking that a few kungFuDeathGrips (on pres shells) might be the way to go here.

Comment 6

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

Attachment: patch for crash

This fixes the crash, but not the AllocateFrame assertion.

(I'm wondering if the AllocateFrame assertion is related to stuff allocated at a caller on the stack.)

Comment 7

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

According to valgrind leak checking with DEBUG_TRACEMALLOC_FRAMEARENA, the leak was because we leaked a rule tree.  However, it doesn't show up with XPCOM_MEM_LEAK_LOG, which is odd.

Comment 8

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

I think not showing up in XPCOM_MEM_LEAK_LOG is because the leak is due to the null-check in nsPresContext::FreeToShell.

Comment 9

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

So, I'm really not sure what to do about the AllocateFrame assertion.  The problem is that the pres context's pres shell pointer is null after PresShell::Destroy is called, which is when we destroy mOldRuleTree.  We could destroy mOldRuleTree during style set destruction, I suppose, but the way things are right now isn't actually doing any harm (I don't think); it's just asserting.

Comment 11

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

Fixed: http://hg.mozilla.org/mozilla-central/rev/5785b70454d3

Comment 12

[Jesse Ruderman]

I filed bug 462788 on the leak and assertion.

Comment 13

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

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204034421 and the appropriate build on OS X.

Comment 14

[Mats Palmgren (inactive)]

crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7301d150bc12

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7301d150bc12