459666 - "ASSERTION: GetPrimaryFrameFor() called while frames are being destroyed!" with filter in HTML

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase triggers the assertion added in bug 458636:

###!!! ASSERTION: GetPrimaryFrameFor() called while frames are being destroyed!: '!mIsDestroyingFrames', file /Users/jruderman/central/layout/base/nsFrameManager.cpp, line 328

Is this the same issue as bug 458493?

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack for the assertion

Comment 2

[Mats Palmgren (inactive)]

Bug 458453 fixes this. It's the issue I pointed out in bug 458453 comment 2.

Comment 3

[Jesse Ruderman]

Bug 458453 did not fix this.  Now I get three assertions:

###!!! ASSERTION: GetPrimaryFrameFor() called while frames are being destroyed!: '!mIsDestroyingFrames', file /Users/jruderman/central/layout/base/nsFrameManager.cpp, line 336

###!!! ASSERTION: Cached frame is incorrect!: 'mElement.get() && static_cast<nsGenericElement*>(mElement.get())->GetPrimaryFrame() == mReferencedFrame', file /Users/jruderman/central/layout/svg/base/src/nsSVGEffects.cpp, line 81

###!!! ASSERTION: PostRestyleEvent after the shell is destroyed (bug 279505): 'Not Reached', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 13470

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

As per comment https://bugzilla.mozilla.org/show_bug.cgi?id=279923#c13 in October 2008, this is the only assertion left on the Firefox leak boxes. Requesting blocking-1.9.1? because this prevents turning assertions fatal on the boxes.

Comment 5

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

(In reply to comment #4)
> As per comment https://bugzilla.mozilla.org/show_bug.cgi?id=279923#c13 in
> October 2008, this is the only assertion left on the Firefox leak boxes.
> Requesting blocking-1.9.1? because this prevents turning assertions fatal on
> the boxes.

Assertions should now be fatal on the Firefox leak boxes since bug 463681 landed.

Comment 6

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

Attachment: fix

The main fix here is to avoid posting a style-change to the frame constructor if the frame constructor thinks it's tearing down the frame tree. Instead of asking the presshell if it's being destroyed, we should ask the frame constructor specifically if it's torn down the frame tree, as that catches the document-reconstruction case too.

The other fix here is just to avoid callling GetPrimaryFrameFor in the NS_ASSERTION that checks the cached frame value, if it's unsafe to do so.

Comment 7

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

Pushed http://hg.mozilla.org/mozilla-central/rev/4167419a5f4a including a crashtest.

Comment 8

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

This caused 478128 so not landing on 1.9.1 until that bug is fixed.

Comment 9

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

Backed out: http://hg.mozilla.org/mozilla-central/rev/989bd3bc6cdf

Comment 10

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

Attachment: fix #2

There are two fixes here. One is like the last patch, just guard the assertion containing GetPrimaryFrameFor so that we don't test it when it's a bad time to call GetPrimaryFrameFor.

The other fix is to have PostRestyleEvent check mPresShell->IsDestroying() instead of mIsDestroyingFrameTree. The latter is also set when we're reconstructing the frame tree, and in that case I think it's fine, in fact good, to proceed with PostRestyleEvent. Also, if we do bail out, I think the bail-out should be silent; swallowing restyles that are sure to be irrelevant is perfectly safe. Code like nsSVGFilter/MaskProperty::DoUpdate that calls PostRestyleEvent to ensure that stuff gets repainted should be able to do that at any time without triggering errors and without having to explicitly guard the call with !presshell->IsDestroying().

I also removed the parameter from WillDestroyFrameTree because it can just call presshell->IsDestroying() to know what's going on.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 370169 [details] [diff] [review]
fix #2

I should really get rid of ReconstructDocElementHierarchy....

Comment 12

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

http://hg.mozilla.org/mozilla-central/rev/adbc4043e8bd

Comment 13

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

Sorry no, that was actually http://hg.mozilla.org/mozilla-central/rev/7157cc2440bb

Comment 14

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/937efb801b34

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED (no assertions) using the attached testcase on debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028