Closed Bug 459666 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, verified1.9.1)

Attachments

(3 files, 1 obsolete file)

Attached file 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?
Summary: "ASSERTION: GetPrimaryFrameFor() called while frames are being destroyed!" with → "ASSERTION: GetPrimaryFrameFor() called while frames are being destroyed!" with filter in HTML
Bug 458453 fixes this. It's the issue I pointed out in bug 458453 comment 2.
Depends on: 458453
Blocks: 279923
Blocks: 463681
Assignee: nobody → mats.palmgren
OS: Mac OS X → All
Hardware: PC → All
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
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.
Flags: blocking1.9.1?
(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.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee: mats.palmgren → roc
Blocks: 463681
No longer blocks: 279923
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #361178 - Flags: superreview?(bzbarsky)
Attachment #361178 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
Attachment #361178 - Flags: superreview?(bzbarsky)
Attachment #361178 - Flags: superreview+
Attachment #361178 - Flags: review?(bzbarsky)
Attachment #361178 - Flags: review+
Whiteboard: [needs review] → [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/4167419a5f4a including a crashtest.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Depends on: 478128
This caused 478128 so not landing on 1.9.1 until that bug is fixed.
Backed out: http://hg.mozilla.org/mozilla-central/rev/989bd3bc6cdf
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Whiteboard: [needs 191 landing] → [needs new patch]
Whiteboard: [needs new patch]
Whiteboard: [needs review]
Attached patch fix #2Splinter Review
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.
Attachment #361178 - Attachment is obsolete: true
Attachment #370169 - Flags: superreview?(bzbarsky)
Attachment #370169 - Flags: review?(bzbarsky)
Comment on attachment 370169 [details] [diff] [review]
fix #2

I should really get rid of ReconstructDocElementHierarchy....
Attachment #370169 - Flags: superreview?(bzbarsky)
Attachment #370169 - Flags: superreview+
Attachment #370169 - Flags: review?(bzbarsky)
Attachment #370169 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/adbc4043e8bd
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 191 landing]
Flags: in-testsuite+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.