Closed
Bug 455314
Opened 16 years ago
Closed 16 years ago
Crash [@ nsCSSFrameConstructor::FindFrameWithContent] with filter, -moz-appearance and moving body
Categories
(Core :: SVG, defect, P2)
Core
SVG
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)
375 bytes,
application/xhtml+xml
|
Details | |
6.03 KB,
text/plain
|
Details | |
951 bytes,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
5.92 KB,
patch
|
MatsPalmgren_bugz
:
review+
MatsPalmgren_bugz
:
superreview+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•16 years ago
|
||
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?
Flags: blocking1.9.1?
OS: Windows XP → All
Hardware: PC → All
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.
Comment 5•16 years ago
|
||
Bug still occurs. roc, do you have more pending changes that will affect this?
Assignee | ||
Comment 6•16 years ago
|
||
No.
Comment 8•16 years ago
|
||
Not any more I don't :-(
Assignee: longsonr → nobody
No longer depends on: 458493
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.
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.
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.
Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Assignee | ||
Updated•16 years ago
|
Priority: -- → P2
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → roc
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.
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.
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().
Assignee | ||
Comment 15•16 years ago
|
||
Actually we need to test it in nsSVGRenderingObserver::GetReferencedFrame.
Assignee | ||
Comment 16•16 years ago
|
||
This fixes the crash and assertion. The bugs this blocks are fixed too.
Assignee | ||
Comment 17•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #349377 -
Attachment is patch: true
Attachment #349377 -
Attachment mime type: application/text → text/plain
Assignee | ||
Comment 18•16 years ago
|
||
Don't try to look up mReferencedFrame during frame removal
Attachment #349380 -
Flags: superreview?(mats.palmgren)
Attachment #349380 -
Flags: review?(mats.palmgren)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review]
Updated•16 years ago
|
Attachment #349380 -
Attachment is patch: true
Attachment #349380 -
Attachment mime type: application/text → text/plain
Comment 19•16 years ago
|
||
part 2 is the same patch as part 1.
Updated•16 years ago
|
Attachment #349377 -
Flags: superreview?(jst)
Attachment #349377 -
Flags: superreview+
Attachment #349377 -
Flags: review?(jst)
Attachment #349377 -
Flags: review+
Assignee | ||
Comment 20•16 years ago
|
||
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)
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 ?
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?
Attachment #349462 -
Flags: superreview?(mats.palmgren)
Attachment #349462 -
Flags: review?(mats.palmgren)
Assignee | ||
Comment 23•16 years ago
|
||
Attachment #349462 -
Attachment is obsolete: true
Attachment #352418 -
Flags: superreview?(mats.palmgren)
Attachment #352418 -
Flags: review?(mats.palmgren)
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.
Attachment #352418 -
Flags: superreview?(mats.palmgren)
Attachment #352418 -
Flags: superreview+
Attachment #352418 -
Flags: review?(mats.palmgren)
Attachment #352418 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review] → [needs landing]
Assignee | ||
Comment 25•16 years ago
|
||
Pushed b77c1308a6d6 and b2355b2d9801 to trunk.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
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
Status: RESOLVED → VERIFIED
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3fe9999f5520 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9441fd649e42
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
Keywords: fixed1.9.1 → verified1.9.1
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Updated•13 years ago
|
Crash Signature: [@ nsCSSFrameConstructor::FindFrameWithContent]
You need to log in
before you can comment on or make changes to this bug.
Description
•