Closed Bug 455314 Opened 11 years ago Closed 11 years ago

Crash [@ nsCSSFrameConstructor::FindFrameWithContent] with filter, -moz-appearance and moving body

Categories

(Core :: SVG, defect, P2, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: martijn.martijn, Assigned: roc)

References

Details

(Keywords: crash, testcase, verified1.9.1)

Crash Data

Attachments

(4 files, 3 obsolete files)

Attached file testcase
See testcase, which crashes current trunk build after 100ms.

In a debug build, I see this assertion before the crash:
###!!! ASSERTION: No callbacks should be registered while we set up this entry:
'!entry->HasContentChangeCallback()', file c:/mozilla-build-1.3/mozilla-central/
content/base/src/nsDocument.cpp, line 3292

And then the crash:
>	gklayout.dll!nsCSSFrameConstructor::FindFrameWithContent(nsFrameManager * aFrameManager=0x0d56e644, nsIFrame * aParentFrame=0x0d590de4, nsIContent * aParentContent=0x0d591bd0, nsIContent * aContent=0x0d591f50, nsFindFrameHint * aHint=0x00000000)  Line 10673 + 0xc bytes	C++
 	gklayout.dll!nsCSSFrameConstructor::FindPrimaryFrameFor(nsFrameManager * aFrameManager=0x0d56e644, nsIContent * aContent=0x0d591f50, nsIFrame * * aFrame=0x0012df94, nsFindFrameHint * aHint=0x00000000)  Line 10789 + 0x21 bytes	C++
 	gklayout.dll!nsFrameManager::GetPrimaryFrameFor(nsIContent * aContent=0x0d591f50, int aIndexHint=-1)  Line 397	C++
 	gklayout.dll!PresShell::GetPrimaryFrameFor(nsIContent * aContent=0x0d591f50)  Line 4902	C++
 	gklayout.dll!nsGenericElement::GetPrimaryFrameFor(nsIContent * aContent=0x0d591f50, nsIDocument * aDocument=0x0d561718)  Line 3493	C++
 	gklayout.dll!nsGenericElement::GetPrimaryFrame()  Line 3463 + 0xd bytes	C++
 	gklayout.dll!nsSVGPropertyBase::GetReferencedFrame(nsIAtom * aFrameType=0x06b90918, int * aOK=0x0012e010)  Line 67 + 0x12 bytes	C++
 	gklayout.dll!nsSVGFilterProperty::GetFilterFrame(int * aOK=0x0012e010)  Line 109	C++
 	gklayout.dll!nsSVGEffects::GetFilterFrame(nsIFrame * aFrame=0x0d590be8)  Line 193 + 0x12 bytes	C++
 	gklayout.dll!nsSVGIntegrationUtils::GetInvalidAreaForChangedSource(nsIFrame * aFrame=0x0d590be8, const nsRect & aInvalidRect={...})  Line 141 + 0x9 bytes	C++
 	gklayout.dll!nsIFrame::InvalidateInternal(const nsRect & aDamageRect={...}, int aX=0, int aY=0, nsIFrame * aForChild=0x0d590be8, int aImmediate=0)  Line 3709 + 0x2c bytes	C++
etc...


http://crash-stats.mozilla.com/report/pending/be2b10d7-8327-11dd-998a-001cc45a2c28
(not there yet)
Yeah, GetPrimaryFrame() during frame destruction is unhealthy...

I have a WIP that fixes it by explicitly invalidating all frames
we're about to delete in DeletingFrameSubtree() and then suppressing
painting while the actual destruction takes place.  roc, would that
work do you think?
Flags: blocking1.9.1?
OS: Windows XP → All
Hardware: PC → All
I've got a big change in my branch that changes the way the relationship between frames and SVG effects is managed. Let me dig that out and see if it affects this.
I guess this will be fixed by bug 455984.
Depends on: 455984
Blocks: 454945
Bug still occurs.  roc, do you have more pending changes that will affect this?
Depends on: 458493
longsonr has a patch in bug 458493.
Assignee: nobody → longsonr
Not any more I don't :-(
Assignee: longsonr → nobody
No longer depends on: 458493
Blocks: 458453
It's the DoUpdate in nsSVGRenderingObserver::InvalidateViaReferencedFrame that's killing us.

I could add a PRPackedBool mDestroyingReferencedFrame to nsSVGRenderingObserver which would be set to true just before the DoUpdate and false afterwards in that function then in nsSVGRenderingObserver::GetReferencedFrame the code would check that variable and return null.

Alternatively maybe nsSVGFilterProperty::DoUpdate could be changed so that rather than calling InvalidateAllContinuations which blows up, maybe it could do something along the lines of what happens for SVG frames i.e. calling PostRestyleEvent. I don't know enough about non-SVG reflows to write that though.
So the question is, (In reply to comment #9)
> It's the DoUpdate in nsSVGRenderingObserver::InvalidateViaReferencedFrame
> that's killing us.
> 
> I could add a PRPackedBool mDestroyingReferencedFrame to nsSVGRenderingObserver
> which would be set to true just before the DoUpdate and false afterwards in
> that function then in nsSVGRenderingObserver::GetReferencedFrame the code would
> check that variable and return null.

With that approach, if the frame for the referenced element is destroyed and recreated, we won't do any repainting, I guess.

> Alternatively maybe nsSVGFilterProperty::DoUpdate could be changed so that
> rather than calling InvalidateAllContinuations which blows up, maybe it could
> do something along the lines of what happens for SVG frames i.e. calling
> PostRestyleEvent. I don't know enough about non-SVG reflows to write that
> though.

That's the way to go. The same code should work as for the SVG case (the outerSVGFrame check is not needed in either case, as far as I can tell). The only difference is that you'll need to add nsChangeHint_ReflowFrame for non-SVG frames.
Blocks: 462369
(In reply to comment #10)

> > Alternatively maybe nsSVGFilterProperty::DoUpdate could be changed so that
> > rather than calling InvalidateAllContinuations which blows up, maybe it could
> > do something along the lines of what happens for SVG frames i.e. calling
> > PostRestyleEvent. I don't know enough about non-SVG reflows to write that
> > though.
> 
> That's the way to go. The same code should work as for the SVG case (the
> outerSVGFrame check is not needed in either case, as far as I can tell). The
> only difference is that you'll need to add nsChangeHint_ReflowFrame for non-SVG
> frames.

That change was checked in under another bug. It fixed the crash there but this bug still crashes.
Flags: blocking1.9.1? → blocking1.9.1+
Assignee: nobody → roc
"ASSERTION: No callbacks should be registered while we set up this entry:" happens when we do a nsDocument::RemoveIDTargetObserver after the element has been removed from the document; it calls GetElementByIdInternal which asserts because the callback is there. RemoveIDTargetObserver doesn't need to call GetElementByIdInternal, it can just look in mIdentifierMap directly, because if there's no entry already there then there's no callback to remove. Easy fix, but it doesn't stop the crash.

I'm not sure what the right way to fix the crash is. Maybe add an extra parameter to GetReferencedFrame to indicate whether it's OK to actually go out and get the frame if we don't have a cached frame. This would be set to true for calls from painting and reflow, but false for calls via Invalidate.
That means, in this testcase, we'll need to be able to handle "no filter frame available" --- I guess by invalidating the filtered frame's entire overflow area just to be sure.
That solution is ugly. A better solution might be to expose mIsDestroyingFrames in nsFrameManager (making it non-#ifdef DEBUG) and then test that in nsSVGIntegrationUtils::GetInvalidAreaForChangedSource, just returning aFrame->GetOverflowRect().
Actually we need to test it in nsSVGRenderingObserver::GetReferencedFrame.
Attached patch fix (obsolete) — Splinter Review
This fixes the crash and assertion. The bugs this blocks are fixed too.
Attached patch part 1Splinter Review
Fix "ASSERTION: No callbacks should be registered while we set up this entry"
Attachment #349357 - Attachment is obsolete: true
Attachment #349377 - Flags: superreview?(jst)
Attachment #349377 - Flags: review?(jst)
Attachment #349377 - Attachment is patch: true
Attachment #349377 - Attachment mime type: application/text → text/plain
Attached patch part 2 (obsolete) — Splinter Review
Don't try to look up mReferencedFrame during frame removal
Attachment #349380 - Flags: superreview?(mats.palmgren)
Attachment #349380 - Flags: review?(mats.palmgren)
Whiteboard: [needs review]
Attachment #349380 - Attachment is patch: true
Attachment #349380 - Attachment mime type: application/text → text/plain
part 2 is the same patch as part 1.
Attachment #349377 - Flags: superreview?(jst)
Attachment #349377 - Flags: superreview+
Attachment #349377 - Flags: review?(jst)
Attachment #349377 - Flags: review+
Attached patch part 2 for real (obsolete) — Splinter Review
I hate it when that happens.
Attachment #349380 - Attachment is obsolete: true
Attachment #349462 - Flags: superreview?(mats.palmgren)
Attachment #349462 - Flags: review?(mats.palmgren)
Attachment #349380 - Flags: superreview?(mats.palmgren)
Attachment #349380 - Flags: review?(mats.palmgren)
I'd like huge to land bug 463681 soon, which depends on this bug by way of 459666 and 458453. Any chance of a review here soon ?
Comment on attachment 349462 [details] [diff] [review]
part 2 for real

This looks like the 1st patch for bug 459613?
Attachment #349462 - Flags: superreview?(mats.palmgren)
Attachment #349462 - Flags: review?(mats.palmgren)
Attached patch part 2Splinter Review
Attachment #349462 - Attachment is obsolete: true
Attachment #352418 - Flags: superreview?(mats.palmgren)
Attachment #352418 - Flags: review?(mats.palmgren)
Comment on attachment 352418 [details] [diff] [review]
part 2

r+sr=mats with the following nit:

> layout/svg/base/src/nsSVGEffects.cpp
>+    nsIDocument* doc = mElement.get()->GetCurrentDoc();
>+    nsIPresShell *shell = doc ? doc->GetPrimaryShell() : nsnull;

Put the * on the same side.
Attachment #352418 - Flags: superreview?(mats.palmgren)
Attachment #352418 - Flags: superreview+
Attachment #352418 - Flags: review?(mats.palmgren)
Attachment #352418 - Flags: review+
Whiteboard: [needs review] → [needs landing]
Pushed b77c1308a6d6 and b2355b2d9801 to trunk.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081212 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Verified fixed on 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204034421
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Crash Signature: [@ nsCSSFrameConstructor::FindFrameWithContent]
You need to log in before you can comment on or make changes to this bug.