Martijn Wargers (dead) Reporter
	Description • 16 years ago

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)

	Mats Palmgren (inactive)
	Comment 2 • 16 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 3 • 16 years ago

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.

	Martijn Wargers (dead) Reporter
	Comment 4 • 16 years ago

I guess this will be fixed by bug 455984.

	Mats Palmgren (inactive)
	Comment 5 • 16 years ago

Bug still occurs.  roc, do you have more pending changes that will affect this?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 16 years ago

No.

	Jesse Ruderman
	Comment 7 • 16 years ago

longsonr has a patch in bug 458493.

	Robert Longson [:longsonr]
	Comment 8 • 16 years ago

Not any more I don't :-(

	Robert Longson [:longsonr]
	Comment 9 • 16 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 10 • 16 years ago

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.

	Robert Longson [:longsonr]
	Comment 11 • 16 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 12 • 16 years ago

"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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 13 • 16 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 16 years ago

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().

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 15 • 16 years ago

Actually we need to test it in nsSVGRenderingObserver::GetReferencedFrame.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 16 years ago

This fixes the crash and assertion. The bugs this blocks are fixed too.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 17 • 16 years ago

Fix "ASSERTION: No callbacks should be registered while we set up this entry"

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 18 • 16 years ago

Don't try to look up mReferencedFrame during frame removal

	Robert Longson [:longsonr]
	Comment 19 • 16 years ago

part 2 is the same patch as part 1.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 20 • 16 years ago

I hate it when that happens.

	Nick Thomas [:nthomas] (UTC+12)
	Comment 21 • 16 years ago

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 ?

	Mats Palmgren (inactive)
	Comment 22 • 16 years ago

Comment on attachment 349462 [details] [diff] [review]
part 2 for real

This looks like the 1st patch for bug 459613?

	Mats Palmgren (inactive)
	Comment 24 • 16 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 25 • 16 years ago

Pushed b77c1308a6d6 and b2355b2d9801 to trunk.

	Martijn Wargers (dead) Reporter
	Comment 26 • 16 years ago

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081212 Minefield/3.2a1pre

	David Baron :dbaron:
	Comment 27 • 16 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3fe9999f5520
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9441fd649e42

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 28 • 16 years ago

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