"ASSERTION: destroy called on frame while scripts not blocked"

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jruderman, Assigned: heycam)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla23
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted image testcase
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.
Posted file stack (gdb)
Posted patch patch (obsolete) — Splinter Review
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...
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #729405 - Flags: review?(roc)
That sounds like a question for Boris.
Flags: needinfo?(bzbarsky)
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)
Blocks: 839955
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)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
(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 → ---
Flags: needinfo?(bzbarsky)
> 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)
(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?
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.
> 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.
(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)
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)
Posted patch patch (v2)Splinter Review
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 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+
Can you check in the testcase as a crashtest please?
> Those methods don't seem to exist;

Er, I mean WillDoReflow/DidDoReflow.  :(  I got it right in comment 4!
https://hg.mozilla.org/mozilla-central/rev/1077b98a75f1
https://hg.mozilla.org/mozilla-central/rev/53c2e7b9753b
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.