Closed
Bug 843917
Opened 12 years ago
Closed 12 years ago
"ASSERTION: destroy called on frame while scripts not blocked"
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: jruderman, Assigned: heycam)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files, 1 obsolete file)
With:
user_pref("svg.text.css-frames.enabled", true);
###!!! ASSERTION: destroy called on frame while scripts not blocked: '!nsContentUtils::IsSafeToRunScript()', file layout/generic/nsFrame.cpp, line 610
It might be important that the second character is both astral and missing, and I might not have picked the best codepoint to guarantee that it's missing.
Reporter | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
roc, I tried using a script blocker in nsSVGTextFrame2::DoReflow, but that then just resulted in an assertion saying that frame destruction should only happen while in an update. So I added this. Does this make sense at all? I don't really know what mozAutoDocUpdate is for, apart from noting that an update is in progress, or whether it really makes sense to use it underneath the nsDocument::EndUpdate call. It does prevent any assertions with this test, and doesn't cause problems with any of my other tests, though...
That sounds like a question for Boris.
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 4•12 years ago
|
||
Which assert are you hitting for the "frame destruction" bit?
In any case, the way presshell does this is as follows:
1) An nsScriptBlocker on the stack.
2) WillDoReflow and DidDoReflow calls (with the latter happening after the
scriptblocker has gone out of scope, which seems slightly fishy but may actually
be required).
I really doubt you want to be doing a document-level update here...
One other note: at the point when the scriptblocker goes out of scope script will be able to run. So make sure to put that high enough up your callstack.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 729405 [details] [diff] [review]
patch
Thanks for the info Boris.
As it turns out, others changes I've made have avoided this assertion being triggered, so no need for the patch on this bug.
Attachment #729405 -
Flags: review?(roc)
Assignee | ||
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
Assignee | ||
Comment 6•12 years ago
|
||
(Not true; I didn't realise I had the patch applied locally.)
So here is what happens. I hit that assertion at the top of nsFrame::DestroyFrom nsFrame.cpp:597 with this on the stack:
http://www.pastebin.mozilla.org/2297671
where the AppendChild at the bottom is called from script. The GlyphMetricsUpdater gets run at this point because:
* when the AppendChild DOM mutation happens, we call nsSVGTextFrame2::NotifyGlyphMetricsChange
* this adds a GlyphMetricsUpdater script runner, since we don't want to do its work immediately after the DOM mutation but before the frames under the nsSVGTextFrame2 have been reflowed
* the reason we want to delay the GlyphMetricsUpdater is that it will call nsSVGUtils::InvalidateBounds on the nsSVGTextFrame2, but when a <text> is filtered, the frames beneath it must be up to date so that we can report a bounding box to the filter so our filter region can be computed to invalidate
So this setup seems to work, and inside nsSVGTextFrame2::GetBBoxContribution we'll reflow its anonymous block child, since we need this up to date to compute our bbox contribution. The assertion is hit because we don't have a script blocker set at the time the reflow of these frames happens.
Now, if I wrap the guts of nsSVGTextFrame2::DoReflow with a nsContentUtils::{Add,Remove}ScriptBlocker pair, this avoids the assertion but lands me in this:
#!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file /z/moz/c/layout/base/nsCSSFrameConstructor.cpp, line 1485
shortly after, which is at the top of nsCSSFrameConstructor::NotifyDestroyingFrame.
As it happens, using the document-level update got me in trouble with a different bug (it triggered a DOM generation increment while I'm in the middle of PaintInactiveLayer, which caused another assertion about not modifying the DOM in the middle of a paint).
So I'm confused. Is it fundamentally wrong to be reflowing my child anonymous block frame at this point? Is it wrong for nsSVGUtils::InvalidateBounds to rely on the anonymous block frame being reflowed? Should I instead be dispatching a runnable to do everything from mozilla::GlyphMetricsUpdater::Run downwards even later than when the script blocker is removed at the end of the AppendChild call? And if so, is there a way I could do this so that it does this before the next paint?
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 7•12 years ago
|
||
> Is it fundamentally wrong to be reflowing my child anonymous block frame at this point?
At first glance, probably not.
> Is it wrong for nsSVGUtils::InvalidateBounds to rely on the anonymous block frame being
> reflowed?
Imo, somewhat, mostly as a matter of performance: forcing sync reflow is not great.
> Should I instead be dispatching a runnable
No.
All you have to do is do the dance that reflow expects to happen; see comment 4.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #7)
> All you have to do is do the dance that reflow expects to happen; see
> comment 4.
I tried with an nsAutoScriptBlocker:
http://pastebin.mozilla.org/2297768
and even with it going out of script before the DidReflow, I get the "Should be in an update while destroying frames" assertion. Did I miss some steps in the dance?
![]() |
||
Comment 9•12 years ago
|
||
Backing up a bit, why do you need the nsSVGUtils::InvalidateBounds call here? You shouldn't need it any more? I'd suggest that you remove it, and replace it with:
nsSVGEffects::InvalidateRenderingObservers(aFrame);
If you have invalidation issues as a result of doing that then you can also try adding:
if (!(aFrame->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD)) {
aFrame->InvalidateFrameSubtree();
}
but if you have to add that, we'll need to figure out why it's necessary.
Anyway, ping me on IRC and I can help work through this.
![]() |
||
Comment 10•12 years ago
|
||
> Did I miss some steps in the dance?
Yes. Calling PresShell::WillReflow/DidReflow, which is what starts and stops updates and does some other bookkeeping.
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #10)
> Yes. Calling PresShell::WillReflow/DidReflow, which is what starts and
> stops updates and does some other bookkeeping.
Those methods don't seem to exist; I was calling methods named like these on the frame itself, though.
But...
(In reply to Jonathan Watt [:jwatt] from comment #9)
> Backing up a bit, why do you need the nsSVGUtils::InvalidateBounds call
> here? You shouldn't need it any more? I'd suggest that you remove it, and
> replace it with:
>
> nsSVGEffects::InvalidateRenderingObservers(aFrame);
This works for me.
Jonathan, I guess I don't understand why InvalidateBounds() is not required at all. Or is it just that it's not required when it's a non-display child? Because GlyphMetricsUpdater::Run will be called even for changes to normally displayed <text> elements. Is it because these changes will cause a reflow of the child frame, and will get invalidated due to that? (Which is different from other SVG frame classes, where we really do need to call InvalidateBounds() in response to for example attribute changes?)
Flags: needinfo?(jwatt)
![]() |
||
Comment 12•12 years ago
|
||
DLBI should take care of invalidating the areas that have changed nowadays, as long as it's informed that it should create a new display list to compare with the previous one (something that it will be informed about under the FrameNeedsReflow() call). This is the case for displayed content (non-display content doesn't need to invalidate, it just needs to tell any rendering observers that they should invalidate). The fact that other SVG frames need to call InvalidateBounds() is a bug that I'm hoping to get a chance to work on very soon.
Flags: needinfo?(jwatt)
Assignee | ||
Comment 13•12 years ago
|
||
Thanks, that makes sense. I don't have any problems with normal text invalidation without the InvalidateBounds() call.
Also, running the SVG text reftests locally for me is now a lot faster with this patch. Before it must have been doing too much invalidation work.
Attachment #729405 -
Attachment is obsolete: true
Attachment #737373 -
Flags: review?(jwatt)
![]() |
||
Comment 14•12 years ago
|
||
Comment on attachment 737373 [details] [diff] [review]
patch (v2)
(In reply to Cameron McCormack (:heycam) from comment #13)
> Also, running the SVG text reftests locally for me is now a lot faster with
> this patch. Before it must have been doing too much invalidation work.
Yeah, in cases where there aren't too many elements to display, DLBI is much faster.
Attachment #737373 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 15•12 years ago
|
||
Comment 16•12 years ago
|
||
Can you check in the testcase as a crashtest please?
Assignee | ||
Comment 17•12 years ago
|
||
Flags: in-testsuite+
![]() |
||
Comment 18•12 years ago
|
||
> Those methods don't seem to exist;
Er, I mean WillDoReflow/DidDoReflow. :( I got it right in comment 4!
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1077b98a75f1
https://hg.mozilla.org/mozilla-central/rev/53c2e7b9753b
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•